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.
WalkthroughAdds an ERC-20 Commerce Escrow Wrapper end-to-end: new Solidity contracts and mock, ABIs/artifacts, TypeScript types and payment-processor integration (encoders + tx helpers + tests), create2 deployment/tooling updates, package dependency, and docker platform entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Payer
participant Wrapper as ERC20CommerceEscrowWrapper
participant Escrow as CommerceEscrow (IAuthCaptureEscrow)
participant FeeProxy as ERC20FeeProxy
participant Token as ERC20
rect #E6F0FF
Note over Payer,Escrow: Authorization
Payer->>Token: approve/transferFrom -> Wrapper
Wrapper->>Escrow: authorize(PaymentInfo, amount, tokenCollector, collectorData)
Escrow-->>Wrapper: authorization recorded
Wrapper-->>Wrapper: store PaymentData
end
rect #E8FFE8
Note over Wrapper,FeeProxy: Capture (operator)
Wrapper->>Escrow: capture(PaymentInfo, captureAmount, feeBps, feeReceiver)
Escrow-->>Wrapper: capturable/refundable updated
Wrapper->>Token: approve(FeeProxy, merchantAmount+fee)
Wrapper->>FeeProxy: transferFromWithReferenceAndFee(token, merchant, merchantAmount, fee, feeReceiver, reference)
FeeProxy->>Token: distribute(merchant, feeReceiver)
end
rect #FFF2E6
Note over Wrapper,Escrow: Void / Reclaim / Refund flows
alt Void (operator)
Wrapper->>Escrow: void(PaymentInfo)
Escrow->>Token: return funds to payer
else Reclaim (payer)
Payer->>Wrapper: reclaimPayment(reference)
Wrapper->>Escrow: reclaim(PaymentInfo)
Escrow->>Token: transfer to payer
else Refund (operator)
Wrapper->>Token: transferFrom(operator, Wrapper, refundAmount)
Wrapper->>Escrow: refund(PaymentInfo, refundAmount, tokenCollector, collectorData)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
| export const erc20CommerceEscrowWrapperArtifact = new ContractArtifact<ERC20CommerceEscrowWrapper>( | ||
| { | ||
| '0.1.0': { | ||
| abi: ABI_0_1_0, | ||
| deployment: { | ||
| private: { | ||
| address: '0x0000000000000000000000000000000000000000', // Placeholder - to be updated with actual deployment | ||
| creationBlockNumber: 0, | ||
| }, | ||
| // Testnet deployments for testing | ||
| sepolia: { | ||
| address: '0x1234567890123456789012345678901234567890', // Placeholder - to be updated with actual deployment | ||
| creationBlockNumber: 0, | ||
| }, | ||
| goerli: { | ||
| address: '0x1234567890123456789012345678901234567890', // Placeholder - to be updated with actual deployment | ||
| creationBlockNumber: 0, | ||
| }, | ||
| mumbai: { | ||
| address: '0x1234567890123456789012345678901234567890', // Placeholder - to be updated with actual deployment | ||
| creationBlockNumber: 0, | ||
| }, | ||
| // TODO: Add deployment addresses for mainnet networks once deployed | ||
| // mainnet: { | ||
| // address: '0x0000000000000000000000000000000000000000', | ||
| // creationBlockNumber: 0, | ||
| // }, | ||
| // matic: { | ||
| // address: '0x0000000000000000000000000000000000000000', | ||
| // creationBlockNumber: 0, | ||
| // }, | ||
| }, | ||
| }, | ||
| }, | ||
| '0.1.0', | ||
| ); |
There was a problem hiding this comment.
All deployment addresses are placeholders and must be updated.
The artifact contains placeholder addresses for all networks:
- Private network: zero address (0x000...000)
- Testnets (Sepolia, Goerli, Mumbai): invalid placeholder (0x123...890)
- Mainnet networks: commented out
Additionally, all creationBlockNumber values are set to 0, which should be updated to the actual deployment block numbers for event indexing and proper initialization.
Before moving out of draft:
- Deploy the contract to each target network
- Update all addresses with actual deployment addresses
- Set correct creation block numbers
- Uncomment and populate mainnet deployment sections if targeting production networks
🤖 Prompt for AI Agents
In
packages/smart-contracts/src/lib/artifacts/ERC20CommerceEscrowWrapper/index.ts
around lines 7 to 42, the artifact uses placeholder addresses and zero
creationBlockNumber values for all networks; replace each placeholder address
with the actual deployed contract address for the target network and set
creationBlockNumber to the block number where that contract was deployed (for
private, sepolia, goerli, mumbai and any mainnet networks you intend to
support), and if deploying to mainnet uncomment and populate the mainnet/matic
sections accordingly so event indexing and initialization use real deployment
metadata.
… 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.
| describe('Reentrancy Protection', () => { | ||
| it('should prevent reentrancy on authorizePayment', async () => { | ||
| // This would require a malicious token contract to test properly | ||
| // For now, we verify the nonReentrant modifier is present | ||
| const authParams = { | ||
| paymentReference: getUniquePaymentReference(), | ||
| payer: payerAddress, | ||
| merchant: merchantAddress, | ||
| operator: operatorAddress, | ||
| token: testERC20.address, | ||
| amount, | ||
| maxAmount, | ||
| preApprovalExpiry, | ||
| authorizationExpiry, | ||
| refundExpiry, | ||
| tokenCollector: tokenCollectorAddress, | ||
| collectorData: '0x', | ||
| }; | ||
|
|
||
| await expect(wrapper.authorizePayment(authParams)).to.emit(wrapper, 'PaymentAuthorized'); | ||
| }); | ||
|
|
||
| it('should prevent reentrancy on capturePayment', async () => { | ||
| const authParams = { | ||
| paymentReference: getUniquePaymentReference(), | ||
| payer: payerAddress, | ||
| merchant: merchantAddress, | ||
| operator: operatorAddress, | ||
| token: testERC20.address, | ||
| amount, | ||
| maxAmount, | ||
| preApprovalExpiry, | ||
| authorizationExpiry, | ||
| refundExpiry, | ||
| tokenCollector: tokenCollectorAddress, | ||
| collectorData: '0x', | ||
| }; | ||
|
|
||
| await wrapper.authorizePayment(authParams); | ||
|
|
||
| await expect( | ||
| wrapper | ||
| .connect(operator) | ||
| .capturePayment(authParams.paymentReference, amount.div(2), feeBps, feeReceiverAddress), | ||
| ).to.emit(wrapper, 'PaymentCaptured'); | ||
| }); | ||
|
|
||
| it('should prevent reentrancy on chargePayment', async () => { | ||
| const chargeParams = { | ||
| paymentReference: getUniquePaymentReference(), | ||
| payer: payerAddress, | ||
| merchant: merchantAddress, | ||
| operator: operatorAddress, | ||
| token: testERC20.address, | ||
| amount, | ||
| maxAmount, | ||
| preApprovalExpiry, | ||
| authorizationExpiry, | ||
| refundExpiry, | ||
| feeBps, | ||
| feeReceiver: feeReceiverAddress, | ||
| tokenCollector: tokenCollectorAddress, | ||
| collectorData: '0x', | ||
| }; | ||
|
|
||
| await expect(wrapper.chargePayment(chargeParams)).to.emit(wrapper, 'PaymentCharged'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Reentrancy tests don't actually test reentrancy protection.
These tests are named to suggest they verify reentrancy protection, but they only verify that normal operations succeed. The comments acknowledge this limitation. This creates false confidence in security testing.
Recommendation: Either implement proper reentrancy tests using a malicious contract that attempts to reenter, or remove these tests and add a comment explaining that reentrancy protection is verified through other means (code review, audit, etc.).
Example of a proper reentrancy test structure:
// Deploy a malicious reentrant contract
const maliciousContract = await new MaliciousReentrant__factory(owner).deploy(
wrapper.address
);
it('should prevent reentrancy on capturePayment', async () => {
// Set up authorization with malicious token
const authParams = {
// ... params with maliciousContract.address as token
};
await wrapper.authorizePayment(authParams);
// Attempt reentrant capture should revert
await expect(
maliciousContract.attackCapture(authParams.paymentReference)
).to.be.revertedWith('ReentrancyGuard: reentrant call');
});🤖 Prompt for AI Agents
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts lines
816-883: the "Reentrancy Protection" tests only assert normal success and do not
exercise reentrancy, so either replace them with true reentrancy tests or
remove/clarify them; to fix, deploy a malicious reentrant contract in the test
suite that accepts ERC20-like calls and attempts to reenter (e.g.,
attackCapture/attackCharge functions), use that contract address as the token in
auth/charge setup, authorize the payment, then call the malicious contract's
attack function and assert it reverts with the ReentrancyGuard message (e.g.,
"ReentrancyGuard: reentrant call"); alternatively, if you remove the tests,
replace them with a short comment stating reentrancy is covered by code-level
ReentrancyGuard and audits and rename/remove the misleading test cases
accordingly.
1 participant
Summary by CodeRabbit
New Features
Tests
Infrastructure