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 ERC20 Commerce Escrow subsystem: new Solidity contracts (wrapper, interface, mocks, attacker, Echidna harness), TypeScript types and runtime integration, artifacts and deployment tooling (CREATE2/deploy task), extensive unit/Echidna tests, security CI/workflows and helper scripts, docs, and a small Docker-compose platform entry and allowance simplification. Changes
Sequence Diagram(s)sequenceDiagram
participant Payer
participant Token as ERC20
participant Wrapper as ERC20CommerceEscrowWrapper
participant Escrow as IAuthCaptureEscrow
participant FeeProxy as ERC20FeeProxy
rect rgba(230,240,255,0.9)
Note over Payer,Wrapper: Authorization
Payer->>Token: approve/transferFrom to Wrapper
Wrapper->>Escrow: authorize(PaymentInfo, amount, tokenCollector, collectorData)
Escrow-->>Wrapper: commercePaymentHash
Wrapper-->>Wrapper: store PaymentData
end
rect rgba(232,255,232,0.9)
Note over Wrapper,FeeProxy: Capture / Charge
Wrapper->>Escrow: capture/charge(PaymentInfo, captureAmount, feeBps, feeReceiver)
Escrow-->>Wrapper: update capturable/refundable
Wrapper->>FeeProxy: transferFromWithReferenceAndFee(token, merchantAmount, paymentRef, feeAmount, feeReceiver)
FeeProxy->>Token: distribute(merchant, feeReceiver)
end
rect rgba(255,242,230,0.9)
Note over Wrapper,Escrow: Void / Reclaim / Refund
alt Void
Wrapper->>Escrow: void(PaymentInfo)
else Reclaim
Payer->>Wrapper: reclaimPayment(paymentRef)
Wrapper->>Escrow: reclaim(PaymentInfo)
else Refund
Wrapper->>Token: transferFrom(operator, Wrapper, refundAmount)
Wrapper->>Escrow: refund(PaymentInfo, refundAmount, tokenCollector, collectorData)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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.
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
bassgeta
left a comment
bassgeta
left a comment
There was a problem hiding this comment.
Looking good, nice work on this and the exhaustive test coverage 🔥
The TS wrapper looks nice and clean, and the contract itself has a lot of comments that explain what's going on.
🚢
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
Outdated
Show resolved
Hide resolved
packages/payment-processor/src/payment/erc20-commerce-escrow-wrapper.ts
Outdated
Show resolved
Hide resolved
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
Partial review - ERC20CommerceEscrowWrapper.sol only
I've reviewed ERC20CommerceEscrowWrapper.sol and posted comments on:
- Missing
feeBpsupper bound validation (high priority) PaymentDatastruct packing opportunity (45% storage savings)InvalidPayererror for better debugging- State delegation to Commerce Escrow (architectural note)
Haven't reviewed the other 16 files yet. The contract looks solid overall - main concerns are the fee validation and storage optimization.
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
Outdated
Show resolved
Hide resolved
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
Outdated
Show resolved
Hide resolved
packages/payment-processor/src/payment/erc20-commerce-escrow-wrapper.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Partial review - Test file and deployment script
I've reviewed the test file and deployment configuration, posting comments on:
Test file (ERC20CommerceEscrowWrapper.test.ts):
- Missing token balance verification across all test cases (critical priority)
- Fee calculation tests need balance checks, not just event verification
- Zero refund test coverage (medium priority - document if deferred)
Deployment script (constructor-args.ts):
- Hardcoded Commerce Escrow address needs network-specific artifacts pattern
Haven't reviewed the other 14 files yet. Test coverage is comprehensive for state/events but needs token movement verification.
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
Show resolved
Hide resolved
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
Code Review Summary
Completed comprehensive review of all 18 files in this PR. The Commerce Escrow payment functionality is well-implemented with proper integration into the Request Network ecosystem.
Highlights:
✅ Smart contract follows established patterns and includes proper validation
✅ Type definitions are comprehensive and match contract structures
✅ Deployment scripts properly configured for CREATE2 deployment
✅ Test coverage is excellent for encoding and integration flows
✅ Payment processor integration follows codebase conventions
Key Issue Identified:
- Interface usage inconsistency in
erc20-commerce-escrow-wrapper.ts(see inline comment)
The implementation is production-ready after addressing the inline comment regarding Interface usage documentation.
…testing scripts - Introduced a new deployment script for ERC20CommerceEscrowWrapper, utilizing official Base contracts. - Added a test script for Base Sepolia deployment, demonstrating wallet creation and deployment process. - Implemented a malicious token contract for testing reentrancy protection in ERC20CommerceEscrowWrapper. - Enhanced unit tests to validate reentrancy protection across various payment functions, ensuring robustness against attacks.
…oving USDT handling - Removed the special handling for USDT in `encodeSetCommerceEscrowAllowance` and `encodeSetRecurringAllowance` functions, streamlining the approval process to a single transaction for all ERC20 tokens. - Updated related tests to reflect the changes, ensuring they now validate the single transaction behavior for token approvals.
…ency - Reorganized the PaymentData struct to reduce storage slots from 11 to 6, achieving approximately 45% gas savings. - Updated data types for amount and expiry fields to smaller types (uint96, uint48) to enhance storage efficiency. - Adjusted related functions to ensure proper validation and casting of payment parameters, maintaining functionality while improving performance.
…lag and error handling - Added an `isActive` boolean to the `PaymentData` struct for efficient existence checks and improved state management. - Introduced a new `InvalidPayer` error for clearer error handling when the caller is not the designated payer. - Updated related logic to utilize the new error for better clarity in payment validation.
…o use unique payment reference - Renamed the test for maximum fee basis points to focus on authorizing payments with a unique payment reference. - Updated the test parameters to reflect the new focus, ensuring clarity and relevance in the test case.
…ract structure - Moved the declarations of `paymentStates` and `authorizedPayments` mappings to follow the `PaymentState` struct, enhancing the readability and organization of the contract code.
…with driver functions and invariants - Transformed the Echidna harness to include six new driver functions that enable comprehensive property-based testing of state-changing operations. - Enhanced invariants to ensure accounting consistency, token conservation, and proper fee calculations. - Updated mock contracts to accurately simulate token transfers, improving the realism of tests. - Added extensive documentation detailing the architecture, testing methodology, and key improvements for maintainability.
❌ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/smart-contracts/scripts/run-slither.sh (1)
64-148: Python report generation is well-structured, with one minor string formatting nit.The embedded Python script correctly parses the Slither JSON report and generates both text and Markdown summaries. However, on line 124, the string slice always appends "..." even for descriptions shorter than 300 characters.
-out.write(f'{finding["description"][:300]}...\n\n') +desc = finding["description"] +out.write(f'{desc[:300] + "..." if len(desc) > 300 else desc}\n\n')This is a minor cosmetic issue and can be deferred unless precision in report formatting is critical.
.github/workflows/security-slither.yml (1)
107-107: Consider standardizing conditional syntax for consistency.The workflow uses mixed styles for conditionals:
- Lines 107 and 134: bare context syntax (
if: github.event_name == 'pull_request' && always())- Line 176: expression wrapper syntax (
if: ${{ github.event_name == 'pull_request' && ... }})Both are functionally equivalent per GitHub Actions documentation, but standardizing to one style (preferably the
${{ }}wrapper for all expressions) would improve readability.- if: github.event_name == 'pull_request' && always() + if: ${{ github.event_name == 'pull_request' && always() }}Also applies to: 134-134, 176-176
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol (2)
40-41: Consider SafeERC20 or explicit return-value checks for token transfersAll ERC20 interactions (
transfer/transferFrom) assume success and ignore the returnedbool. While this is a test harness, usingSafeERC20(or at least checking return values) would make the mock more robust to non‑standard tokens and closer to production patterns.
Based on learnings-import '@openzeppelin/contracts/token/ERC20/IERC20.sol'; +import '@openzeppelin/contracts/token/ERC20/IERC20.sol'; +import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol'; + +using SafeERC20 for IERC20; @@ - IERC20(paymentInfo.token).transferFrom(paymentInfo.payer, address(this), amount); + IERC20(paymentInfo.token).safeTransferFrom(paymentInfo.payer, address(this), amount); @@ - IERC20(paymentInfo.token).transfer(paymentInfo.receiver, captureAmount); + IERC20(paymentInfo.token).safeTransfer(paymentInfo.receiver, captureAmount); @@ - IERC20(paymentInfo.token).transfer(paymentInfo.payer, amountToVoid); + IERC20(paymentInfo.token).safeTransfer(paymentInfo.payer, amountToVoid); @@ - IERC20(paymentInfo.token).transferFrom(paymentInfo.payer, paymentInfo.receiver, amount); + IERC20(paymentInfo.token).safeTransferFrom(paymentInfo.payer, paymentInfo.receiver, amount); @@ - IERC20(paymentInfo.token).transfer(paymentInfo.payer, amountToReclaim); + IERC20(paymentInfo.token).safeTransfer(paymentInfo.payer, amountToReclaim); @@ - IERC20(paymentInfo.token).transferFrom(paymentInfo.operator, address(this), refundAmount); - IERC20(paymentInfo.token).transfer(paymentInfo.payer, refundAmount); + IERC20(paymentInfo.token).safeTransferFrom(paymentInfo.operator, address(this), refundAmount); + IERC20(paymentInfo.token).safeTransfer(paymentInfo.payer, refundAmount);Also applies to: 66-67, 98-99, 118-119, 139-140, 160-161
131-145: Optionally alignreclaimbehavior withvoidfor zero-amount cases
reclaimcurrently allows calls even whencapturableAmountis zero and will emitReclaimCalledafter transferring0tokens.voidinstead reverts when there is nothing to void. For clearer semantics and easier reasoning in tests, you might want to mirrorvoid’s behavior by requiring a non‑zero amount to reclaim.function reclaim(PaymentInfo memory paymentInfo) external override { bytes32 hash = this.getHash(paymentInfo); require(authorizedPayments[hash], 'Payment not authorized'); PaymentState storage state = paymentStates[hash]; - uint120 amountToReclaim = state.capturableAmount; + uint120 amountToReclaim = state.capturableAmount; + require(amountToReclaim > 0, 'Nothing to reclaim');packages/smart-contracts/ECHIDNA_CHANGES_SUMMARY.md (2)
51-56: Add languages to generic fenced code blocks to satisfy markdownlintSome fenced blocks don’t specify a language (markdownlint MD040). Adding a generic language such as
textwill keep formatting and fix lint errors.-``` +```text authorize(1000) → capture(500, 2.5%) → void() → authorize(2000) → charge(1500, 1%)@@
-+text
✅ ECHIDNA_HARNESS_IMPROVEMENTS.md (600 lines)
- Detailed explanation of all changes
- Before/after comparison
- Testing methodology
- Security properties verified
@@
-+text
✅ 718 lines total
✅ 6 driver functions
✅ 16 invariant functions
✅ 3 mock contracts (ERC20, AuthCaptureEscrow, ERC20FeeProxy)
✅ Comprehensive documentationAlso applies to: 121-137, 281-287 --- `345-347`: **Wrap bare URLs in Markdown links to meet MD034 (no-bare-urls)** Two reference links are bare URLs; wrapping them in Markdown links will satisfy markdownlint. ```diff -- **Echidna Docs:** https://github.com/crytic/echidna -- **Property Testing:** https://trail-of-bits.github.io/echidna/ +- **Echidna Docs:** [github.com/crytic/echidna](https://github.com/crytic/echidna) +- **Property Testing:** [trail-of-bits.github.io/echidna](https://trail-of-bits.github.io/echidna/)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/security-slither.yml(1 hunks)packages/smart-contracts/ECHIDNA_CHANGES_SUMMARY.md(1 hunks)packages/smart-contracts/scripts/run-slither.sh(1 hunks)packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol(1 hunks)packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol(1 hunks)packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
- packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:11.730Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:30-34
Timestamp: 2024-10-17T16:00:11.730Z
Learning: In `packages/smart-contracts/src/contracts/test/UsdtFake.sol`, the code is part of the test harness and not intended for production, so deviations from ERC20 standards are acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:27.352Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:36-43
Timestamp: 2024-10-17T16:00:27.352Z
Learning: The `UsdtFake.sol` contract is a test harness, not intended for production, so standard compliance issues can be ignored in this file.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-21T09:06:12.938Z
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.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-12-18T03:53:54.370Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:03.170Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:63-66
Timestamp: 2024-10-17T16:00:03.170Z
Learning: In test harness contracts like `UsdtFake.sol`, which are not meant for production, having an unrestricted `mint` function is acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
🪛 markdownlint-cli2 (0.18.1)
packages/smart-contracts/ECHIDNA_CHANGES_SUMMARY.md
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
123-123: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
281-281: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
346-346: Bare URL used
(MD034, no-bare-urls)
347-347: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: Slither Static Analysis
- GitHub Check: build-and-test
🔇 Additional comments (4)
packages/smart-contracts/scripts/run-slither.sh (1)
40-52: EXIT_CODE capture fix has been correctly applied.The script now properly captures Slither's exit code at line 50 with
EXIT_CODE=$?and uses it to determine the script's exit status at lines 150–157. This resolves the previous issue whereEXIT_CODEwas hard-coded to 0, preventing proper failure detection..github/workflows/security-slither.yml (2)
106-131: ✅ Shell script hardening properly implemented.The parse step now correctly uses grouped redirections with quoted
$GITHUB_OUTPUT(lines 118–123 and 125–130), which addresses the previous shellcheck warnings (SC2129, SC2086). The implementation satisfies both actionlint and shellcheck requirements.
175-179: ✅ Failure condition now correctly targets High severity findings.The fail step now checks
steps.parse.outputs.HIGH != '0'instead of relying on the Slither exit code, ensuring the job fails only when High severity issues are detected. The step name and logic intent are now properly aligned. This directly addresses the previous review feedback.packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol (1)
7-182: Overall escrow mock state machine looks coherent for testingThe authorization, capture, void, charge, reclaim, and refund flows form a consistent state machine and move tokens in ways that make sense for the Echidna harness (escrow holds funds on authorize, sends to receiver on capture/charge, returns to payer on void/reclaim/refund). The
PaymentStateaccounting (capturable vs refundable) also lines up with expected escrow semantics for fuzzing.
Based on learnings
…e through wrapper - Modified payment voiding, reclaiming, and refunding processes to ensure funds are routed through the wrapper before reaching the payer. - Updated comments for clarity on the new flow of funds, aligning with the actual escrow behavior. - Adjusted test cases to reflect changes in payment authorization and token transfer logic, ensuring accurate assertions on token amounts during various payment operations.
❌ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts (1)
1680-1715:refundPaymentreentrancy test still a no‑opThis test is titled “should prevent reentrancy attack on refundPayment”, but it:
- Authorizes and captures a payment with the regular
testERC20,- Calls
maliciousToken.setupAttack(...),- Then exits without ever calling
wrapper.refundPayment(...)or asserting on events/reverts.As written, it cannot exercise the nonReentrant guard or the malicious token path and will continue to pass even if reentrancy protection is removed or miswired.
Given earlier feedback on this exact test, consider either:
- Wiring it up to an actual refund attempt (e.g., arranging for the malicious token to be involved in the refund path and asserting an
AttackAttemptedevent and/or revert), or- Converting it to
it.skipwith a clear TODO comment explaining that refund reentrancy coverage is deferred due to token-collector complexity.This avoids false security coverage while keeping intent documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol(1 hunks)packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol(1 hunks)packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (38)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.solpackages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-17T16:00:11.730Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:30-34
Timestamp: 2024-10-17T16:00:11.730Z
Learning: In `packages/smart-contracts/src/contracts/test/UsdtFake.sol`, the code is part of the test harness and not intended for production, so deviations from ERC20 standards are acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.solpackages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-17T16:00:27.352Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:36-43
Timestamp: 2024-10-17T16:00:27.352Z
Learning: The `UsdtFake.sol` contract is a test harness, not intended for production, so standard compliance issues can be ignored in this file.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-12-18T03:53:54.370Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.solpackages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-17T16:00:03.170Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:63-66
Timestamp: 2024-10-17T16:00:03.170Z
Learning: In test harness contracts like `UsdtFake.sol`, which are not meant for production, having an unrestricted `mint` function is acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-28T20:00:25.780Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/smart-contracts/scripts/test-deploy-single-request-proxy.ts:14-32
Timestamp: 2024-10-28T20:00:25.780Z
Learning: In test files, such as `packages/smart-contracts/scripts/test-deploy-single-request-proxy.ts`, extensive error handling and input validation are considered unnecessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-08T18:24:06.144Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:237-246
Timestamp: 2024-11-08T18:24:06.144Z
Learning: In `packages/payment-processor/test/payment/single-request-proxy.test.ts`, when asserting the `feeProxyUsed` in emitted events from `SingleRequestProxyFactory`, retrieve the `erc20FeeProxy` (or `ethereumFeeProxy`) public variable from the `SingleRequestProxyFactory` contract instead of using `wallet.address`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-06T14:48:18.698Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1484
File: packages/advanced-logic/test/extensions/payment-network/any-to-near.test.ts:0-0
Timestamp: 2024-11-06T14:48:18.698Z
Learning: In `packages/advanced-logic/test/extensions/payment-network/any-to-near.test.ts`, when the existing happy path tests are deemed sufficient, additional test cases may not be necessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-12-04T05:05:19.610Z
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.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-21T09:06:12.938Z
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.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-12T17:48:47.072Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:211-219
Timestamp: 2024-11-12T17:48:47.072Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, the error handling logic in `payWithEthereumSingleRequestForwarder` is correct and does not require changes to differentiate between ERC20 and Ethereum SingleRequestForwarder contracts.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-17T18:33:41.124Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts:0-0
Timestamp: 2024-10-17T18:33:41.124Z
Learning: In the `EthereumSingleRequestProxy` contract, the custom reentrancy guard is nearly identical to OpenZeppelin's ReentrancyGuard, except that it allows the first reentrancy call from the `ERC20FeeProxy`. We assume it works correctly without needing further tests.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-28T21:11:48.524Z
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.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-12-09T18:59:04.613Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts:80-99
Timestamp: 2024-12-09T18:59:04.613Z
Learning: In the `RequestNetwork` codebase, payment processor functions such as `payErc20ProxyRequestFromHinkalShieldedAddress` in `packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts` should not include explicit request validation or try/catch blocks, and should rely on the underlying components (like `hinkalObject`) to handle error reporting.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-05T14:43:07.965Z
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`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-08T18:24:19.095Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:198-206
Timestamp: 2024-11-08T18:24:19.095Z
Learning: In tests for `SingleRequestProxyFactory`, the `feeProxyUsed` in events should be verified by retrieving the `ethereumFeeProxy` public variable from the `SingleRequestProxyFactory` contract, not `wallet.address`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-04T14:32:57.040Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc20-fee-proxy.ts:186-186
Timestamp: 2024-11-04T14:32:57.040Z
Learning: In the `preparePrivateErc20FeeProxyPaymentTransaction` function in `packages/payment-processor/src/payment/erc20-fee-proxy.ts`, we should not make an allowance call on the token contract, as it is not relevant to our use case.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-29T08:02:02.600Z
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.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-02T05:15:18.868Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1316
File: packages/utils/test/retry.test.ts:149-216
Timestamp: 2024-10-02T05:15:18.868Z
Learning: Developers prefer unrolled loops and explicit comments in test code, particularly in 'retry.test.ts' for the exponential backoff test, to show the number of calls and total elapsed time for retries, rather than refactoring using loops.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-05T05:33:32.481Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1478
File: packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts:38-43
Timestamp: 2024-11-05T05:33:32.481Z
Learning: In the RequestNetwork codebase, setup scripts such as `setupSingleRequestProxyFactory.ts` do not include contract existence checks before interacting with contracts, even though scripts like `check-deployer.ts` do include such checks.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-04T12:18:13.888Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:19-27
Timestamp: 2024-11-04T12:18:13.888Z
Learning: In the file `packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts`, when using `payPrivateErc20FeeProxyRequest`, the returned `tx` is a `RelayerTransaction`, so calling `.wait()` on it is unnecessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-05T14:43:16.298Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol:46-46
Timestamp: 2024-10-05T14:43:16.298Z
Learning: In `packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol`, within the `receive()` function, it's acceptable to approve the entire `balance` to `erc20FeeProxy` because `balance == paymentAmount + feeAmount`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2025-06-23T09:35:04.263Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1633
File: packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol:92-133
Timestamp: 2025-06-23T09:35:04.263Z
Learning: In the ERC20RecurringPaymentProxy contract, events were intentionally omitted to save gas costs since the underlying ERC20FeeProxy contract already emits events when payments are processed. This avoids duplicate event emissions while maintaining observability.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-29T09:00:54.169Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/src/payment/single-request-proxy.ts:202-209
Timestamp: 2024-10-29T09:00:54.169Z
Learning: In the `packages/payment-processor/src/payment/single-request-proxy.ts` file, within the `payWithEthereumSingleRequestProxy` function, the current error handling is acceptable as per the project's expectations.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-22T13:13:26.166Z
Learnt from: rodrigopavezi
Repo: RequestNetwork/requestNetwork PR: 1475
File: packages/transaction-manager/test/unit/utils/test-data.ts:0-0
Timestamp: 2024-11-22T13:13:26.166Z
Learning: In `packages/transaction-manager/test/unit/utils/test-data.ts`, the `FakeLitProtocolProvider` class uses `{}` as the return type for its methods `encrypt` and `decrypt`. Changing the return type to a more specific interface caused issues, so the current return type `{}` should remain as is.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-05T05:04:01.710Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1478
File: packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts:45-58
Timestamp: 2024-11-05T05:04:01.710Z
Learning: When executing blockchain transactions in scripts (e.g., in `packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts`), ensure that transactions are executed serially rather than in parallel to maintain correct execution order and prevent potential issues.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-12T16:54:02.702Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:104-104
Timestamp: 2024-11-12T16:54:02.702Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, when the smart contract has not changed, event argument names such as `proxyAddress` remain the same, even if variable names in the code are updated to use new terminology like `forwarderAddress`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.tspackages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-05T14:43:14.816Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol:11-15
Timestamp: 2024-10-05T14:43:14.816Z
Learning: In the `EthereumSingleRequestProxy` contract, the state variables `payee`, `paymentReference`, `feeAddress`, `feeAmount`, and `ethereumFeeProxy` should remain public, as they need to be accessed externally.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-09-27T11:42:01.062Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/SingleRequestProxyFactory.sol:0-0
Timestamp: 2024-09-27T11:42:01.062Z
Learning: In the `createEthereumSingleRequestProxy` function, it's acceptable for `_feeAddress` to be the zero address, as setting the fee address to zero is a common practice. Adding zero address checks is unnecessary and adds extra gas cost.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-15T07:50:54.734Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol:77-78
Timestamp: 2024-10-15T07:50:54.734Z
Learning: In the `receive()` function of the `EthereumSingleRequestProxy` contract (`packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol`), the current error handling for the call to `ethereumFeeProxy` is considered sufficient, and no additional error handling is required.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-28T20:00:53.026Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/smart-contracts/src/lib/artifacts/SingleRequestProxyFactory/0.1.0.json:24-124
Timestamp: 2024-10-28T20:00:53.026Z
Learning: In our smart contracts, we should remove the `indexed` keyword from all event parameters except for `paymentReference`, as we use the payment-subgraph for indexing and this reduces gas costs.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-15T07:50:58.094Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/SingleRequestProxyFactory.sol:22-27
Timestamp: 2024-10-15T07:50:58.094Z
Learning: In the `SingleRequestProxyFactory` contract, the `paymentReference` parameter in events should remain `indexed`, even if removing `indexed` could reduce gas costs.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-08T20:01:10.313Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/src/payment/single-request-proxy.ts:0-0
Timestamp: 2024-11-08T20:01:10.313Z
Learning: In `packages/payment-processor/src/payment/single-request-proxy.ts`, when decoding event data, the team prefers not to include documentation of the event data structure if maintaining it would be difficult.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-04T20:53:41.522Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/prepare-hinkal.ts:5-13
Timestamp: 2024-11-04T20:53:41.522Z
Learning: When reviewing the RequestNetwork codebase, avoid suggesting the addition of example usage in documentation if it would make the comments overly verbose, especially for functions like `prepareHinkal` in `packages/payment-processor/src/payment/prepare-hinkal.ts`.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-04T20:54:04.489Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/prepared-transaction.ts:9-17
Timestamp: 2024-11-04T20:54:04.489Z
Learning: In the `IPreparedPrivateTransaction` interface within `packages/payment-processor/src/payment/prepared-transaction.ts`, the team prefers concise documentation comments and avoids overly verbose explanations.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: Slither Static Analysis
- GitHub Check: build-and-test
🔇 Additional comments (2)
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts (1)
180-201: Balance checks around authorization, capture, and charge look solidThe tests now assert concrete token balances for payer, merchant, fee receiver, escrow, and the wrapper across:
- authorization (
should transfer correct token amounts during authorization),- capture (including partial and multi-fee scenarios),
- and charge,
and consistently verify that
wrapper.addressends with zero balance. This closes the earlier gap where only events were checked and ensures real token flows match expectations.Also applies to: 377-407, 996-1021
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol (1)
10-70: Wrapper fee/state design and guards look consistent and robustThe wrapper:
- Clearly documents and implements a merchant‑pays platform fee model, bypassing Commerce Escrow’s own fee mechanism and routing all fees through
ERC20FeeProxy.- Uses packed
PaymentDatawithuint96/uint48and explicitScalarOverflowchecks before downcasting, matching the types used in tests.- Validates
feeBps(≤ 10000) in bothcapturePaymentand_transferToMerchantto prevent underflow in merchant amounts.- Applies
ReentrancyGuardand role‑based modifiers (onlyOperator,onlyPayer) on all state‑changing methods that touch escrow and ERC20 flows.- Delegates payment-state truth to
commerceEscrow.paymentStatevia the stored hash, avoiding duplicate state machines in the wrapper.No additional issues spotted; the implementation aligns well with the accompanying tests and previous feedback.
Also applies to: 141-210, 248-295, 439-486, 559-663, 667-707, 709-762
…ments - Removed the tokenCollector parameter from the refund function, as the wrapper now handles token transfers directly. - Updated comments to clarify the new flow of funds and the role of the wrapper in managing refunds, enhancing code readability and alignment with actual behavior.
❌ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol (2)
31-51: Consider preventing duplicate authorization.The
authorizefunction can be called multiple times for the samepaymentHash. Each call will pull additional tokens from the payer but overwrite the previous state, potentially losing track of escrowed tokens and causing misleading test results. If the real escrow contract prevents duplicate authorization, the mock should match that behavior.Consider adding a check:
function authorize( PaymentInfo memory paymentInfo, uint256 amount, address, /* tokenCollector */ bytes calldata /* collectorData */ ) external override { bytes32 hash = this.getHash(paymentInfo); + require(!authorizedPayments[hash], 'Payment already authorized'); // Transfer tokens from payer to this contract (simulating escrow) IERC20(paymentInfo.token).transferFrom(paymentInfo.payer, address(this), amount);
132-147: Add validation to prevent zero-value reclaim.Unlike
void()(line 94),reclaim()doesn't validate thatcapturableAmount > 0. This allows a zero-value transfer and emitsReclaimCalledwith a zero amount, which could be misleading in tests.Consider adding a check for consistency:
function reclaim(PaymentInfo memory paymentInfo) external override { bytes32 hash = this.getHash(paymentInfo); require(authorizedPayments[hash], 'Payment not authorized'); PaymentState storage state = paymentStates[hash]; + require(state.capturableAmount > 0, 'Nothing to reclaim'); uint120 amountToReclaim = state.capturableAmount;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:11.730Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:30-34
Timestamp: 2024-10-17T16:00:11.730Z
Learning: In `packages/smart-contracts/src/contracts/test/UsdtFake.sol`, the code is part of the test harness and not intended for production, so deviations from ERC20 standards are acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:27.352Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:36-43
Timestamp: 2024-10-17T16:00:27.352Z
Learning: The `UsdtFake.sol` contract is a test harness, not intended for production, so standard compliance issues can be ignored in this file.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-12T17:48:47.072Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:211-219
Timestamp: 2024-11-12T17:48:47.072Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, the error handling logic in `payWithEthereumSingleRequestForwarder` is correct and does not require changes to differentiate between ERC20 and Ethereum SingleRequestForwarder contracts.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-04T14:32:57.040Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc20-fee-proxy.ts:186-186
Timestamp: 2024-11-04T14:32:57.040Z
Learning: In the `preparePrivateErc20FeeProxyPaymentTransaction` function in `packages/payment-processor/src/payment/erc20-fee-proxy.ts`, we should not make an allowance call on the token contract, as it is not relevant to our use case.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-08T18:24:06.144Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:237-246
Timestamp: 2024-11-08T18:24:06.144Z
Learning: In `packages/payment-processor/test/payment/single-request-proxy.test.ts`, when asserting the `feeProxyUsed` in emitted events from `SingleRequestProxyFactory`, retrieve the `erc20FeeProxy` (or `ethereumFeeProxy`) public variable from the `SingleRequestProxyFactory` contract instead of using `wallet.address`.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-12-09T18:59:04.613Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts:80-99
Timestamp: 2024-12-09T18:59:04.613Z
Learning: In the `RequestNetwork` codebase, payment processor functions such as `payErc20ProxyRequestFromHinkalShieldedAddress` in `packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts` should not include explicit request validation or try/catch blocks, and should rely on the underlying components (like `hinkalObject`) to handle error reporting.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-29T09:00:54.169Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/src/payment/single-request-proxy.ts:202-209
Timestamp: 2024-10-29T09:00:54.169Z
Learning: In the `packages/payment-processor/src/payment/single-request-proxy.ts` file, within the `payWithEthereumSingleRequestProxy` function, the current error handling is acceptable as per the project's expectations.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-28T21:11:48.524Z
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.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-12-18T03:53:54.370Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:03.170Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:63-66
Timestamp: 2024-10-17T16:00:03.170Z
Learning: In test harness contracts like `UsdtFake.sol`, which are not meant for production, having an unrestricted `mint` function is acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: Slither Static Analysis
- GitHub Check: build-and-test
🔇 Additional comments (1)
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol (1)
149-166: LGTM! Refund fix correctly addresses previous review.The
refund()implementation now correctly matches the wrapper flow. The mock no longer attempts token transfers (which would revert due to missing allowances) and simply updates the refundable state and emits the event, while the wrapper handles the actual token movement to the payer via ERC20FeeProxy.
…ents - Revised comments to better explain the simplified refund process in the mock escrow, detailing the role of the wrapper in handling token transfers. - Removed unnecessary token transfer logic from the mock, as the wrapper already manages the tokens, streamlining the internal state update process.
❌ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol (1)
27-29: Consider makinggetHashinternal for efficiency.The function is called via
this.getHash()throughout the contract, which uses an external call. Making itinternalorprivatewould be more gas-efficient. However, since this is a test mock where gas costs are not critical, the current pattern is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol(1 hunks)packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/smart-contracts/src/contracts/test/EchidnaERC20CommerceEscrowWrapper.sol
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:11.730Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:30-34
Timestamp: 2024-10-17T16:00:11.730Z
Learning: In `packages/smart-contracts/src/contracts/test/UsdtFake.sol`, the code is part of the test harness and not intended for production, so deviations from ERC20 standards are acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:27.352Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:36-43
Timestamp: 2024-10-17T16:00:27.352Z
Learning: The `UsdtFake.sol` contract is a test harness, not intended for production, so standard compliance issues can be ignored in this file.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-12T17:48:47.072Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:211-219
Timestamp: 2024-11-12T17:48:47.072Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, the error handling logic in `payWithEthereumSingleRequestForwarder` is correct and does not require changes to differentiate between ERC20 and Ethereum SingleRequestForwarder contracts.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-08T18:24:06.144Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:237-246
Timestamp: 2024-11-08T18:24:06.144Z
Learning: In `packages/payment-processor/test/payment/single-request-proxy.test.ts`, when asserting the `feeProxyUsed` in emitted events from `SingleRequestProxyFactory`, retrieve the `erc20FeeProxy` (or `ethereumFeeProxy`) public variable from the `SingleRequestProxyFactory` contract instead of using `wallet.address`.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-04T14:32:57.040Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc20-fee-proxy.ts:186-186
Timestamp: 2024-11-04T14:32:57.040Z
Learning: In the `preparePrivateErc20FeeProxyPaymentTransaction` function in `packages/payment-processor/src/payment/erc20-fee-proxy.ts`, we should not make an allowance call on the token contract, as it is not relevant to our use case.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-12-09T18:59:04.613Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts:80-99
Timestamp: 2024-12-09T18:59:04.613Z
Learning: In the `RequestNetwork` codebase, payment processor functions such as `payErc20ProxyRequestFromHinkalShieldedAddress` in `packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts` should not include explicit request validation or try/catch blocks, and should rely on the underlying components (like `hinkalObject`) to handle error reporting.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-29T09:00:54.169Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/src/payment/single-request-proxy.ts:202-209
Timestamp: 2024-10-29T09:00:54.169Z
Learning: In the `packages/payment-processor/src/payment/single-request-proxy.ts` file, within the `payWithEthereumSingleRequestProxy` function, the current error handling is acceptable as per the project's expectations.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-12-18T03:53:54.370Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-12T16:54:02.702Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:104-104
Timestamp: 2024-11-12T16:54:02.702Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, when the smart contract has not changed, event argument names such as `proxyAddress` remain the same, even if variable names in the code are updated to use new terminology like `forwarderAddress`.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:03.170Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:63-66
Timestamp: 2024-10-17T16:00:03.170Z
Learning: In test harness contracts like `UsdtFake.sol`, which are not meant for production, having an unrestricted `mint` function is acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-21T09:06:12.938Z
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.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Slither Static Analysis
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: build-and-test
🔇 Additional comments (2)
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol (2)
1-9: LGTM!The contract setup and imports are appropriate for a mock escrow test harness.
10-25: LGTM!The state structures and events provide appropriate test observability for the mock escrow contract.
… avoid invalid payment references - Updated the test counter to start at 1 instead of 0, preventing the generation of an invalid payment reference (0x0000000000000000). - Incremented the counter within the unique payment reference generation function to ensure unique references for each test case.
❌ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts (1)
1609-1716: Non-skipped reentrancy tests don't actually verify reentrancy protection.The non-skipped reentrancy tests (voidPayment, reclaimPayment, refundPayment) only verify that functions don't revert, but don't actually test whether reentrancy attacks are blocked. The refundPayment test is particularly problematic - it sets up an attack but never calls
refundPayment, making it a complete no-op.These tests create false confidence in security coverage without providing meaningful verification.
Recommendation: Either implement proper reentrancy verification (e.g., verify that
AttackAttemptedevents showsuccess=false) or replace these tests with a comment documenting that reentrancy protection relies on OpenZeppelin's battle-testedReentrancyGuardmodifier (similar to the approach forEthereumSingleRequestProxy).Based on learnings
Example refactor (Option 1 - Document reliance on OpenZeppelin):
- describe('Reentrancy Protection', () => { - let maliciousToken: MaliciousReentrant; - beforeEach(async () => { - maliciousToken = await new MaliciousReentrant__factory(owner).deploy(...); - }); - describe('voidPayment reentrancy', () => { - it('should prevent reentrancy attack on voidPayment', async () => { - // ... test that only checks function doesn't revert - }); - }); - // ... similar tests - }); + // Note: Reentrancy protection is provided by OpenZeppelin's ReentrancyGuard modifier + // on all state-changing functions (authorizePayment, capturePayment, voidPayment, etc.). + // This protection is verified through code review and has been battle-tested across + // thousands of production deployments. The malicious token tests are skipped due to + // SafeERC20's incompatibility with non-standard token implementations.
📜 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 (28)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-12-18T03:53:54.370Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-28T20:00:25.780Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/smart-contracts/scripts/test-deploy-single-request-proxy.ts:14-32
Timestamp: 2024-10-28T20:00:25.780Z
Learning: In test files, such as `packages/smart-contracts/scripts/test-deploy-single-request-proxy.ts`, extensive error handling and input validation are considered unnecessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-08T18:24:06.144Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:237-246
Timestamp: 2024-11-08T18:24:06.144Z
Learning: In `packages/payment-processor/test/payment/single-request-proxy.test.ts`, when asserting the `feeProxyUsed` in emitted events from `SingleRequestProxyFactory`, retrieve the `erc20FeeProxy` (or `ethereumFeeProxy`) public variable from the `SingleRequestProxyFactory` contract instead of using `wallet.address`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-12-04T05:05:19.610Z
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.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-06T14:48:18.698Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1484
File: packages/advanced-logic/test/extensions/payment-network/any-to-near.test.ts:0-0
Timestamp: 2024-11-06T14:48:18.698Z
Learning: In `packages/advanced-logic/test/extensions/payment-network/any-to-near.test.ts`, when the existing happy path tests are deemed sufficient, additional test cases may not be necessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-21T09:06:12.938Z
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.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-12T17:48:47.072Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:211-219
Timestamp: 2024-11-12T17:48:47.072Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, the error handling logic in `payWithEthereumSingleRequestForwarder` is correct and does not require changes to differentiate between ERC20 and Ethereum SingleRequestForwarder contracts.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-17T18:33:41.124Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts:0-0
Timestamp: 2024-10-17T18:33:41.124Z
Learning: In the `EthereumSingleRequestProxy` contract, the custom reentrancy guard is nearly identical to OpenZeppelin's ReentrancyGuard, except that it allows the first reentrancy call from the `ERC20FeeProxy`. We assume it works correctly without needing further tests.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-28T21:11:48.524Z
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.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-12-09T18:59:04.613Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts:80-99
Timestamp: 2024-12-09T18:59:04.613Z
Learning: In the `RequestNetwork` codebase, payment processor functions such as `payErc20ProxyRequestFromHinkalShieldedAddress` in `packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts` should not include explicit request validation or try/catch blocks, and should rely on the underlying components (like `hinkalObject`) to handle error reporting.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-05T14:43:07.965Z
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`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-08T18:24:19.095Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:198-206
Timestamp: 2024-11-08T18:24:19.095Z
Learning: In tests for `SingleRequestProxyFactory`, the `feeProxyUsed` in events should be verified by retrieving the `ethereumFeeProxy` public variable from the `SingleRequestProxyFactory` contract, not `wallet.address`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-04T14:32:57.040Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc20-fee-proxy.ts:186-186
Timestamp: 2024-11-04T14:32:57.040Z
Learning: In the `preparePrivateErc20FeeProxyPaymentTransaction` function in `packages/payment-processor/src/payment/erc20-fee-proxy.ts`, we should not make an allowance call on the token contract, as it is not relevant to our use case.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-29T08:02:02.600Z
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.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-02T05:15:18.868Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1316
File: packages/utils/test/retry.test.ts:149-216
Timestamp: 2024-10-02T05:15:18.868Z
Learning: Developers prefer unrolled loops and explicit comments in test code, particularly in 'retry.test.ts' for the exponential backoff test, to show the number of calls and total elapsed time for retries, rather than refactoring using loops.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-05T05:33:32.481Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1478
File: packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts:38-43
Timestamp: 2024-11-05T05:33:32.481Z
Learning: In the RequestNetwork codebase, setup scripts such as `setupSingleRequestProxyFactory.ts` do not include contract existence checks before interacting with contracts, even though scripts like `check-deployer.ts` do include such checks.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-04T12:18:13.888Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:19-27
Timestamp: 2024-11-04T12:18:13.888Z
Learning: In the file `packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts`, when using `payPrivateErc20FeeProxyRequest`, the returned `tx` is a `RelayerTransaction`, so calling `.wait()` on it is unnecessary.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-05T14:43:16.298Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol:46-46
Timestamp: 2024-10-05T14:43:16.298Z
Learning: In `packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol`, within the `receive()` function, it's acceptable to approve the entire `balance` to `erc20FeeProxy` because `balance == paymentAmount + feeAmount`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2025-06-23T09:35:04.263Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1633
File: packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol:92-133
Timestamp: 2025-06-23T09:35:04.263Z
Learning: In the ERC20RecurringPaymentProxy contract, events were intentionally omitted to save gas costs since the underlying ERC20FeeProxy contract already emits events when payments are processed. This avoids duplicate event emissions while maintaining observability.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-10-29T09:00:54.169Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/src/payment/single-request-proxy.ts:202-209
Timestamp: 2024-10-29T09:00:54.169Z
Learning: In the `packages/payment-processor/src/payment/single-request-proxy.ts` file, within the `payWithEthereumSingleRequestProxy` function, the current error handling is acceptable as per the project's expectations.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-22T13:13:26.166Z
Learnt from: rodrigopavezi
Repo: RequestNetwork/requestNetwork PR: 1475
File: packages/transaction-manager/test/unit/utils/test-data.ts:0-0
Timestamp: 2024-11-22T13:13:26.166Z
Learning: In `packages/transaction-manager/test/unit/utils/test-data.ts`, the `FakeLitProtocolProvider` class uses `{}` as the return type for its methods `encrypt` and `decrypt`. Changing the return type to a more specific interface caused issues, so the current return type `{}` should remain as is.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-05T05:04:01.710Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1478
File: packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts:45-58
Timestamp: 2024-11-05T05:04:01.710Z
Learning: When executing blockchain transactions in scripts (e.g., in `packages/smart-contracts/scripts-create2/contract-setup/setupSingleRequestProxyFactory.ts`), ensure that transactions are executed serially rather than in parallel to maintain correct execution order and prevent potential issues.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-12T16:54:02.702Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:104-104
Timestamp: 2024-11-12T16:54:02.702Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, when the smart contract has not changed, event argument names such as `proxyAddress` remain the same, even if variable names in the code are updated to use new terminology like `forwarderAddress`.
Applied to files:
packages/smart-contracts/test/contracts/ERC20CommerceEscrowWrapper.test.ts
📚 Learning: 2024-11-04T20:55:51.395Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:23-32
Timestamp: 2024-11-04T20:55:51.395Z
Learning: In `packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts`, the team has decided not to implement a delay or graceful shutdown handling after `process.exit(0)`, as they consider it unnecessary.
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). (3)
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: Slither Static Analysis
- GitHub Check: build-and-test
…for security - Updated the token approval logic to reset approvals to zero after each use, improving security against potential token misuse. - Revised comments for clarity on the payment flow and the role of ERC20FeeProxy in handling token transfers, ensuring better understanding of the code's functionality.
❌ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol (1)
375-376: Consider removing redundant validation for gas efficiency.Lines 375-376 validate
amountandmaxAmountagainstuint96, but these checks are redundant:
_storePaymentDatais only called from_executeAuthorization(line 282) and_executeCharge(line 581)- Both callers invoke
_createPaymentInfofirst, which already validates these values (lines 334-335)- The comment on line 361 acknowledges: "Values are validated in _createPaymentInfo before this function is called"
If you prefer defense-in-depth, the redundant checks are harmless. If you prefer gas optimization, you could remove them:
function _storePaymentData( bytes8 paymentReference, address payer, address merchant, address operator, address token, uint256 amount, uint256 maxAmount, uint256 preApprovalExpiry, uint256 authorizationExpiry, uint256 refundExpiry, bytes32 commerceHash ) internal { - if (amount > type(uint96).max) revert ScalarOverflow(); - if (maxAmount > type(uint96).max) revert ScalarOverflow(); - payments[paymentReference] = PaymentData({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
📚 Learning: 2024-10-05T14:43:14.816Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol:11-15
Timestamp: 2024-10-05T14:43:14.816Z
Learning: In the `EthereumSingleRequestProxy` contract, the state variables `payee`, `paymentReference`, `feeAddress`, `feeAmount`, and `ethereumFeeProxy` should remain public, as they need to be accessed externally.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-12T16:54:02.702Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:104-104
Timestamp: 2024-11-12T16:54:02.702Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, when the smart contract has not changed, event argument names such as `proxyAddress` remain the same, even if variable names in the code are updated to use new terminology like `forwarderAddress`.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-12T17:48:47.072Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:211-219
Timestamp: 2024-11-12T17:48:47.072Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, the error handling logic in `payWithEthereumSingleRequestForwarder` is correct and does not require changes to differentiate between ERC20 and Ethereum SingleRequestForwarder contracts.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-12-09T18:59:04.613Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts:80-99
Timestamp: 2024-12-09T18:59:04.613Z
Learning: In the `RequestNetwork` codebase, payment processor functions such as `payErc20ProxyRequestFromHinkalShieldedAddress` in `packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts` should not include explicit request validation or try/catch blocks, and should rely on the underlying components (like `hinkalObject`) to handle error reporting.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-28T21:11:48.524Z
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.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-08T18:24:06.144Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:237-246
Timestamp: 2024-11-08T18:24:06.144Z
Learning: In `packages/payment-processor/test/payment/single-request-proxy.test.ts`, when asserting the `feeProxyUsed` in emitted events from `SingleRequestProxyFactory`, retrieve the `erc20FeeProxy` (or `ethereumFeeProxy`) public variable from the `SingleRequestProxyFactory` contract instead of using `wallet.address`.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-09-27T11:42:01.062Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/SingleRequestProxyFactory.sol:0-0
Timestamp: 2024-09-27T11:42:01.062Z
Learning: In the `createEthereumSingleRequestProxy` function, it's acceptable for `_feeAddress` to be the zero address, as setting the fee address to zero is a common practice. Adding zero address checks is unnecessary and adds extra gas cost.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-05T14:43:16.298Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol:46-46
Timestamp: 2024-10-05T14:43:16.298Z
Learning: In `packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol`, within the `receive()` function, it's acceptable to approve the entire `balance` to `erc20FeeProxy` because `balance == paymentAmount + feeAmount`.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-04T14:32:57.040Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc20-fee-proxy.ts:186-186
Timestamp: 2024-11-04T14:32:57.040Z
Learning: In the `preparePrivateErc20FeeProxyPaymentTransaction` function in `packages/payment-processor/src/payment/erc20-fee-proxy.ts`, we should not make an allowance call on the token contract, as it is not relevant to our use case.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-08T18:24:19.095Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:198-206
Timestamp: 2024-11-08T18:24:19.095Z
Learning: In tests for `SingleRequestProxyFactory`, the `feeProxyUsed` in events should be verified by retrieving the `ethereumFeeProxy` public variable from the `SingleRequestProxyFactory` contract, not `wallet.address`.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-15T07:50:54.734Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol:77-78
Timestamp: 2024-10-15T07:50:54.734Z
Learning: In the `receive()` function of the `EthereumSingleRequestProxy` contract (`packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol`), the current error handling for the call to `ethereumFeeProxy` is considered sufficient, and no additional error handling is required.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2025-06-23T09:35:04.263Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1633
File: packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol:92-133
Timestamp: 2025-06-23T09:35:04.263Z
Learning: In the ERC20RecurringPaymentProxy contract, events were intentionally omitted to save gas costs since the underlying ERC20FeeProxy contract already emits events when payments are processed. This avoids duplicate event emissions while maintaining observability.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-28T20:00:53.026Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/smart-contracts/src/lib/artifacts/SingleRequestProxyFactory/0.1.0.json:24-124
Timestamp: 2024-10-28T20:00:53.026Z
Learning: In our smart contracts, we should remove the `indexed` keyword from all event parameters except for `paymentReference`, as we use the payment-subgraph for indexing and this reduces gas costs.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-15T07:50:58.094Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/SingleRequestProxyFactory.sol:22-27
Timestamp: 2024-10-15T07:50:58.094Z
Learning: In the `SingleRequestProxyFactory` contract, the `paymentReference` parameter in events should remain `indexed`, even if removing `indexed` could reduce gas costs.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-08T20:01:10.313Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/src/payment/single-request-proxy.ts:0-0
Timestamp: 2024-11-08T20:01:10.313Z
Learning: In `packages/payment-processor/src/payment/single-request-proxy.ts`, when decoding event data, the team prefers not to include documentation of the event data structure if maintaining it would be difficult.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-04T20:53:41.522Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/prepare-hinkal.ts:5-13
Timestamp: 2024-11-04T20:53:41.522Z
Learning: When reviewing the RequestNetwork codebase, avoid suggesting the addition of example usage in documentation if it would make the comments overly verbose, especially for functions like `prepareHinkal` in `packages/payment-processor/src/payment/prepare-hinkal.ts`.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-11-04T20:54:04.489Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/prepared-transaction.ts:9-17
Timestamp: 2024-11-04T20:54:04.489Z
Learning: In the `IPreparedPrivateTransaction` interface within `packages/payment-processor/src/payment/prepared-transaction.ts`, the team prefers concise documentation comments and avoids overly verbose explanations.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
📚 Learning: 2024-10-05T14:43:15.124Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol:36-56
Timestamp: 2024-10-05T14:43:15.124Z
Learning: In the `ERC20SingleRequestProxy` contract (`packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol`), the `receive()` function is intentionally used to trigger logic when the smart contract receives a 0 ETH transfer, as per the design. Replacing it with a named function is not appropriate in this context.
Applied to files:
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: Slither Static Analysis
- GitHub Check: build-and-test
🔇 Additional comments (3)
packages/smart-contracts/src/contracts/ERC20CommerceEscrowWrapper.sol (3)
1-246: Excellent security and design patterns throughout the contract.The implementation demonstrates strong engineering practices:
- ReentrancyGuard protection on all state-changing operations
- SafeERC20 for USDT compatibility (approval reset pattern)
- Immutable references prevent upgrade attacks
- Custom errors for gas efficiency
- Struct packing optimizes storage to 6 slots (45% savings)
- Clear fee architecture documentation (merchant-pays model)
- Access control properly delegates existence checks to
commercePaymentHash- Events follow team standards (only
paymentReferenceindexed)
779-834: LGTM: View functions properly delegate to Commerce Escrow.The view functions correctly:
- Check payment existence via
commercePaymentHash != bytes32(0)- Delegate state queries to Commerce Escrow's
paymentState()- Provide clean interfaces for checking operation eligibility
This maintains the design principle of minimal wrapper state with authoritative state in Commerce Escrow.
322-338: Verify that Commerce Escrow validates temporal ordering of expiry timestamps.The wrapper accepts
preApprovalExpiry,authorizationExpiry, andrefundExpirywithout validating their temporal ordering (e.g., ensuringpreApprovalExpiry < authorizationExpiry < refundExpiry). The interface definition and test mock implementation (MockAuthCaptureEscrow) contain no such validation logic.The actual Commerce Escrow contract is not in this repository, so its validation cannot be verified here. Please confirm with the Commerce Escrow implementation that it validates these temporal relationships to prevent logical inconsistencies.
…claiming - Introduced a requirement to ensure that the capturable amount is greater than zero before allowing a reclaim operation, preventing unnecessary transactions and improving contract safety.
❌ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol (1)
89-149: Void/charge/reclaim token routing aligns with the wrapper‑centric designBoth
voidandreclaimmove the remainingcapturableAmountfrom the mock topaymentInfo.receiver(the wrapper) and zero it out, trusting the wrapper to forward funds back to the payer, whilechargedirectly pulls tokens frompaymentInfo.payerto the wrapper and marks the full amount as refundable. If you later needhasCollectedPaymentto distinguish “funds currently in escrow” from “payment fully settled”, consider adding extra state transitions, but for this mock the current, simpler model is adequate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 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.
📚 Learning: 2024-10-17T18:30:55.410Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts:150-152
Timestamp: 2024-10-17T18:30:55.410Z
Learning: In `packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts`, the skipped test `'should process a partial payment correctly'` exists intentionally to show that partial payments are supported without duplicating previous happy-path tests.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:11.730Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:30-34
Timestamp: 2024-10-17T16:00:11.730Z
Learning: In `packages/smart-contracts/src/contracts/test/UsdtFake.sol`, the code is part of the test harness and not intended for production, so deviations from ERC20 standards are acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:27.352Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:36-43
Timestamp: 2024-10-17T16:00:27.352Z
Learning: The `UsdtFake.sol` contract is a test harness, not intended for production, so standard compliance issues can be ignored in this file.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-12T17:48:47.072Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:211-219
Timestamp: 2024-11-12T17:48:47.072Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, the error handling logic in `payWithEthereumSingleRequestForwarder` is correct and does not require changes to differentiate between ERC20 and Ethereum SingleRequestForwarder contracts.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-28T16:03:33.215Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:251-270
Timestamp: 2024-10-28T16:03:33.215Z
Learning: When testing the payment-processor module, specifically in `packages/payment-processor/test/payment/single-request-proxy.test.ts`, it's acceptable to omit tests for partial payments if they have already been covered at the smart-contract level.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-08T18:24:06.144Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:237-246
Timestamp: 2024-11-08T18:24:06.144Z
Learning: In `packages/payment-processor/test/payment/single-request-proxy.test.ts`, when asserting the `feeProxyUsed` in emitted events from `SingleRequestProxyFactory`, retrieve the `erc20FeeProxy` (or `ethereumFeeProxy`) public variable from the `SingleRequestProxyFactory` contract instead of using `wallet.address`.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-04T14:32:57.040Z
Learnt from: giorgi-kiknavelidze
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc20-fee-proxy.ts:186-186
Timestamp: 2024-11-04T14:32:57.040Z
Learning: In the `preparePrivateErc20FeeProxyPaymentTransaction` function in `packages/payment-processor/src/payment/erc20-fee-proxy.ts`, we should not make an allowance call on the token contract, as it is not relevant to our use case.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-12-09T18:59:04.613Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts:80-99
Timestamp: 2024-12-09T18:59:04.613Z
Learning: In the `RequestNetwork` codebase, payment processor functions such as `payErc20ProxyRequestFromHinkalShieldedAddress` in `packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts` should not include explicit request validation or try/catch blocks, and should rely on the underlying components (like `hinkalObject`) to handle error reporting.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-29T09:00:54.169Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1474
File: packages/payment-processor/src/payment/single-request-proxy.ts:202-209
Timestamp: 2024-10-29T09:00:54.169Z
Learning: In the `packages/payment-processor/src/payment/single-request-proxy.ts` file, within the `payWithEthereumSingleRequestProxy` function, the current error handling is acceptable as per the project's expectations.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-12-18T03:53:54.370Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-12T16:54:02.702Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1488
File: packages/payment-processor/src/payment/single-request-forwarder.ts:104-104
Timestamp: 2024-11-12T16:54:02.702Z
Learning: In `packages/payment-processor/src/payment/single-request-forwarder.ts`, when the smart contract has not changed, event argument names such as `proxyAddress` remain the same, even if variable names in the code are updated to use new terminology like `forwarderAddress`.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-11-05T16:53:05.280Z
Learnt from: MantisClone
Repo: RequestNetwork/requestNetwork PR: 1481
File: packages/integration-test/test/scheduled/erc20-proxy.test.ts:0-0
Timestamp: 2024-11-05T16:53:05.280Z
Learning: In `packages/integration-test/test/scheduled/erc20-proxy.test.ts`, when upgrading dependencies like `ethers`, additional error handling test cases for contract interactions and provider errors may not be necessary.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
📚 Learning: 2024-10-17T16:00:03.170Z
Learnt from: aimensahnoun
Repo: RequestNetwork/requestNetwork PR: 1453
File: packages/smart-contracts/src/contracts/test/UsdtFake.sol:63-66
Timestamp: 2024-10-17T16:00:03.170Z
Learning: In test harness contracts like `UsdtFake.sol`, which are not meant for production, having an unrestricted `mint` function is acceptable.
Applied to files:
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Echidna Property-Based Fuzzing
- GitHub Check: Slither Static Analysis
- GitHub Check: build-and-test
🔇 Additional comments (5)
packages/smart-contracts/src/contracts/test/MockAuthCaptureEscrow.sol (5)
7-25: State struct, mappings, and events are well‑shaped for the escrow test harness
PaymentStatekeyed bykeccak(paymentInfo)plus the dedicated events should give tests all the observability they need, and thebool + uint120 + uint120packing is fine given 0.8.x checked casts (very large amounts will just revert, which is acceptable in tests). I don't see any issues in this section for the intended mock usage. Based on learnings
31-51: Authorize flow correctly escrows funds and seeds initial state
authorizepulls tokens frompaymentInfo.payerinto the mock, marks the payment as authorized, and sets the fullamountas capturable with zero refundable balance, which is consistent with an “authorized but not yet captured” escrow state. Just ensure your test setup givesMockAuthCaptureEscrowsufficient allowance from the payer, since the wrapper contract itself is not the token spender in this step.
53-88: Capture andpaymentStatebookkeeping look internally consistentThe capture path enforces
capturableAmount >= captureAmount, transfers that amount from the mock to the wrapper (receiver), decreasescapturableAmount, and increasesrefundableAmount, which matches “captured but refundable” semantics; thepaymentStateview simply exposes the stored struct and should behave predictably even across multiple partial captures.
151-176: Refund logic now correctly limits itself to state updates and events
refundonly checksauthorizedPayments[hash], enforcesstate.refundableAmount >= refundAmount, decrementsrefundableAmount, and emitsRefundCalled, leaving the wrapper to handle the actual token movement; this matches the documented wrapper flow and avoids the previous allowance/transferFrommismatch that could cause reverts.
178-191:setPaymentStatehelper cleanly supports targeted test scenariosDirectly seeding
paymentStatesand flaggingauthorizedPayments[paymentHash] = truefrom tests is a practical way to reach edge states without replaying full flows, and is acceptable given this lives undercontracts/testand is not production code.
Summary by CodeRabbit
New Features
Behavioral Changes
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Fix #1650