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.
There was a problem hiding this comment.
Confirming this finding.
The "Reentrancy Protection" test suite (lines 816-883) validates that operations succeed under the nonReentrant modifier, but doesn't actually test reentrancy attack scenarios.
What's missing:
A malicious ERC20 contract that attempts reentrant calls during token transfer callbacks. For example:
contract MaliciousReentrantToken is ERC20 {
ERC20CommerceEscrowWrapper public target;
function transfer(address to, uint256 amount) public override returns (bool) {
// Attempt reentrancy during transfer
if (to == address(target)) {
target.capturePayment(...); // Should revert with ReentrancyGuard
}
return super.transfer(to, amount);
}
}Recommendation: Either implement proper reentrancy tests with malicious contracts, or remove these tests and document that reentrancy protection is verified through code review and the nonReentrant modifier from OpenZeppelin's battle-tested ReentrancyGuard.
There was a problem hiding this comment.
@MantisClone, thank you for confirming this finding and providing the detailed explanation with the example!
I can help implement either approach:
Option 1: Implement Proper Reentrancy Tests
I can provide a complete solution including:
-
Malicious Reentrant Token Contract (Solidity):
- A token that attempts to reenter
capturePayment,chargePayment, etc. during transfer callbacks - Should be placed in
packages/smart-contracts/src/contracts/test/MaliciousReentrantToken.sol
- A token that attempts to reenter
-
Updated Test Suite:
- Deploy the malicious token in the test setup
- Create test cases that attempt reentrancy and verify they revert with "ReentrancyGuard: reentrant call"
- Cover all protected functions:
authorizePayment,capturePayment,chargePayment,voidPayment,reclaimPayment,refundPayment
Option 2: Remove Tests and Document
Remove the "Reentrancy Protection" describe block (lines 816-883) and replace with a brief comment explaining:
// Note: Reentrancy protection is provided by OpenZeppelin's ReentrancyGuard modifier
// on all state-changing functions. This protection is verified through code review
// and has been battle-tested in production across thousands of contracts.Which approach would you prefer? If Option 1, I can generate the complete malicious contract and test cases for you to review.
🧠 Learnings used
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: 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.
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: 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`.
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:138-139
Timestamp: 2024-10-29T08:02:02.600Z
Learning: When testing invalid requests in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to use `ts-expect-error` to suppress TypeScript errors when the request intentionally lacks required properties.
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.
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: 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.
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.
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.
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.
Learnt from: rodrigopavezi
Repo: RequestNetwork/requestNetwork PR: 1475
File: packages/transaction-manager/test/unit/utils/test-data.ts:87-119
Timestamp: 2024-11-21T09:06:12.938Z
Learning: In `packages/transaction-manager/test/unit/utils/test-data.ts`, mocks like `fakeEpkCipherProvider` do not require extensive test coverage for input validation and error handling.
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.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol:19-23
Timestamp: 2024-10-05T14:43:07.965Z
Learning: In the `EthereumSingleRequestProxy` contract, a custom reentrancy guard is used instead of OpenZeppelin's `ReentrancyGuard`, with minor edits to allow reentrancy by the `ethereumFeeProxy`.
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/src/payment/single-request-proxy.ts:123-184
Timestamp: 2024-10-28T21:11:48.524Z
Learning: When suggesting error handling improvements in `packages/payment-processor/src/payment/single-request-proxy.ts`, avoid recommending try-catch blocks that re-throw errors without additional processing.
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.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:0-0
Timestamp: 2024-12-04T05:05:19.610Z
Learning: The function `createRequestForHinkal` in `erc-20-private-payment-hinkal.test.ts` is intended for testing purposes only and should remain in the test directory.
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.
🚢
| uint256 merchantAmount = captureAmount - feeAmount; | ||
|
|
||
| // Approve ERC20FeeProxy to spend the full amount we received | ||
| IERC20(payment.token).forceApprove(address(erc20FeeProxy), captureAmount); |
There was a problem hiding this comment.
Why do we need to call forceApprove here wouldn't calling an ordinary approve be sufficient?
There was a problem hiding this comment.
forceApprove is required for USDT compatibility.
USDT requires allowances to be reset to 0 before setting a new non-zero value. Regular approve() will revert if there's any existing allowance.
forceApprove (from OpenZeppelin's SafeERC20) handles this automatically:
- Tries
approve(spender, value) - If that fails, does
approve(spender, 0)thenapprove(spender, value)
This is the recommended OpenZeppelin pattern for supporting USDT and other non-standard tokens.
| const transactions: Array<{ to: string; data: string; value: number }> = []; | ||
|
|
||
| if (isUSDT) { | ||
| const resetData = paymentTokenContract.interface.encodeFunctionData('approve', [ |
There was a problem hiding this comment.
Do we do it for all of the chains or just specific onces, because Ethereum fully blocks but not other chains.
There was a problem hiding this comment.
You're right - the USDT approval quirk is Ethereum mainnet only. Other chains don't have this restriction.
Recommendation: Drop the manual approve(0) reset from the payment processor.
The smart contract already uses forceApprove from OpenZeppelin's SafeERC20, which handles this intelligently:
- Tries
approve(spender, amount)first - Only if that fails, does
approve(0)thenapprove(amount)
So on Ethereum it pays for 2 approvals, everywhere else just 1. No need for chain-specific logic in the processor layer when the contract already handles it optimally.
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.
| bytes8 paymentReference, | ||
| uint256 captureAmount, | ||
| uint16 feeBps, | ||
| address feeReceiver |
There was a problem hiding this comment.
Fee basis points validation missing - could cause underflow
The feeBps parameter has no upper bound validation. If feeBps > 10000 (100%), the fee calculation would result in feeAmount > captureAmount, causing merchantAmount underflow and transaction revert.
Recommendation: Add validation before fee calculation:
if (feeBps > 10000) revert InvalidFeeBps();This issue also affects _transferToMerchant (line 524).
| mapping(bytes8 => PaymentData) public payments; | ||
|
|
||
| /// @notice Internal payment data structure | ||
| struct PaymentData { |
There was a problem hiding this comment.
PaymentData struct should use struct packing for gas efficiency
The current PaymentData struct uses 11 storage slots when it could use only 6 with proper packing. This wastes ~40% gas on every payment operation.
The Commerce Escrow already uses packed structs (uint120 for amounts, uint48 for timestamps). The wrapper should follow the same pattern:
Recommended packed layout:
struct PaymentData {
address payer; // Slot 0 (20 bytes)
bool isActive; // Slot 0 (1 byte) ← PACKED
address merchant; // Slot 1 (20 bytes)
uint96 amount; // Slot 1 (12 bytes) ← PACKED
address operator; // Slot 2 (20 bytes)
uint96 maxAmount; // Slot 2 (12 bytes) ← PACKED
address token; // Slot 3 (20 bytes)
uint48 preApprovalExpiry; // Slot 3 (6 bytes) ← PACKED
uint48 authorizationExpiry; // Slot 3 (6 bytes) ← PACKED
uint48 refundExpiry; // Slot 4 (6 bytes)
bytes32 commercePaymentHash;// Slot 5 (32 bytes)
}Benefits:
- 45% storage gas savings per payment (11 slots → 6 slots)
- Type consistency with Commerce Escrow interface
- Eliminates overflow validation in
_createPaymentInfoFromStored(types match escrow requirements) uint96supports up to ~79B tokens (18 decimals) - sufficient for all practical use casesuint48timestamps valid until year 8,921,556 - no practical limitation
|
|
||
| // 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 |
There was a problem hiding this comment.
Consider adding dedicated InvalidPayer error for clarity
The onlyPayer modifier reuses InvalidOperator error with a comment "Reusing the same error for simplicity". While this saves a few lines of code, it reduces clarity and makes debugging harder for integrators.
Recommendation: Define a dedicated error:
error InvalidPayer(address sender, address expectedPayer);Then use it in the modifier:
if (msg.sender != payment.payer) {
revert InvalidPayer(msg.sender, payment.payer);
}Why this is worth doing despite the "simplicity" comment:
- No runtime gas impact (both errors encode the same parameter types)
- Improves error semantics and self-documentation
- Makes off-chain error handling more precise (different error selectors)
- Better developer experience when debugging failed transactions
| // Capture from escrow with NO FEE - let ERC20FeeProxy handle fee distribution | ||
| // This way the wrapper receives the full captureAmount | ||
| commerceEscrow.capture(paymentInfo, captureAmount, 0, address(0)); | ||
|
|
There was a problem hiding this comment.
Commerce Escrow already prevents double-capture
The wrapper never stores capturableAmount - it always queries commerceEscrow.paymentState(). Commerce Escrow already prevents double-capture by tracking capturableAmount and reverting with InsufficientAuthorization if capture exceeds available amount.
No need to add wrapper-level state tracking for payment finalization - delegate state management to Commerce Escrow.
| mapping(bytes8 => PaymentData) public payments; | ||
|
|
||
| /// @notice Internal payment data structure | ||
| struct PaymentData { |
There was a problem hiding this comment.
Consider delegating existence check to Commerce Escrow
The isActive boolean duplicates information already tracked by Commerce Escrow's paymentState[hash].hasCollectedPayment.
Could simplify by checking commerceEscrow.paymentState(hash).hasCollectedPayment instead of maintaining separate wrapper state. This would eliminate potential state synchronization issues and reduce storage costs.
| uint256 merchantAmount = captureAmount - feeAmount; | ||
|
|
||
| // Approve ERC20FeeProxy to spend the full amount we received | ||
| IERC20(payment.token).forceApprove(address(erc20FeeProxy), captureAmount); |
There was a problem hiding this comment.
forceApprove is required for USDT compatibility.
USDT requires allowances to be reset to 0 before setting a new non-zero value. Regular approve() will revert if there's any existing allowance.
forceApprove (from OpenZeppelin's SafeERC20) handles this automatically:
- Tries
approve(spender, value) - If that fails, does
approve(spender, 0)thenapprove(spender, value)
This is the recommended OpenZeppelin pattern for supporting USDT and other non-standard tokens.
| const transactions: Array<{ to: string; data: string; value: number }> = []; | ||
|
|
||
| if (isUSDT) { | ||
| const resetData = paymentTokenContract.interface.encodeFunctionData('approve', [ |
There was a problem hiding this comment.
You're right - the USDT approval quirk is Ethereum mainnet only. Other chains don't have this restriction.
Recommendation: Drop the manual approve(0) reset from the payment processor.
The smart contract already uses forceApprove from OpenZeppelin's SafeERC20, which handles this intelligently:
- Tries
approve(spender, amount)first - Only if that fails, does
approve(0)thenapprove(amount)
So on Ethereum it pays for 2 approvals, everywhere else just 1. No need for chain-specific logic in the processor layer when the contract already handles it optimally.
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.
There was a problem hiding this comment.
Tests verify event emissions and state storage but never check actual token balance changes. This is a critical gap - events can emit correctly while tokens fail to transfer.
Missing across all functions:
- ✅ Authorization: Verify payer → escrow token flow
- ✅ Capture: Verify escrow → merchant + fee receiver split
- ✅ Void: Verify escrow → payer refund
- ✅ Charge: Verify payer → merchant + fee receiver
- ✅ Reclaim: Verify escrow → payer
- ✅ Refund: Verify operator → payer
Example fix for capture:
it('should transfer correct token amounts during capture', async () => {
const merchantBefore = await testERC20.balanceOf(merchantAddress);
const feeReceiverBefore = await testERC20.balanceOf(feeReceiverAddress);
await wrapper.connect(operator).capturePayment(
paymentReference, captureAmount, feeBps, feeReceiverAddress
);
const feeAmount = captureAmount.mul(feeBps).div(10000);
expect(await testERC20.balanceOf(merchantAddress))
.to.equal(merchantBefore.add(captureAmount.sub(feeAmount)));
expect(await testERC20.balanceOf(feeReceiverAddress))
.to.equal(feeReceiverBefore.add(feeAmount));
expect(await testERC20.balanceOf(wrapper.address))
.to.equal(0, 'Tokens should not get stuck in wrapper');
});Checklist:
- Add balance checks to authorization tests
- Add balance checks to capture tests
- Add balance checks to void tests
- Add balance checks to charge tests
- Add balance checks to reclaim tests
- Add balance checks to refund tests (see separate comment)
| .capturePayment(authParams.paymentReference, amount, feeBps, feeReceiverAddress); | ||
| }); | ||
|
|
||
| it('should revert if called by non-operator (access control test)', async () => { |
There was a problem hiding this comment.
Refund functionality has zero happy path tests (only access control). If refund flows are deferred to future work, consider:
- Add TODO comment indicating refund tests are planned
- Document refund flow requirements so future tests cover:
- Operator liquidity provision
- Token transfer verification
- Partial refund scenarios
- Add integration tests if unit tests are too complex with mocks
Current note (lines 640-652):
// Note: Refund functionality test is complex due to mock contract interactions
// The wrapper expects operator to have tokens and approve the tokenCollector
// This is tested in integration tests with real contractsSuggested update:
// TODO: Add refund functionality tests in future PR
// Refund flows are deferred to post-MVP work
// Integration tests should cover:
// - Operator provides liquidity for refund
// - Payer receives refund tokens
// - Partial refunds work correctlyNot blocking if refund is out of scope for initial release.
| 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.
Confirming this finding.
The "Reentrancy Protection" test suite (lines 816-883) validates that operations succeed under the nonReentrant modifier, but doesn't actually test reentrancy attack scenarios.
What's missing:
A malicious ERC20 contract that attempts reentrant calls during token transfer callbacks. For example:
contract MaliciousReentrantToken is ERC20 {
ERC20CommerceEscrowWrapper public target;
function transfer(address to, uint256 amount) public override returns (bool) {
// Attempt reentrancy during transfer
if (to == address(target)) {
target.capturePayment(...); // Should revert with ReentrancyGuard
}
return super.transfer(to, amount);
}
}Recommendation: Either implement proper reentrancy tests with malicious contracts, or remove these tests and document that reentrancy protection is verified through code review and the nonReentrant modifier from OpenZeppelin's battle-tested ReentrancyGuard.
| }); | ||
|
|
||
| it('should charge payment successfully', async () => { | ||
| const expectedFeeAmount = amount.mul(feeBps).div(10000); |
There was a problem hiding this comment.
Fee calculation tests verify events but not actual token amounts
The test calculates expectedFeeAmount but only verifies event emissions - it never checks that the fee receiver's token balance actually increased by this amount.
Current gap:
const expectedFeeAmount = amount.mul(feeBps).div(10000);
// ... test only checks events, not balanceOf()Recommended improvement to existing fee tests:
Add balance verification to existing chargePayment tests (and similar capture tests):
// Before charging payment
const merchantBalanceBefore = await testERC20.balanceOf(merchantAddress);
const feeReceiverBalanceBefore = await testERC20.balanceOf(feeReceiverAddress);
const tx = await wrapper.chargePayment(chargeParams);
// After charging payment - verify actual token transfers
const merchantBalanceAfter = await testERC20.balanceOf(merchantAddress);
const feeReceiverBalanceAfter = await testERC20.balanceOf(feeReceiverAddress);
const expectedFeeAmount = amount.mul(feeBps).div(10000);
const expectedMerchantAmount = amount.sub(expectedFeeAmount);
expect(merchantBalanceAfter.sub(merchantBalanceBefore)).to.equal(expectedMerchantAmount);
expect(feeReceiverBalanceAfter.sub(feeReceiverBalanceBefore)).to.equal(expectedFeeAmount);Also test edge cases:
- 0% fee (feeBps = 0): verify merchant gets full amount, fee receiver gets nothing
- 100% fee (feeBps = 10000): verify merchant gets nothing, fee receiver gets all
- 2.5% fee (current): verify exact split matches calculation
This ensures fees are actually transferred, not just emitted in events.
Severity: High - Missing critical validation of actual token transfers
There was a problem hiding this comment.
Fee mechanism design needs clarification
The current fee implementation has some architectural constraints that should be documented:
Current implementation:
- Fee is always paid by the merchant (subtracted from capture amount at line 389)
- Fee supports single recipient only (one
feeReceiveraddress) - Fee is a platform fee (not Request Network protocol/API fee)
- Fee calculation:
feeAmount = (captureAmount * feeBps) / 10000
Design questions to clarify:
-
Who pays the fee?
- Current: Merchant receives
captureAmount - feeAmount - Alternative: Could payer pay fee on top (like
amount + feeAmount)? - Recommendation: Document this is platform fee, merchant pays
- Current: Merchant receives
-
Single vs multiple fee recipients:
- Current: One
feeReceiveraddress - Future need: Request Network API fee + Platform fee split?
- Recommendation: Document future enhancement path if dual fees are needed
- Current: One
-
Fee type naming:
- "Fee" is ambiguous (platform? protocol? API?)
- Recommendation: Clarify in comments this is "Platform Fee" to distinguish from RN protocol fees
-
Configurability:
- Should there be an option for payer-pays-fee vs merchant-pays-fee?
- Currently hard-coded as merchant-pays
Suggested documentation additions:
/// @param feeBps Platform fee in basis points (paid by merchant, subtracted from capture)
/// @param feeReceiver Platform fee recipient (separate from RN protocol fees)Severity: Medium - Architectural clarity needed for future extensibility
| throw new Error('ERC20CommerceEscrowWrapper requires network parameter'); | ||
| } | ||
| // Constructor requires commerceEscrow address and erc20FeeProxy address | ||
| // Using the deployed AuthCaptureEscrow address |
There was a problem hiding this comment.
Hardcoded Commerce Escrow address will break multi-chain deployments
The Commerce Escrow address is hardcoded to a single value for all networks, but each blockchain will have its own deployed AuthCaptureEscrow contract.
Current issue:
const commerceEscrowAddress = '0xBdEA0D1bcC5966192B070Fdf62aB4EF5b4420cff';This assumes the same address exists on mainnet, sepolia, matic, bsc, etc., which is unlikely.
Recommended fix:
Create an artifacts file for AuthCaptureEscrow following the existing pattern used by ERC20FeeProxy:
- Create
packages/smart-contracts/src/lib/artifacts/AuthCaptureEscrow/index.ts:
import { CurrencyTypes } from '@requestnetwork/types';
export default {
getAddress(networkName: CurrencyTypes.EvmChainName): string {
switch (networkName) {
case 'mainnet':
return '0xBdEA0D1bcC5966192B070Fdf62aB4EF5b4420cff';
case 'sepolia':
return '0x...'; // Sepolia deployment address
case 'matic':
return '0x...'; // Polygon deployment address
// ... other networks
default:
throw new Error(`No AuthCaptureEscrow deployment for network: ${networkName}`);
}
}
};- Update constructor-args.ts:
case 'ERC20CommerceEscrowWrapper': {
const commerceEscrowArtifact = artifacts.authCaptureEscrowArtifact;
const commerceEscrowAddress = commerceEscrowArtifact.getAddress(network);
const erc20FeeProxy = artifacts.erc20FeeProxyArtifact;
const erc20FeeProxyAddress = erc20FeeProxy.getAddress(network);
return [commerceEscrowAddress, erc20FeeProxyAddress];
}This ensures proper network-specific addresses and prevents deployment failures.
Severity: High - Will cause incorrect deployments on non-mainnet chains
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.
| const wrapperContract = erc20CommerceEscrowWrapperArtifact.connect(network, provider); | ||
|
|
||
| // Use utils.Interface to encode with the raw ABI to avoid TypeScript interface issues | ||
| const iface = new utils.Interface( |
There was a problem hiding this comment.
Inconsistent Interface usage pattern
encodeAuthorizePayment and encodeChargePayment use utils.Interface workaround (lines 127, 218) while encodeCapturePayment uses direct interface.encodeFunctionData (line 169).
Reason: Functions with many parameters (12+ params) hit TypeScript limitations when encoding. The workaround is needed for authorize/charge but not for simpler functions like capture (4 params).
Recommendation: Add JSDoc comment to document the pattern:
/**
* @remarks
* Uses utils.Interface to handle large parameter count (TypeScript encoding limitation).
* Similar pattern in single-request-forwarder.ts.
*/This makes the inconsistency intentional rather than appearing accidental.
Summary by CodeRabbit
New Features
Tests
Infrastructure
Fix #1650