feat: add Commerce Escrow payment functionality #1649
Conversation
…figurations - Introduced Commerce Escrow payment types and parameters in the payment-types module. - Added Commerce Escrow wrapper to smart contracts and updated related scripts for deployment and verification. - Updated Docker Compose file to specify platform for services. - Added commerce-payments dependency in smart contracts package.
WalkthroughThis pull request implements a comprehensive ERC20CommerceEscrowWrapper integration for Request Network's payment processor, adding a Solidity smart contract that wraps commerce escrow payments, TypeScript SDK functions for transaction orchestration, extensive test coverage (unit and property-based fuzzing), security analysis tooling (Slither and Echidna), and Base Sepolia testnet support with supporting infrastructure, types, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as TypeScript<br/>Payment Processor
participant Wrapper as ERC20CommerceEscrowWrapper
participant Escrow as Commerce Escrow
participant ERC20FeeProxy as ERC20FeeProxy
participant Token as ERC20 Token
rect rgb(200, 220, 255)
Note over User,Token: Authorization Phase
User->>Client: authorizePayment(params)
Client->>Wrapper: authorizePayment(paymentReference, payer, token, amount, maxAmount)
Wrapper->>Token: transferFrom(payer, escrow, amount)
Token-->>Wrapper: ✓
Wrapper->>Escrow: authorize(paymentInfo, amount, ...)
Escrow-->>Wrapper: ✓
Wrapper-->>Client: TransactionResponse
end
rect rgb(200, 255, 220)
Note over User,Token: Capture Phase (Merchant-Pays-Fee)
User->>Client: capturePayment(paymentReference, captureAmount, feeBps)
Client->>Wrapper: capturePayment(paymentReference, captureAmount, feeBps, feeReceiver)
Wrapper->>Escrow: capture(paymentInfo, captureAmount, ...)
Escrow-->>Wrapper: ✓
rect rgb(255, 240, 200)
Note over Wrapper,ERC20FeeProxy: Fee Calculation
Wrapper->>Wrapper: feeAmount = captureAmount × feeBps / 10000<br/>merchantAmount = captureAmount - feeAmount
end
Wrapper->>ERC20FeeProxy: transferFromWithReferenceAndFee(token, merchant, merchantAmount, paymentRef, feeAmount, feeReceiver)
ERC20FeeProxy->>Token: transferFrom(wrapper, merchant, merchantAmount)
Token-->>ERC20FeeProxy: ✓
ERC20FeeProxy->>Token: transferFrom(wrapper, feeReceiver, feeAmount)
Token-->>ERC20FeeProxy: ✓
Wrapper-->>Client: TransactionResponse
end
rect rgb(255, 220, 220)
Note over User,Token: Refund Phase
User->>Client: refundPayment(paymentReference, refundAmount)
Client->>Wrapper: refundPayment(paymentReference, refundAmount, ...)
Wrapper->>Escrow: refund(paymentInfo, refundAmount, ...)
Escrow-->>Wrapper: ✓
Wrapper->>ERC20FeeProxy: transferFromWithReferenceAndFee(token, payer, refundAmount, paymentRef, 0, address(0))
ERC20FeeProxy->>Token: transferFrom(wrapper, payer, refundAmount)
Token-->>ERC20FeeProxy: ✓
Wrapper-->>Client: TransactionResponse
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Comment |
…l parameters - Refactored `encodeAuthorizePayment` and `encodeChargePayment` functions to pass individual parameters instead of a struct. - Updated tests to reflect changes in parameter handling and added edge case scenarios for payment processing. - Adjusted network configurations in tests to use the Sepolia testnet. - Enhanced error handling for unsupported networks and invalid payment references in tests.
…s struct - Updated `encodeAuthorizePayment` and `encodeChargePayment` functions to accept a single params struct instead of individual parameters. - This change enhances code readability and maintainability by reducing parameter handling complexity.
…Interface - Updated `encodeAuthorizePayment` and `encodeChargePayment` functions to utilize `utils.Interface` for encoding, allowing for individual parameters to be passed instead of a struct. - This change improves compatibility with TypeScript and aligns with the ABI expectations for function calls.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (23)
docker-compose.yml (1)
4-4: Forcing linux/amd64 may slow Apple Silicon dev and CI runners; consider making it opt‑in.Set via DOCKER_DEFAULT_PLATFORM or a compose profile/env override instead of hardcoding, and pin image tags (e.g., postgres:16) for reproducibility.
Have you validated these services on arm64 lately? If not, we can add a profile like
profiles: ["amd64"]and documentdocker compose --profile amd64 up.Also applies to: 26-26, 34-34, 47-47, 58-58
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol (8)
515-536: Validate feeBps and clear allowance after transfer to limit ERC20 allowance exposure.
- Enforce
feeBps <= 10_000.- After
transferFromWithReferenceAndFee, reset allowance to 0.function _transferToMerchant( @@ - uint256 feeAmount = (amount * feeBps) / 10000; + if (feeBps > 10_000) revert InvalidFeeBps(); + uint256 feeAmount = (amount * feeBps) / 10_000; uint256 merchantAmount = amount - feeAmount; IERC20(token).forceApprove(address(erc20FeeProxy), amount); erc20FeeProxy.transferFromWithReferenceAndFee( @@ feeReceiver ); + // Clear residual allowance + IERC20(token).forceApprove(address(erc20FeeProxy), 0);
371-391: Also bound feeBps and clear allowance in capturePayment.Mirror the same protections when capturing.
- // Calculate fee amounts - ERC20FeeProxy will handle the split - uint256 feeAmount = (captureAmount * feeBps) / 10000; + if (feeBps > 10_000) revert InvalidFeeBps(); + // Calculate fee amounts - ERC20FeeProxy will handle the split + uint256 feeAmount = (captureAmount * feeBps) / 10_000; @@ erc20FeeProxy.transferFromWithReferenceAndFee( @@ ); + // Clear residual allowance + IERC20(payment.token).forceApprove(address(erc20FeeProxy), 0);
200-212: Add upstream validation: amount ≤ maxAmount to avoid wasted gas on escrow revert.Escrow likely enforces this; checking here improves UX.
function authorizePayment(AuthParams calldata params) external nonReentrant { @@ - // Create and execute authorization + if (params.amount > params.maxAmount) revert ScalarOverflow(); + // Create and execute authorization
346-350: Avoid external self‑call; call internal logic directly to save gas.Use
_executeAuthorization(params)and keep validations in this function. Current pattern is safe but costs an extra call.
400-438: Mark payment inactive after a successful void to prevent redundant calls and save lookups.This avoids later
onlyOperator/state calls on a finalized payment.commerceEscrow.void(paymentInfo); @@ emit PaymentVoided( @@ ); + payment.isActive = false;
538-576: Also mark payment inactive on reclaim.Same reasoning as void.
commerceEscrow.reclaim(paymentInfo); @@ emit PaymentReclaimed( @@ ); + payment.isActive = false;
584-627: Clear temporary approval after refund; optionally validate refundAmount > 0.Minimize allowance window; small input sanity check helps.
- IERC20(payment.token).forceApprove(tokenCollector, refundAmount); + IERC20(payment.token).forceApprove(tokenCollector, refundAmount); @@ commerceEscrow.refund(paymentInfo, refundAmount, tokenCollector, collectorData); + // Clear residual allowance + IERC20(payment.token).forceApprove(tokenCollector, 0);
173-184: Use a dedicated error for payer checks to improve debuggability.Reusing
InvalidOperatoris confusing.- if (msg.sender != payment.payer) { - revert InvalidOperator(msg.sender, payment.payer); // Reusing the same error for simplicity - } + if (msg.sender != payment.payer) { + revert InvalidPayer(msg.sender, payment.payer); + }Add:
error ZeroAddress(); +error InvalidPayer(address sender, address expectedPayer);packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts (4)
43-48: Stabilize time-dependent testsAvoid Date.now()/provider time drift. Use Hardhat time helpers to set block timestamps deterministically before calling state-changing functions. Example: setNextBlockTimestamp + mine.
Apply Hardhat helpers in beforeEach where expiries are computed to prevent flaky behavior across environments.
Also applies to: 1012-1032
435-447: Confirm event emitter (wrapper vs ERC20FeeProxy)Assertions expect TransferWithReferenceAndFee to be emitted by wrapper. If the event originates from ERC20FeeProxy, target that contract instead or have the wrapper re-emit. Please verify actual emitter to avoid false negatives.
Also applies to: 545-557
341-353: Tighten revert assertionsUse revertedWith/ to pin failure reason (e.g., non-operator, over-capture, nonexistent payment). This reduces accidental passes due to unrelated reverts.
Also applies to: 461-486, 659-662
767-787: “Reentrancy Protection” tests don’t verify nonReentrantThese tests only execute happy paths. If the goal is to assert the guard exists, check for the modifier via ABI or attempt a crafted reentrant call using a malicious token/mock. Otherwise, rename the describe block to reflect intent.
Also applies to: 789-833
packages/payment-processor/test/payment/erc20-commerce-escrow-wrapper.test.ts (4)
75-103: Decouple from concrete deployment addressesTests expect '0x123…' for sepolia/goerli/mumbai without mocking the source of truth. Stub the artifact getter or spy on getCommerceEscrowWrapperAddress to return deterministic values, or assert “is valid address” rather than equality.
81-92: Relax brittle error string matchesMultiple tests assert an exact message. Prefer regex (toThrow(/No deployment for network/)) or inline snapshots to avoid breakage from punctuation/wording changes while still validating semantics.
Also applies to: 252-262, 367-375, 818-826
265-297: Nice mocking pattern; minor isolation tweakThe jest.doMock + resetModules flow is good. Consider jest.isolateModules for tighter scoping and to avoid cross-test cache bleed if this file grows.
768-805: Gas price edge-cases: add explicit expectationsYou already mock gasPrice extremes. Consider also asserting sendTransaction call args include or omit gas parameters per your helpers’ behavior to catch accidental overrides.
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol (3)
5-6: Use SafeERC20 for robust token interactionstransfer/transferFrom return values are ignored; some tokens don’t revert on failure. Even for mocks, using SafeERC20 prevents silent failures and aligns with OZ best practices.
Apply SafeERC20 and replace calls with safeTransfer/safeTransferFrom.
Also applies to: 39-41, 66-71, 98-103, 117-125, 139-145, 159-167
15-17: Guard against uint120 downcast truncationExplicit casts from uint256 to uint120 can silently truncate. Add bounds checks (require(amount <= type(uint120).max, ...)) before casting or keep state as uint256 in the mock to avoid surprises in edge-case tests.
Also applies to: 45-47, 69-71, 97-103, 121-125, 164-165
43-47: Authorize sets hasCollectedPayment=trueSemantically “collected” often means captured to receiver. If this flag is intended to mean “funds held in escrow,” rename or document it. Otherwise set false on authorize and true only after capture/charge. Confirm expectations with wrapper tests.
packages/payment-processor/src/payment/erc20-commerce-escrow-wrapper.ts (3)
521-534: Avoid unsafe BigNumber → number conversions for timestampstoNumber() can overflow for large values. Either:
- return strings/BigNumbers for time fields, or
- add a safe guard before converting (throw if > Number.MAX_SAFE_INTEGER).
Would you like a small helper (toSafeNumber) and type update to prevent silent precision loss?
299-323: DRY the transaction helperssendTransaction boilerplate is duplicated. Extract a tiny sendToWrapper(network, signer, data) to reduce repetition and centralize future gas/nonce tweaks.
Also applies to: 334-358, 369-393, 404-428, 439-463, 474-498
71-104: USDT approve reset: consider making it token-behavior drivenYou require callers to pass isUSDT. Optionally detect non-standard approve behavior by token address (lookup) or expose a helper isUsdtLike(tokenAddress) in the payment-processor to avoid misconfiguration by integrators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
docker-compose.yml(4 hunks)packages/payment-processor/src/index.ts(1 hunks)packages/payment-processor/src/payment/erc20-commerce-escrow-wrapper.ts(1 hunks)packages/payment-processor/test/payment/erc20-commerce-escrow-wrapper.test.ts(1 hunks)packages/smart-contracts/package.json(1 hunks)packages/smart-contracts/scripts-create2/compute-one-address.ts(1 hunks)packages/smart-contracts/scripts-create2/constructor-args.ts(1 hunks)packages/smart-contracts/scripts-create2/utils.ts(2 hunks)packages/smart-contracts/scripts-create2/verify.ts(1 hunks)packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol(1 hunks)packages/smart-contracts/src/contracts/interfaces/IAuthCaptureEscrow.sol(1 hunks)packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol(1 hunks)packages/smart-contracts/src/lib/artifacts/ERC20CommerceEscrowWrapper/0.1.0.json(1 hunks)packages/smart-contracts/src/lib/artifacts/ERC20CommerceEscrowWrapper/index.ts(1 hunks)packages/smart-contracts/src/lib/artifacts/index.ts(1 hunks)packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts(1 hunks)packages/types/src/payment-types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/payment-processor/test/payment/erc20-commerce-escrow-wrapper.test.ts
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/payment-processor/test/payment/erc20-commerce-escrow-wrapper.test.ts
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-06T14:48:18.698Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1484
File: packages/advanced-logic/test/extensions/payment-network/any-to-near.test.ts:0-0
Timestamp: 2024-11-06T14:48:18.698Z
Learning: In `packages/advanced-logic/test/extensions/payment-network/any-to-near.test.ts`, when the existing happy path tests are deemed sufficient, additional test cases may not be necessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
🔇 Additional comments (9)
packages/smart-contracts/src/contracts/interfaces/IAuthCaptureEscrow.sol (1)
1-79: LGTM — clear interface and tight types.The struct packing (uint120/uint48) is good for gas; function set matches wrapper usage.
packages/payment-processor/src/index.ts (1)
33-33: Build configuration will correctly emit dist/payment/erc20-commerce-escrow-wrapper.js(.d.ts) — no issues found.Verification confirms:
- Source file exists with proper exports
- Re-export statement correctly placed in index.ts line 33
- tsconfig.build.json configured with outDir="dist" and rootDir="src", ensuring individual module files are emitted
- Tree-shaking preserved via
export *statementThe build will automatically create the expected dist artifacts on standard TypeScript compilation.
packages/smart-contracts/scripts-create2/verify.ts (1)
54-55: LGTM! Verification integration is consistent with existing contracts.The addition of both ERC20RecurringPaymentProxy and ERC20CommerceEscrowWrapper cases follows the established pattern and correctly reuses the shared verification logic.
packages/smart-contracts/src/lib/artifacts/index.ts (1)
19-19: LGTM! Export follows existing pattern.The export statement is correctly placed and maintains consistency with other artifact exports.
packages/smart-contracts/scripts-create2/compute-one-address.ts (1)
69-70: LGTM! Address computation integration is correct.The new cases properly integrate with the existing CREATE2 address computation flow.
packages/smart-contracts/scripts-create2/utils.ts (2)
25-25: LGTM! Deployment list updated correctly.
66-67: LGTM! Artifact resolution follows established pattern.packages/smart-contracts/src/lib/artifacts/ERC20CommerceEscrowWrapper/0.1.0.json (1)
1-853: LGTM! ABI definition is comprehensive and well-structured.The ABI correctly defines:
- Constructor parameters for commerce escrow and fee proxy dependencies
- Custom error types for validation failures
- Events covering the full authorize/capture/void/refund lifecycle
- Public functions for payment operations and state queries
- PaymentData struct matching the TypeScript type definitions
packages/types/src/payment-types.ts (1)
416-486: LGTM! Type definitions correctly model the Commerce Escrow contract interface.The new interfaces provide comprehensive type safety for:
- Payment data structure matching the on-chain PaymentData struct
- Authorization parameters with expiry timestamps and collector configuration
- Capture parameters with fee calculation (basis points)
- Charge parameters appropriately extending authorization with capture fees
- Refund parameters with collector data
- Payment state queries
Field types appropriately use
BigNumberishfor amounts andnumberfor timestamps and BPS values.
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
Outdated
Show resolved
Hide resolved
packages/smart-contracts/src/lib/artifacts/ERC20CommerceEscrowWrapper/index.ts
Show resolved
Hide resolved
… escrow address - Enhanced error messaging in `getCommerceEscrowWrapperAddress` for better clarity on network deployments. - Updated the placeholder commerce escrow address in `constructor-args.ts` to the actual deployed AuthCaptureEscrow address. - Added new `ScalarOverflow` error to `ERC20CommerceEscrowWrapper` for better overflow handling in payment parameters. - Adjusted payment processing logic to ensure no fees are taken at escrow, aligning with ERC20FeeProxy for compatibility.
…sts and event assertions - Added comprehensive tests for encoding functions in `erc20-commerce-escrow-wrapper` to verify function selectors and parameter inclusion. - Improved event assertions in `ERC20CommerceEscrowWrapper` tests to check emitted events with exact values for payment authorization, capture, voiding, charging, and reclaiming payments. - Validated function signatures and parameter types across various payment functions to ensure expected behavior.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol (1)
178-187: Consider dedicated error for payer validation.The
onlyPayermodifier reusesInvalidOperatorerror, which could confuse debugging. A dedicatedInvalidPayererror would improve clarity.Apply this diff if desired:
/// @notice Invalid operator for this payment error InvalidOperator(address sender, address expectedOperator); + /// @notice Invalid payer for this payment + error InvalidPayer(address sender, address expectedPayer);modifier onlyPayer(bytes8 paymentReference) { PaymentData storage payment = payments[paymentReference]; if (!payment.isActive) revert PaymentNotFound(); // Check if the caller is the payer for this payment if (msg.sender != payment.payer) { - revert InvalidOperator(msg.sender, payment.payer); // Reusing the same error for simplicity + revert InvalidPayer(msg.sender, payment.payer); } _; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/payment-processor/src/payment/erc20-commerce-escrow-wrapper.ts(1 hunks)packages/payment-processor/test/payment/erc20-commerce-escrow-wrapper.test.ts(1 hunks)packages/smart-contracts/package.json(1 hunks)packages/smart-contracts/scripts-create2/constructor-args.ts(1 hunks)packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol(1 hunks)packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/payment-processor/test/payment/erc20-commerce-escrow-wrapper.test.ts
- packages/smart-contracts/scripts-create2/constructor-args.ts
- packages/smart-contracts/package.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (14)
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts (4)
1-90: LGTM: Well-structured test setup.The test scaffolding properly deploys all contracts (TestERC20, ERC20FeeProxy, MockAuthCaptureEscrow, ERC20CommerceEscrowWrapper), allocates tokens, and sets up test participants. The unique payment reference generator is a good pattern for avoiding collisions across tests.
238-276: Verify that wrapper delegates validation to the escrow contract.Tests confirm the wrapper doesn't validate amount vs maxAmount relationships or expiry timestamp ordering. This suggests validation is delegated to the underlying
commerceEscrowcontract.Please confirm this is the intended design - that the wrapper trusts the escrow to enforce business rules while focusing on authorization, fee handling, and Request Network integration.
635-672: LGTM: Refund testing appropriately scoped.The test suite correctly focuses on access control for refunds at the unit level, with the comment noting that full refund flow testing requires integration tests with real contracts. This is a reasonable testing strategy given the complexity of the operator token pull flow.
906-1129: Excellent security test coverage.The attack vector tests comprehensively cover front-running protection, access control enforcement, overflow/underflow scenarios, and boundary conditions. This demonstrates a security-first testing approach.
packages/payment-processor/src/payment/erc20-commerce-escrow-wrapper.ts (4)
22-30: LGTM: Error message properly aligned.The error message now matches the expected format from tests and other call sites as requested in the previous review.
71-104: LGTM: Proper USDT allowance handling.The function correctly implements the USDT quirk by resetting allowance to zero before setting a new value. This prevents the well-known USDT approval race condition issue.
115-146: Acceptable workaround for TypeScript interface limitations.The manual parameter encoding using
utils.Interfaceand individual parameter passing avoids TypeScript struct mapping issues. While not ideal, this is a pragmatic solution that maintains type safety at the boundaries.
509-535: LGTM: Proper type conversions for consumer ergonomics.The function correctly converts BigNumber fields to more ergonomic JavaScript types, making the returned data easier to work with in TypeScript consumers.
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol (6)
270-300: LGTM: Overflow checks properly implemented.The explicit bounds checks before casting to smaller uint types (uint120, uint48) address the previous review concern about silent numeric truncation. This prevents large inputs from being truncated before hashing/authorizing.
469-526: LGTM: Double fee issue resolved.The charge flow now correctly applies fees only once via ERC20FeeProxy, passing zero fee parameters to
commerceEscrow.charge. This addresses the previous critical review concern about double fee application.
370-411: LGTM: Secure capture implementation with proper fee handling.The capture flow correctly:
- Enforces operator-only access
- Retrieves full amount from escrow without fees
- Delegates fee splitting to ERC20FeeProxy for Request Network event compatibility
- Uses
forceApprovefor USDT-like token compatibility
415-589: LGTM: Void and reclaim flows properly delegated.Both functions correctly:
- Enforce appropriate access control (operator for void, payer for reclaim)
- Query state before the operation
- Delegate actual token transfers to the escrow contract
- Emit Request Network-compatible events for integration
596-640: LGTM: Refund flow handles multi-party token movement correctly.The refund implementation properly orchestrates the token flow from the operator through the wrapper to the collector and ultimately to the payer, with appropriate approvals at each step.
642-689: LGTM: Clean view function implementations.The query functions appropriately check payment existence and delegate state queries to the underlying escrow contract. The capability checks (
canCapture,canVoid) provide useful consumer utilities.
…wWrapper tests - Improved event assertions for payment authorization, capture, voiding, charging, and reclaiming payments to verify emitted events with exact values. - Updated tests to utilize transaction receipts for event validation, ensuring accurate checks for emitted event arguments. - Removed unnecessary assertions for parameters not stored in the PaymentData struct.
…Wrapper tests - Replaced direct event assertions with `expect(tx).to.emit` for `CaptureCalled` and `ChargeCalled` events to enhance clarity and maintainability. - Removed redundant checks for event parameters that are already validated through transaction receipts.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts (5)
44-47: Consider using blockchain timestamp for time constants.The time constants use
Date.now()which captures JavaScript execution time rather than blockchain time. For more accurate testing, consider obtaining the current block timestamp from the provider. While this may not affect the current tests if the wrapper doesn't validate expiry times, it's a best practice for time-dependent smart contract testing.Apply this diff to use blockchain time:
- // Time constants - const currentTime = Math.floor(Date.now() / 1000); - const preApprovalExpiry = currentTime + 3600; // 1 hour - const authorizationExpiry = currentTime + 7200; // 2 hours - const refundExpiry = currentTime + 86400; // 24 hours + // Time constants - will be set in before hook + let currentTime: number; + let preApprovalExpiry: number; + let authorizationExpiry: number; + let refundExpiry: number;Then in the
beforehook after deploying contracts:// Get current blockchain time const currentBlock = await ethers.provider.getBlock('latest'); currentTime = currentBlock.timestamp; preApprovalExpiry = currentTime + 3600; // 1 hour authorizationExpiry = currentTime + 7200; // 2 hours refundExpiry = currentTime + 86400; // 24 hours
415-432: Consider verifying remaining capturable amount in partial capture test.The partial capture test successfully verifies that multiple captures can be performed, but doesn't check the remaining capturable amount after each operation. Consider adding state verification to ensure the capturable amount decreases correctly.
Based on learnings
Add state verification:
// First partial capture await expect( wrapper .connect(operator) .capturePayment(authParams.paymentReference, firstCapture, feeBps, feeReceiverAddress), ).to.emit(wrapper, 'PaymentCaptured'); + + // Verify remaining capturable amount + let [hasCollected, capturable, refundable] = await wrapper.getPaymentState( + authParams.paymentReference, + ); + expect(capturable).to.equal(amount.sub(firstCapture)); // Second partial capture await expect( wrapper .connect(operator) .capturePayment(authParams.paymentReference, secondCapture, feeBps, feeReceiverAddress), ).to.emit(wrapper, 'PaymentCaptured'); + + // Verify final capturable amount + [hasCollected, capturable, refundable] = await wrapper.getPaymentState( + authParams.paymentReference, + ); + expect(capturable).to.equal(amount.sub(firstCapture).sub(secondCapture));
519-564: Charge functionality has minimal test coverage.The charge tests only cover the happy path and invalid reference scenarios. Consider adding tests for:
- Access control (who can call chargePayment)
- Edge cases (zero amounts, fee calculations, etc.)
- State verification after charging
If charge is a critical payment path, more comprehensive testing would improve confidence.
566-612: Reclaim tests cover core functionality.The reclaim tests verify the happy path (payer can reclaim their authorized payment) and access control (non-payers cannot reclaim). Consider adding edge case tests for reclaim after partial/full capture if this is a critical flow.
1017-1038: Gas limit test doesn't verify gas consumption.The test is titled "Gas Limit Edge Cases" but only verifies that large collector data is accepted, not that it stays within gas limits. Consider either:
- Measuring actual gas consumption and asserting it's reasonable, OR
- Renaming to "Large Collector Data Handling"
To measure gas:
it('should handle large collector data', async () => { const largeData = '0x' + 'ff'.repeat(1000); // 1000 bytes of data const authParams = { paymentReference: getUniquePaymentReference(), payer: payerAddress, merchant: merchantAddress, operator: operatorAddress, token: testERC20.address, amount, maxAmount, preApprovalExpiry, authorizationExpiry, refundExpiry, tokenCollector: tokenCollectorAddress, collectorData: largeData, }; - await expect(wrapper.authorizePayment(authParams)).to.emit(wrapper, 'PaymentAuthorized'); + const tx = await wrapper.authorizePayment(authParams); + const receipt = await tx.wait(); + expect(receipt.gasUsed).to.be.lt(5000000); // Assert reasonable gas limit + await expect(tx).to.emit(wrapper, 'PaymentAuthorized'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
🔇 Additional comments (8)
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts (8)
91-123: LGTM! Comprehensive constructor validation tests.The constructor tests effectively cover initialization with valid addresses and zero address validation for both parameters. The tests ensure the contract properly validates its dependencies during deployment.
436-517: LGTM! Comprehensive void functionality tests.The void tests effectively cover the happy path, access control, and important edge cases including voiding after capture and double void attempts. The tests verify proper token return to the payer with no fees.
653-754: LGTM! Comprehensive view function tests.The view function tests thoroughly cover all getter methods with both valid and invalid payment references, including edge cases and state transitions. The tests properly verify the contract's read-only interface.
756-814: Edge case tests cover key scenarios.The edge case tests verify handling of zero amounts, large amounts, and empty collector data. Combined with the boundary value tests later in the file, this provides good coverage of edge conditions.
885-964: LGTM! Comprehensive security-focused tests.The attack vector tests effectively cover front-running protection (duplicate payment references) and access control attacks (unauthorized role access). These tests verify that the contract properly enforces role-based permissions and prevents common attack patterns.
1041-1108: LGTM! Thorough boundary value tests.The boundary value tests cover minimum amounts (1 wei), time boundaries using block timestamps, and maximum fee basis points (100%). Note that lines 1062-1063 demonstrate the proper way to obtain blockchain timestamps, which could be applied to the time constants initialization as suggested earlier.
232-270: Validation delegation pattern is correctly implemented and documented.The wrapper properly delegates validation to the underlying escrow contract. The wrapper validates only critical addresses (payer, merchant, operator, token) but intentionally skips validation of amounts, expiry times, and time ordering—delegating these checks to the escrow contract's
authorizefunction. The test suite accurately documents this behavior. This is a valid thin-wrapper design pattern.
614-651: Verify integration test coverage for refund execution.The claim that refund functionality "is tested in integration tests with real contracts" (lines 648-650) is unsubstantiated. Inspection found:
- Smart-contracts: access control only (1 test)
- Payment-processor: function encoding validation only
- Integration tests: refund addresses configured as parameters, but no actual refund execution tests
Confirm that integration tests actually execute
refundPayment()and verify:
- Tokens transferred to payer
- Payment state updated correctly
- Only operator can initiate refund
If integration tests don't cover this, add executable refund tests to validate the full flow.
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
bassgeta
left a comment
bassgeta
left a comment
There was a problem hiding this comment.
Looking good, nice work on this and the exhaustive test coverage 🔥
The TS wrapper looks nice and clean, and the contract itself has a lot of comments that explain what's going on.
🚢
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
Outdated
Show resolved
Hide resolved
packages/payment-processor/src/payment/erc20-commerce-escrow-wrapper.ts
Outdated
Show resolved
Hide resolved
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
Partial review - ERC20CommerceEscrowWrapper.sol only
I've reviewed ERC20CommerceEscrowWrapper.sol and posted comments on:
- Missing
feeBpsupper bound validation (high priority) PaymentDatastruct packing opportunity (45% storage savings)InvalidPayererror for better debugging- State delegation to Commerce Escrow (architectural note)
Haven't reviewed the other 16 files yet. The contract looks solid overall - main concerns are the fee validation and storage optimization.
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
Outdated
Show resolved
Hide resolved
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
Outdated
Show resolved
Hide resolved
packages/payment-processor/src/payment/erc20-commerce-escrow-wrapper.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Partial review - Test file and deployment script
I've reviewed the test file and deployment configuration, posting comments on:
Test file (ERC20CommerceEscrowWrapper.test.ts):
- Missing token balance verification across all test cases (critical priority)
- Fee calculation tests need balance checks, not just event verification
- Zero refund test coverage (medium priority - document if deferred)
Deployment script (constructor-args.ts):
- Hardcoded Commerce Escrow address needs network-specific artifacts pattern
Haven't reviewed the other 14 files yet. Test coverage is comprehensive for state/events but needs token movement verification.
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
Code Review Summary
Completed comprehensive review of all 18 files in this PR. The Commerce Escrow payment functionality is well-implemented with proper integration into the Request Network ecosystem.
Highlights:
✅ Smart contract follows established patterns and includes proper validation
✅ Type definitions are comprehensive and match contract structures
✅ Deployment scripts properly configured for CREATE2 deployment
✅ Test coverage is excellent for encoding and integration flows
✅ Payment processor integration follows codebase conventions
Key Issue Identified:
- Interface usage inconsistency in
erc20-commerce-escrow-wrapper.ts(see inline comment)
The implementation is production-ready after addressing the inline comment regarding Interface usage documentation.
…testing scripts - Introduced a new deployment script for ERC20CommerceEscrowWrapper, utilizing official Base contracts. - Added a test script for Base Sepolia deployment, demonstrating wallet creation and deployment process. - Implemented a malicious token contract for testing reentrancy protection in ERC20CommerceEscrowWrapper. - Enhanced unit tests to validate reentrancy protection across various payment functions, ensuring robustness against attacks.
…oving USDT handling - Removed the special handling for USDT in `encodeSetCommerceEscrowAllowance` and `encodeSetRecurringAllowance` functions, streamlining the approval process to a single transaction for all ERC20 tokens. - Updated related tests to reflect the changes, ensuring they now validate the single transaction behavior for token approvals.
…ency - Reorganized the PaymentData struct to reduce storage slots from 11 to 6, achieving approximately 45% gas savings. - Updated data types for amount and expiry fields to smaller types (uint96, uint48) to enhance storage efficiency. - Adjusted related functions to ensure proper validation and casting of payment parameters, maintaining functionality while improving performance.
…lag and error handling - Added an `isActive` boolean to the `PaymentData` struct for efficient existence checks and improved state management. - Introduced a new `InvalidPayer` error for clearer error handling when the caller is not the designated payer. - Updated related logic to utilize the new error for better clarity in payment validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol (4)
65-267: Driver functions now meaningfully exercise the wrapper stateThe constructor plus
driver_*functions are wired in a sensible way: they mint bounded amounts, set the right approvals, call the wrapper entry points, and update aggregate counters only on successful calls. This should give Echidna decent coverage over authorize / capture / void / charge / reclaim / refund flows without blowing up gas usage.If you want to cut some noise later, you could optionally early‑return on
captureAmount == 0indriver_capturePaymentto avoid repeatedly callingcapturePaymentwith zero amounts, but it’s not a correctness issue.
402-416: Consider renaming localsupplyinechidna_token_conservationto avoid shadowingInside
echidna_token_conservationyou declare a localuint256 supply = token.totalSupply();, which shadows the state variablesupply. It’s logically correct, but the name clash makes it harder to see at a glance whether the invariant is using the tracked supply or the live token supply.Renaming the local (e.g., to
currentSupply) would clarify intent without changing behavior:- function echidna_token_conservation() public returns (bool) { - uint256 supply = token.totalSupply(); + function echidna_token_conservation() public returns (bool) { + uint256 currentSupply = token.totalSupply(); @@ - // Supply should equal accounted tokens (within small margin for rounding) - return supply == accountedFor; + // Supply should equal accounted tokens + return currentSupply == accountedFor; }
378-399: Tighten or drop invariants that are currently vacuousA few invariants are effectively always
trueand won’t catch regressions:
echidna_merchant_receives_nonnegative(Lines 386–391):merchantBalance < type(uint256).maxwill hold for any valid ERC‑20 state; it doesn’t encode a real safety property.echidna_payment_ref_counter_monotonic(Lines 431–437):paymentRefCounter >= 0is trivially true for auint256, so it can never fail.echidna_fee_bps_validation_enforced(Lines 490–497): unconditionally returnstrue, so it doesn’t assert anything about the implementation.To keep the fuzzing report signal‑to‑noise high, I’d either:
- Strengthen these (e.g., by comparing against stored snapshots or real wrapper/escrow state), or
- Remove them and rely on the more meaningful invariants you already have.
Also applies to: 386-391, 431-437, 490-497
46-52: Unused accounting trackers could be wired into invariants or removedThe counters
totalVoided,totalReclaimed, andtotalRefundedare updated in the driver functions but aren’t referenced by any invariant yet. That’s not wrong, but it adds some conceptual overhead.Either wiring them into additional invariants (e.g., bounding reclaimed/refunded amounts against authorized/captured) or removing them until needed would keep the harness a bit leaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/security-echidna.yml(1 hunks)packages/smart-contracts/.slither.config.json(1 hunks)packages/smart-contracts/echidna.config.yml(1 hunks)packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/security-echidna.yml
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-17T16:00:11.730Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:30-34
Timestamp: 2024-10-17T16:00:11.730Z
Learning: In `packages/smart-contracts/src/contracts/test/UsdtFake.sol`, the code is part of the test harness and not intended for production, so deviations from ERC20 standards are acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-08T18:24:06.144Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:237-246
Timestamp: 2024-11-08T18:24:06.144Z
Learning: In `packages/payment-processor/test/payment/single-request-proxy.test.ts`, when asserting the `feeProxyUsed` in emitted events from `SingleRequestProxyFactory`, retrieve the `erc20FeeProxy` (or `ethereumFeeProxy`) public variable from the `SingleRequestProxyFactory` contract instead of using `wallet.address`.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-12T17:48:47.072Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:211-219
Timestamp: 2024-11-12T17:48:47.072Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, the error handling logic in `payWithEthereumSingleRequestForwarder` is correct and does not require changes to differentiate between ERC20 and Ethereum SingleRequestForwarder contracts.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-12-09T18:59:04.613Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts:80-99
Timestamp: 2024-12-09T18:59:04.613Z
Learning: In the `RequestNetwork` codebase, payment processor functions such as `payErc20ProxyRequestFromHinkalShieldedAddress` in `packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts` should not include explicit request validation or try/catch blocks, and should rely on the underlying components (like `hinkalObject`) to handle error reporting.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-17T16:00:27.352Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:36-43
Timestamp: 2024-10-17T16:00:27.352Z
Learning: The `UsdtFake.sol` contract is a test harness, not intended for production, so standard compliance issues can be ignored in this file.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-08T18:24:19.095Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:198-206
Timestamp: 2024-11-08T18:24:19.095Z
Learning: In tests for `SingleRequestProxyFactory`, the `feeProxyUsed` in events should be verified by retrieving the `ethereumFeeProxy` public variable from the `SingleRequestProxyFactory` contract, not `wallet.address`.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-05T14:43:14.816Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol:11-15
Timestamp: 2024-10-05T14:43:14.816Z
Learning: In the `EthereumSingleRequestProxy` contract, the state variables `payee`, `paymentReference`, `feeAddress`, `feeAmount`, and `ethereumFeeProxy` should remain public, as they need to be accessed externally.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-12T16:54:02.702Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:104-104
Timestamp: 2024-11-12T16:54:02.702Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, when the smart contract has not changed, event argument names such as `proxyAddress` remain the same, even if variable names in the code are updated to use new terminology like `forwarderAddress`.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-29T09:00:54.169Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/src/payment/single-request-proxy.ts:202-209
Timestamp: 2024-10-29T09:00:54.169Z
Learning: In the `packages/payment-processor/src/payment/single-request-proxy.ts` file, within the `payWithEthereumSingleRequestProxy` function, the current error handling is acceptable as per the project's expectations.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-06T14:48:18.698Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1484
File: packages/advanced-logic/test/extensions/payment-network/any-to-near.test.ts:0-0
Timestamp: 2024-11-06T14:48:18.698Z
Learning: In `packages/advanced-logic/test/extensions/payment-network/any-to-near.test.ts`, when the existing happy path tests are deemed sufficient, additional test cases may not be necessary.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-05T14:43:16.298Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol:46-46
Timestamp: 2024-10-05T14:43:16.298Z
Learning: In `packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol`, within the `receive()` function, it's acceptable to approve the entire `balance` to `erc20FeeProxy` because `balance == paymentAmount + feeAmount`.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-04T14:32:57.040Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc20-fee-proxy.ts:186-186
Timestamp: 2024-11-04T14:32:57.040Z
Learning: In the `preparePrivateErc20FeeProxyPaymentTransaction` function in `packages/payment-processor/src/payment/erc20-fee-proxy.ts`, we should not make an allowance call on the token contract, as it is not relevant to our use case.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2025-06-23T09:35:04.263Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1633
File: packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol:92-133
Timestamp: 2025-06-23T09:35:04.263Z
Learning: In the ERC20RecurringPaymentProxy contract, events were intentionally omitted to save gas costs since the underlying ERC20FeeProxy contract already emits events when payments are processed. This avoids duplicate event emissions while maintaining observability.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-12-18T03:53:54.370Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-17T18:33:41.124Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts:0-0
Timestamp: 2024-10-17T18:33:41.124Z
Learning: In the `EthereumSingleRequestProxy` contract, the custom reentrancy guard is nearly identical to OpenZeppelin's ReentrancyGuard, except that it allows the first reentrancy call from the `ERC20FeeProxy`. We assume it works correctly without needing further tests.
Applied to files:
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Slither Static Analysis
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: build-and-test
🔇 Additional comments (4)
packages/smart-contracts/.slither.config.json (2)
1-14: LGTM! Slither configuration is comprehensive and well-configured.The configuration appropriately enables all severity levels for complete coverage, excludes test artifacts and dependencies, maps OpenZeppelin imports, and aligns compiler optimization settings with production deployments. The detector exclusions for
naming-conventionandsimilar-namesare reasonable for reducing noise on non-critical style violations.
8-8: solc_remaps path is correctly configured for the monorepo structure.The path
../../node_modules/@openzeppelinis correct for a Yarn workspace monorepo. Frompackages/smart-contracts, going up two directories (../../) reaches the repository root, where root-levelnode_modules/@openzeppelinis installed via the Yarn workspace configuration. The smart-contracts package.json confirms@openzeppelin/contractsis listed as a devDependency, so the path will resolve correctly when dependencies are installed.packages/smart-contracts/echidna.config.yml (1)
1-53: Echidna config looks reasonable for CI and local runsThe configuration is consistent and conservative (limits, timeouts, coverage, corpus, and sender setup) and should work fine as a shared base for ERC20CommerceEscrowWrapper fuzzing. No blocking issues from this file alone.
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol (1)
504-552: Mocks are appropriate for a test harness
MockERC20,MockAuthCaptureEscrow, andMockERC20FeeProxyare intentionally minimal and deviate from full production‑grade behavior (no access control onmint, no zero‑address checks, simplified state), but that’s acceptable here since they’re only used as part of the fuzzing harness to approximate flows.No blocking issues from these mocks as long as they remain scoped to testing, which is consistent with the rest of the codebase’s approach to test harness contracts.
Also applies to: 556-690, 692-729
…eppelin - Enhanced the Echidna Docker wrapper script to dynamically find the monorepo root, ensuring accessibility to node_modules. - Updated the remapping configuration for OpenZeppelin to use relative paths, improving compatibility for local execution. - Improved comments for clarity on the purpose of changes and the functionality of the testing setup.
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/smart-contracts/scripts/run-echidna.sh (2)
118-119: Remove unused variable or export for external use.
CONTRACTS_DIRis assigned on line 118 but never used in the script. Verify whether this was intended for external consumption (in which case it should be exported) or should be removed.- CONTRACTS_DIR=$(pwd) MONOREPO_ROOT=$(cd ../.. && pwd)
59-60: Document the Solidity compiler version requirement.The script hardcodes solc version 0.8.9 (lines 46, 48, 59). Verify that this version is compatible with the
EchidnaERC20CommerceEscrowWrappercontract being tested. Consider adding a comment explaining why this specific version is required..github/workflows/security-echidna.yml (2)
65-81: Document the Docker wrapper assumptions.The Docker wrapper script (lines 65-81) searches for
lerna.jsonto identify the monorepo root, then calculates the relative path to mount volumes. This is reasonable for the RequestNetwork monorepo structure but could fail if:
- The repo layout changes and
lerna.jsonmoves or is removed- The working directory at execution time is outside the repo
Consider adding a comment explaining the layout assumptions, or add defensive validation (e.g., verify the mounted path is accessible inside the container).
185-255: Improve type safety in PR comment script.Line 217 uses
if (failed > 0)wherefailedis a string. While JavaScript's type coercion makes this work (e.g.,"1" > 0evaluates to true), it's clearer to be explicit. The same applies to line 199'spassRatecalculation.- if (failed > 0) { + if (parseInt(failed, 10) > 0) { - const passRate = total > 0 ? ((passed / total) * 100).toFixed(1) : '0'; + const passRate = parseInt(total, 10) > 0 ? ((parseInt(passed, 10) / parseInt(total, 10)) * 100).toFixed(1) : '0';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/security-echidna.yml(1 hunks)packages/smart-contracts/scripts/run-echidna.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
🪛 Shellcheck (0.11.0)
packages/smart-contracts/scripts/run-echidna.sh
[warning] 118-118: CONTRACTS_DIR appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: Slither Static Analysis
- GitHub Check: build-and-test
🔇 Additional comments (2)
.github/workflows/security-echidna.yml (2)
157-172: Verify grep patterns match actual Echidna output format.The result parsing logic (lines 157-158) relies on grep patterns
"echidna.*: passed"and"echidna.*: failed"to count test results. The subsequent defensive line-extraction logic (lines 161-162) suggests these patterns may return multiple matches or unexpected formatting.Before merging, confirm:
- What is the exact Echidna output format for passed/failed properties?
- Do these grep patterns reliably capture only the property counts?
- Why is last-line extraction needed—could it indicate the patterns are too broad?
139-139: Verify Echidna remapping path resolves correctly in Docker.Line 139 uses a relative remapping path
../../node_modules/@openzeppelin/assuming execution frompackages/smart-contracts(confirmed by line 122'sworking-directory). Verify that:
- The Docker volume mount (line 77) makes this relative path accessible inside the container
- Echidna successfully resolves
@openzeppelin/imports with this remappingIf there are issues at runtime, consider using an absolute path within the container or improving path resolution documentation.
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
- Modified the grep command in the security workflow to reflect the change in Echidna's output from "passed" to "passing", ensuring accurate counting of test results. - Improved comments for clarity regarding the output format of the Echidna report.
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
- Introduced Base Sepolia network definitions across multiple modules, including EVM chains, ERC20 token support, and payment detection. - Updated contract artifacts to include deployment addresses for Base Sepolia, ensuring compatibility with the new network. - Enhanced type definitions to recognize Base Sepolia as a valid EVM chain name.
…ent addresses - Eliminated placeholder addresses for Sepolia, Goerli, and Mumbai testnets from the ERC20CommerceEscrowWrapper artifact, streamlining the deployment configuration. - Retained the Base Sepolia address while marking other networks for future updates, ensuring clarity in the deployment process.
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/smart-contracts/scripts/update-base-sepolia-addresses.js (1)
58-61: Consider validating Ethereum address format.The script updates addresses without validating that they're valid Ethereum addresses (0x followed by 40 hex characters). While this is an admin script, adding basic validation would catch typos early.
You could add a simple validation helper:
const isValidAddress = (addr) => /^0x[0-9a-fA-F]{40}$/.test(addr); // After line 27, add: if (!isValidAddress(erc20FeeProxyAddress) || !isValidAddress(escrowWrapperAddress)) { console.error('❌ Error: Invalid Ethereum address format\n'); console.log('Addresses must be in format: 0x followed by 40 hexadecimal characters\n'); process.exit(1); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
BASE_SEPOLIA_README.md(1 hunks)packages/currency/src/chains/evm/data/base-sepolia.ts(1 hunks)packages/currency/src/chains/evm/index.ts(2 hunks)packages/currency/src/erc20/chains/base-sepolia.ts(1 hunks)packages/currency/src/erc20/chains/index.ts(2 hunks)packages/payment-detection/src/eth/multichainExplorerApiProvider.ts(1 hunks)packages/smart-contracts/scripts/deploy-base-sepolia.sh(1 hunks)packages/smart-contracts/scripts/update-base-sepolia-addresses.js(1 hunks)packages/smart-contracts/src/lib/artifacts/AuthCaptureEscrow/index.ts(1 hunks)packages/smart-contracts/src/lib/artifacts/ERC20CommerceEscrowWrapper/index.ts(1 hunks)packages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts(1 hunks)packages/types/src/currency-types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/smart-contracts/src/lib/artifacts/ERC20CommerceEscrowWrapper/index.ts
- packages/smart-contracts/src/lib/artifacts/AuthCaptureEscrow/index.ts
🧰 Additional context used
🧠 Learnings (23)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
📚 Learning: 2024-11-12T16:54:02.702Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:104-104
Timestamp: 2024-11-12T16:54:02.702Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, when the smart contract has not changed, event argument names such as `proxyAddress` remain the same, even if variable names in the code are updated to use new terminology like `forwarderAddress`.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts
📚 Learning: 2024-11-12T16:52:41.557Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/smart-contracts/src/lib/artifacts/SingleRequestProxyFactory/index.ts:5-5
Timestamp: 2024-11-12T16:52:41.557Z
Learning: When the smart contracts are not being modified, types like `SingleRequestProxyFactory` in `packages/smart-contracts/src/lib/artifacts/SingleRequestProxyFactory/index.ts` should remain unchanged, even if terminology elsewhere in the code is updated.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts
📚 Learning: 2024-11-11T16:10:26.692Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1486
File: packages/smart-contracts/deploy/deploy-zk-single-request-proxy.ts:1-24
Timestamp: 2024-11-11T16:10:26.692Z
Learning: The team prefers to maintain individual deployment scripts for each network in `packages/smart-contracts/deploy`, rather than consolidating them into a unified script.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/scripts/deploy-base-sepolia.sh
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/currency/src/erc20/chains/base-sepolia.tspackages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts
📚 Learning: 2024-10-28T20:00:33.707Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/smart-contracts/scripts/test-deploy-single-request-proxy.ts:4-7
Timestamp: 2024-10-28T20:00:33.707Z
Learning: In TypeScript test files (e.g., `test-deploy-single-request-proxy.ts`), it's acceptable to use `string` types for Ethereum addresses in interfaces (like `FeeProxyAddresses`), as stricter type safety is not necessary.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.tspackages/currency/src/chains/evm/data/base-sepolia.ts
📚 Learning: 2024-11-05T05:04:01.710Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1478
File: packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts:45-58
Timestamp: 2024-11-05T05:04:01.710Z
Learning: When executing blockchain transactions in scripts (e.g., in `packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts`), ensure that transactions are executed serially rather than in parallel to maintain correct execution order and prevent potential issues.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.js
📚 Learning: 2024-11-05T05:33:36.551Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1478
File: packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts:30-36
Timestamp: 2024-11-05T05:33:36.551Z
Learning: In the RequestNetwork project, admin scripts like `setupSingleRequestProxyFactory.ts` in `packages/smart-contracts/scripts-create2/contract-setup/` do not require extensive error checking.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/scripts/deploy-base-sepolia.sh
📚 Learning: 2024-11-05T05:33:32.481Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1478
File: packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts:38-43
Timestamp: 2024-11-05T05:33:32.481Z
Learning: In the RequestNetwork codebase, setup scripts such as `setupSingleRequestProxyFactory.ts` do not include contract existence checks before interacting with contracts, even though scripts like `check-deployer.ts` do include such checks.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/scripts/deploy-base-sepolia.sh
📚 Learning: 2024-10-28T20:00:25.780Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/smart-contracts/scripts/test-deploy-single-request-proxy.ts:14-32
Timestamp: 2024-10-28T20:00:25.780Z
Learning: In test files, such as `packages/smart-contracts/scripts/test-deploy-single-request-proxy.ts`, extensive error handling and input validation are considered unnecessary.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/scripts/deploy-base-sepolia.sh
📚 Learning: 2024-11-08T18:24:06.144Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:237-246
Timestamp: 2024-11-08T18:24:06.144Z
Learning: In `packages/payment-processor/test/payment/single-request-proxy.test.ts`, when asserting the `feeProxyUsed` in emitted events from `SingleRequestProxyFactory`, retrieve the `erc20FeeProxy` (or `ethereumFeeProxy`) public variable from the `SingleRequestProxyFactory` contract instead of using `wallet.address`.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts
📚 Learning: 2024-10-30T17:54:34.513Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1478
File: packages/smart-contracts/scripts-create2/contract-setup/update-fee-proxies.ts:29-31
Timestamp: 2024-10-30T17:54:34.513Z
Learning: When logging missing contract deployments in `updateFeeProxies`, prefer using `console.warn` over `console.info` to highlight potential issues.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts
📚 Learning: 2024-10-28T20:02:48.835Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/smart-contracts/scripts/test-deploy-single-request-proxy.ts:29-29
Timestamp: 2024-10-28T20:02:48.835Z
Learning: In test deployment scripts (e.g., files like 'test-deploy-*.ts' in 'packages/smart-contracts/scripts'), it's acceptable to use `console.log` for logging.
Applied to files:
packages/smart-contracts/scripts/update-base-sepolia-addresses.jspackages/smart-contracts/scripts/deploy-base-sepolia.sh
📚 Learning: 2025-06-23T09:32:16.214Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1633
File: packages/smart-contracts/scripts/test-deploy-erc20-recurring-payment-proxy.ts:16-16
Timestamp: 2025-06-23T09:32:16.214Z
Learning: Test deployment scripts in the RequestNetwork repository (files with `test-deploy-` prefix) use the same deployer address for multiple roles for simplicity in local testing environments. Production deployments use separate addresses configured through environment variables.
Applied to files:
packages/smart-contracts/scripts/deploy-base-sepolia.sh
📚 Learning: 2024-11-04T14:30:34.835Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/usage-examples/src/hinkal/hinkalRequestData.ts:5-6
Timestamp: 2024-11-04T14:30:34.835Z
Learning: In `packages/usage-examples/src/hinkal/hinkalRequestData.ts`, it's acceptable to hardcode the USDC contract address without creating a mapping of USDC assets on specific networks.
Applied to files:
packages/currency/src/erc20/chains/base-sepolia.ts
📚 Learning: 2024-12-04T05:01:13.722Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/usage-examples/src/hinkal/hinkalRequestData.ts:0-0
Timestamp: 2024-12-04T05:01:13.722Z
Learning: In `packages/usage-examples/src/hinkal/hinkalRequestData.ts`, when integrating Hinkal, it's acceptable for the network to be set to 'base' as the payment chain while using the Sepolia gateway URL ('https://sepolia.gateway.request.network') as the request storage chain.
Applied to files:
packages/currency/src/erc20/chains/index.tspackages/payment-detection/src/eth/multichainExplorerApiProvider.tspackages/currency/src/chains/evm/data/base-sepolia.tsBASE_SEPOLIA_README.md
📚 Learning: 2024-10-05T14:43:14.816Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol:11-15
Timestamp: 2024-10-05T14:43:14.816Z
Learning: In the `EthereumSingleRequestProxy` contract, the state variables `payee`, `paymentReference`, `feeAddress`, `feeAmount`, and `ethereumFeeProxy` should remain public, as they need to be accessed externally.
Applied to files:
packages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts
📚 Learning: 2024-11-08T18:24:19.095Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:198-206
Timestamp: 2024-11-08T18:24:19.095Z
Learning: In tests for `SingleRequestProxyFactory`, the `feeProxyUsed` in events should be verified by retrieving the `ethereumFeeProxy` public variable from the `SingleRequestProxyFactory` contract, not `wallet.address`.
Applied to files:
packages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts
📚 Learning: 2024-11-04T14:32:57.040Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc20-fee-proxy.ts:186-186
Timestamp: 2024-11-04T14:32:57.040Z
Learning: In the `preparePrivateErc20FeeProxyPaymentTransaction` function in `packages/payment-processor/src/payment/erc20-fee-proxy.ts`, we should not make an allowance call on the token contract, as it is not relevant to our use case.
Applied to files:
packages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts
📚 Learning: 2024-09-27T11:42:01.062Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/SingleRequestProxyFactory.sol:0-0
Timestamp: 2024-09-27T11:42:01.062Z
Learning: In the `createEthereumSingleRequestProxy` function, it's acceptable for `_feeAddress` to be the zero address, as setting the fee address to zero is a common practice. Adding zero address checks is unnecessary and adds extra gas cost.
Applied to files:
packages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts
📚 Learning: 2024-12-18T03:53:54.370Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
Applied to files:
packages/currency/src/chains/evm/data/base-sepolia.ts
📚 Learning: 2024-11-05T16:58:18.471Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/payment-detection/test/provider.test.ts:25-25
Timestamp: 2024-11-05T16:58:18.471Z
Learning: In `provider.test.ts`, when testing `getDefaultProvider`, we use a chain that Infura supports but is not in our own RPC list (such as `maticmum`) to ensure that the function correctly falls back to `InfuraProvider`.
Applied to files:
packages/currency/src/chains/evm/data/base-sepolia.ts
📚 Learning: 2024-12-13T10:00:17.504Z
Learnt from: rodrigopavezi
Repo: RequestNetwork/requestNetwork PR: 1512
File: packages/integration-test/test/lit-protocol.test.ts:48-50
Timestamp: 2024-12-13T10:00:17.504Z
Learning: In `packages/integration-test/test/lit-protocol.test.ts`, the wallet private key `'0x7b595b2bb732edddc4d4fe758ae528c7a748c40f0f6220f4494e214f15c5bfeb'` is fixed and can be hardcoded in the test file.
Applied to files:
packages/currency/src/chains/evm/data/base-sepolia.ts
🧬 Code graph analysis (3)
packages/currency/src/erc20/chains/base-sepolia.ts (1)
packages/types/src/currency-types.ts (1)
TokenMap(66-66)
packages/currency/src/erc20/chains/index.ts (1)
packages/currency/src/erc20/chains/base-sepolia.ts (1)
supportedBaseSepoliaERC20(4-12)
packages/currency/src/chains/evm/data/base-sepolia.ts (3)
packages/types/src/index.ts (1)
CurrencyTypes(25-25)packages/types/src/currency-types.ts (1)
TokenMap(66-66)packages/currency/src/erc20/chains/base-sepolia.ts (1)
supportedBaseSepoliaERC20(4-12)
🪛 Gitleaks (8.29.0)
packages/smart-contracts/scripts/deploy-base-sepolia.sh
[high] 65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
BASE_SEPOLIA_README.md
52-52: Bare URL used
(MD034, no-bare-urls)
53-53: Bare URL used
(MD034, no-bare-urls)
161-161: Bare URL used
(MD034, no-bare-urls)
193-193: Bare URL used
(MD034, no-bare-urls)
194-194: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
packages/smart-contracts/scripts/deploy-base-sepolia.sh
[warning] 24-24: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Slither Static Analysis
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: build-and-test
🔇 Additional comments (9)
packages/smart-contracts/src/lib/artifacts/ERC20FeeProxy/index.ts (1)
166-169: No issues identified; this is intentional placeholder deployment data for a new network.The base-sepolia entry follows the documented deployment workflow for adding new networks to RequestNetwork. The code references an
update-base-sepolia-addresses.jsscript designed specifically to update these placeholder values after the actual contract deployment on base-sepolia occurs. SettingcreationBlockNumber: 0for a newly added network is standard practice—while it causes event log scanning to start from genesis (inefficient), it functions correctly and will be updated to the actual deployment block once deployment completes.packages/types/src/currency-types.ts (1)
12-12: LGTM!The Base Sepolia chain name is correctly added to the
EvmChainNameunion type, following the existing pattern.BASE_SEPOLIA_README.md (1)
1-198: LGTM!The documentation is comprehensive and well-structured, covering deployment steps, network details, SDK usage examples, troubleshooting, and helpful resources.
Note: The markdown linter warnings about bare URLs (MD034) in tables and reference sections can be safely ignored—they improve readability in documentation.
packages/currency/src/erc20/chains/index.ts (1)
16-16: LGTM!The Base Sepolia ERC20 token mapping is correctly imported and added to
supportedNetworks, following the existing pattern for other chains.Also applies to: 35-35
packages/currency/src/chains/evm/index.ts (1)
31-31: LGTM!The Base Sepolia chain definition is correctly imported and mapped in the
chainsrecord, consistent with the existing pattern.Also applies to: 67-67
packages/smart-contracts/scripts/deploy-base-sepolia.sh (1)
1-104: LGTM! The deployment script is well-structured.The script provides clear user guidance, validates prerequisites, and includes helpful post-deployment instructions. The Gitleaks warning about line 65 is a false positive—it's flagging the AuthCaptureEscrow contract address as a potential API key.
packages/currency/src/erc20/chains/base-sepolia.ts (1)
1-12: USDC address verified as correct.The USDC contract address
0x036CbD53842c5426634e7929541eC2318f3dCF7ematches the official Base Sepolia deployment. No changes needed.packages/currency/src/chains/evm/data/base-sepolia.ts (2)
1-2: LGTM!The imports are clean and necessary. Both
CurrencyTypesandsupportedBaseSepoliaERC20are used in the exported constants below.
4-8: Base Sepolia chain ID is correct.The chain ID 84532 is the correct identifier for Base Sepolia testnet. The configuration is accurate and structurally sound.
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
- Introduced a new case for the Base Sepolia network in the MultichainExplorerApiProvider, providing the appropriate API URL for integration. - Updated the deploy script to source environment variables more robustly, enhancing the loading process for configuration files.
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
Looks good @rodrigopavezi. 👍 Thank you for being so patient. 🙇
- Deleted the security README.md file, which contained information on security testing tools and reports for smart contracts, as it is no longer relevant to the current project structure.
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
- Replaced the placeholder address for the Sepolia deployment with a new placeholder, indicating it will be updated with the actual deployment address in the future. - Updated the creation block numbers for the Base Mainnet and Base Sepolia deployments to reflect the correct values, ensuring accurate deployment configuration.
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Fix #1650