feat: add recurring payment smart contract #1633
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes introduce a new recurring payments feature for ERC20 tokens on EVM-compatible blockchains. This includes a new Solidity smart contract ( Changes
Sequence Diagram(s)sequenceDiagram
participant Subscriber
participant Executor
participant ERC20RecurringPaymentProxy
participant ERC20Token
participant ERC20FeeProxy
participant Recipient
participant FeeAddress
Subscriber->>ERC20Token: Approve allowance to Proxy
Executor->>ERC20RecurringPaymentProxy: execute(permit, signature, index, ref)
ERC20RecurringPaymentProxy->>ERC20RecurringPaymentProxy: Validate signature, timing, and order
ERC20RecurringPaymentProxy->>ERC20Token: transferFrom(subscriber, proxy, amount+fee+gasFee)
ERC20RecurringPaymentProxy->>ERC20Token: approve(ERC20FeeProxy, amount+fee)
ERC20RecurringPaymentProxy->>ERC20FeeProxy: transferFromWithReferenceAndFee(token, recipient, amount, ref, fee, feeAddress)
ERC20FeeProxy->>Recipient: Transfer amount
ERC20FeeProxy->>FeeAddress: Transfer fee
ERC20RecurringPaymentProxy->>Executor: Transfer gasFee (if any)
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…t and add contract artifacts
…nd update bitmap logic
…ced access control
…C20RecurringPaymentProxy functionality
…ove unused dependencies
…cenarios and edge case validations
…eth_signTypedData_v4 for improved security
…port both object and string formats for compatibility
…ent test state across executions
…ion test with updated permit parameters
…owance for recurring payments
…data for ERC20 approval with multiple methods
…allowance and executing recurring payments with detailed error handling
…curring payment functions including allowance retrieval, approval encoding, and execution
…on for improved clarity and maintainability
… and execution data with validation checks
github-actions
bot
requested review from
alexandre-abrioux,
bassgeta,
leoslr and
rodrigopavezi
aimensahnoun
requested review from
MantisClone
and removed request for
alexandre-abrioux and
leoslr
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
packages/smart-contracts/test/contracts/ERC20RecurringPaymentProxy.test.ts (2)
248-251: Specify the expected revert reason for zero address validation.The test should verify the specific error message when setting zero address as fee proxy.
- await expect(erc20RecurringPaymentProxy.setFeeProxy(ethers.constants.AddressZero)).to.be - .reverted; + await expect( + erc20RecurringPaymentProxy.setFeeProxy(ethers.constants.AddressZero) + ).to.be.revertedWithCustomError( + erc20RecurringPaymentProxy, + 'ERC20RecurringPaymentProxy__ZeroAddress' + );
400-410: Add test case for malformed signature.The current test only verifies rejection when signed by wrong account. Consider adding a test for malformed signatures.
Add this test after the current "should revert with bad signature" test:
it('should revert with malformed signature', async () => { const permit = createSchedulePermit(); const malformedSignature = '0x1234'; // Too short to be valid const paymentReference = '0x1234567890abcdef'; await expect( erc20RecurringPaymentProxy .connect(executor) .execute(permit, malformedSignature, 1, paymentReference), ).to.be.reverted; });packages/payment-processor/src/payment/erc20-recurring-payment-proxy.ts (1)
19-21: Fix typo in documentation.Remove the double period at the end of the @throws documentation.
- * @throws {Error} If the `ERC20RecurringPaymentProxy` has no known deployment - * on the provided `network`.. + * @throws {Error} If the `ERC20RecurringPaymentProxy` has no known deployment + * on the provided `network`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/payment-processor/src/payment/erc20-recurring-payment-proxy.ts(1 hunks)packages/payment-processor/test/payment/erc-20-recurring-payment.test.ts(1 hunks)packages/smart-contracts/scripts-create2/compute-one-address.ts(1 hunks)packages/smart-contracts/scripts-create2/constructor-args.ts(2 hunks)packages/smart-contracts/scripts-create2/utils.ts(2 hunks)packages/smart-contracts/scripts/test-deploy-all.ts(2 hunks)packages/smart-contracts/scripts/test-deploy-erc20-recurring-payment-proxy.ts(1 hunks)packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol(1 hunks)packages/smart-contracts/src/lib/artifacts/ERC20RecurringPaymentProxy/0.1.0.json(1 hunks)packages/smart-contracts/src/lib/artifacts/ERC20RecurringPaymentProxy/index.ts(1 hunks)packages/smart-contracts/src/lib/artifacts/index.ts(1 hunks)packages/smart-contracts/test/contracts/ERC20RecurringPaymentProxy.test.ts(1 hunks)packages/types/src/payment-types.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
packages/smart-contracts/scripts/test-deploy-all.ts (1)
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.
packages/payment-processor/test/payment/erc-20-recurring-payment.test.ts (3)
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.
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.
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.
packages/smart-contracts/test/contracts/ERC20RecurringPaymentProxy.test.ts (2)
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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (21)
packages/smart-contracts/scripts-create2/compute-one-address.ts (1)
68-69: LGTM! Consistent integration with existing deployment logic.The addition of
ERC20RecurringPaymentProxyto the case block follows the established pattern for contracts requiring constructor arguments in CREATE2 deployment.packages/smart-contracts/src/lib/artifacts/index.ts (1)
18-18: LGTM! Proper artifact export following established patterns.The export is correctly placed in the Payment Proxies section and follows the naming convention.
packages/smart-contracts/scripts/test-deploy-all.ts (2)
11-11: LGTM! Proper import following established patterns.The import statement follows the existing naming convention and module structure.
24-24: LGTM! Correct deployment integration with proper dependency management.The deployment call correctly passes
mainPaymentAddresses.ERC20FeeProxyAddressas a dependency, indicating proper understanding of the contract's requirements. The placement at the end of the deployment sequence ensures all dependencies are available.packages/smart-contracts/src/lib/artifacts/ERC20RecurringPaymentProxy/index.ts (2)
1-6: LGTM! Proper artifact structure and imports.The imports and type definitions follow the established patterns for contract artifacts.
7-20: ```shell
#!/bin/bashSearch for CREATE2 or deterministic deployment address computation in the codebase
Look for direct use of ethers.utils.getCreate2Address
rg -i "getCreate2Address" -H -n -A5 -B3
Look for any generic computeAddress helper
rg -i "computeAddress" -H -n -A5 -B3
Catch any references to CREATE2 in scripts or typings
rg -i "create2" -H -n -A5 -B3
</details> <details> <summary>packages/smart-contracts/scripts-create2/utils.ts (2)</summary> `24-24`: **LGTM! Proper addition to CREATE2 deployment list.** The contract is correctly added to the deployment list following the established pattern and logical grouping with other proxy contracts. --- `63-64`: **LGTM! Consistent artifact resolution logic.** The case statement follows the existing pattern and correctly returns the corresponding artifact using the established naming convention. </details> <details> <summary>packages/types/src/payment-types.ts (1)</summary> `397-413`: **LGTM!** The `SchedulePermit` interface is well-structured with appropriate types for all fields. Using `BigNumberish` for numeric values ensures compatibility with ethers.js. </details> <details> <summary>packages/smart-contracts/scripts-create2/constructor-args.ts (1)</summary> `11-18`: **LGTM!** The helper function correctly checks for the required environment variable and throws a descriptive error. </details> <details> <summary>packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol (1)</summary> `125-126`: **Good implementation of USDT-safe approve pattern** The zero-approve before setting the actual allowance is a best practice for handling tokens like USDT that don't allow changing non-zero allowances. </details> <details> <summary>packages/smart-contracts/test/contracts/ERC20RecurringPaymentProxy.test.ts (8)</summary> `1-5`: **LGTM!** Imports are correctly organized and include all necessary dependencies. --- `6-61`: **LGTM!** Test setup is comprehensive with proper contract deployments and signer initialization. --- `63-137`: **LGTM!** Helper functions are well-implemented with proper EIP712 signature creation and provider compatibility handling. --- `139-161`: **LGTM!** Deployment tests thoroughly verify initial contract state including roles and pause status. --- `163-191`: **LGTM!** Access control tests properly verify role constants and initial role assignments. --- `193-228`: **LGTM!** setExecutor tests comprehensively cover access control, role changes, and event emissions. --- `332-602`: **Excellent test coverage for the execute function!** The tests comprehensively cover all execution scenarios including timing, ordering, replay protection, balance checks, and edge cases. --- `604-633`: **LGTM!** Integration test properly verifies that paused state blocks execution. </details> <details> <summary>packages/payment-processor/src/payment/erc20-recurring-payment-proxy.ts (1)</summary> `162-228`: **LGTM!** The helper functions `getRecurringPaymentProxyAddress` and `encodeRecurringPaymentExecution` are well-implemented. </details> <details> <summary>packages/smart-contracts/src/lib/artifacts/ERC20RecurringPaymentProxy/0.1.0.json (1)</summary> `1-572`: **LGTM!** The ABI artifact is complete and well-structured with proper error definitions, events, and function signatures for the ERC20RecurringPaymentProxy contract. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+4
to
+5
| export async function deployERC20RecurringPaymentProxy( | ||
| args: any, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use specific type instead of any for args parameter
The args parameter should have a specific type for better type safety and maintainability.
🤖 Prompt for AI Agents
In packages/smart-contracts/scripts/test-deploy-erc20-recurring-payment-proxy.ts
at lines 4 to 5, the args parameter is typed as any, which reduces type safety.
Replace the any type with a specific interface or type that accurately describes
the expected structure and properties of args to improve maintainability and
catch type errors early.
| hre, | ||
| 'ERC20RecurringPaymentProxy', | ||
| { | ||
| constructorArguments: [deployer.address, deployer.address, erc20FeeProxyAddress], |
There was a problem hiding this comment.
Avoid using the same address for admin and executor roles
Using deployer.address for both admin and executor roles violates the principle of separation of concerns. In production, these should be different addresses to ensure proper access control.
Consider using separate addresses:
- constructorArguments: [deployer.address, deployer.address, erc20FeeProxyAddress],
+ constructorArguments: [adminAddress, executorAddress, erc20FeeProxyAddress],Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/smart-contracts/scripts/test-deploy-erc20-recurring-payment-proxy.ts
at line 16, the constructorArguments array uses deployer.address for both admin
and executor roles, which breaks separation of concerns. Replace the second
deployer.address with a different address, such as another signer or a
predefined executor address, to ensure admin and executor roles are assigned to
distinct entities for proper access control.
| @@ -78,6 +87,18 @@ export const getConstructorArgs = ( | |||
|
|
|||
| return [ethereumFeeProxyAddress, erc20FeeProxyAddress, getAdminWalletAddress(contract)]; | |||
| } | |||
| case 'ERC20RecurringPaymentProxy': { | |||
| if (!network) { | |||
| throw new Error('SingleRequestProxyFactory requires network parameter'); | |||
There was a problem hiding this comment.
Fix incorrect error message
The error message incorrectly references SingleRequestProxyFactory instead of ERC20RecurringPaymentProxy.
Apply this fix:
- throw new Error('SingleRequestProxyFactory requires network parameter');
+ throw new Error('ERC20RecurringPaymentProxy requires network parameter');🤖 Prompt for AI Agents
In packages/smart-contracts/scripts-create2/constructor-args.ts at line 92, the
error message incorrectly mentions SingleRequestProxyFactory instead of
ERC20RecurringPaymentProxy. Update the error message string to correctly
reference ERC20RecurringPaymentProxy to accurately reflect the source of the
error.
| // Verify it's a valid hex string | ||
| expect(encodedData.startsWith('0x')).toBe(true); | ||
| // Verify it contains the execute method signature | ||
| expect(encodedData.includes('execute')).toBe(true); |
There was a problem hiding this comment.
Fix incorrect test assertion for method signature
The test checks if the encoded data includes the string 'execute', but it should check for the method signature hex instead.
The encoded data won't contain the string 'execute'. You should check for the method signature (first 4 bytes) instead:
- expect(encodedData.includes('execute')).toBe(true);
+ // Check for execute method signature (first 4 bytes)
+ expect(encodedData.slice(0, 10)).toMatch(/^0x[a-f0-9]{8}$/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(encodedData.includes('execute')).toBe(true); | |
| // Check for execute method signature (first 4 bytes) | |
| expect(encodedData.slice(0, 10)).toMatch(/^0x[a-f0-9]{8}$/); |
🤖 Prompt for AI Agents
In packages/payment-processor/test/payment/erc-20-recurring-payment.test.ts at
line 131, the test incorrectly asserts that the encoded data includes the string
'execute'. Instead, update the assertion to check that the encoded data starts
with or includes the correct method signature hex (the first 4 bytes of the
encoded function call) corresponding to the 'execute' method. Replace the string
check with a comparison against the method signature hex to correctly validate
the encoded data.
Comment on lines
+92
to
+133
| function execute( | ||
| SchedulePermit calldata p, | ||
| bytes calldata signature, | ||
| uint8 index, | ||
| bytes calldata paymentReference | ||
| ) external whenNotPaused onlyRole(EXECUTOR_ROLE) nonReentrant { | ||
| bytes32 digest = _hashSchedule(p); | ||
|
|
||
| if (digest.recover(signature) != p.subscriber) | ||
| revert ERC20RecurringPaymentProxy__BadSignature(); | ||
| if (block.timestamp > p.deadline) revert ERC20RecurringPaymentProxy__SignatureExpired(); | ||
|
|
||
| if (index >= 256) revert ERC20RecurringPaymentProxy__IndexTooLarge(); | ||
| if (index != lastExecutionIndex[digest] + 1) | ||
| revert ERC20RecurringPaymentProxy__ExecutionOutOfOrder(); | ||
| lastExecutionIndex[digest] = index; | ||
|
|
||
| if (index > p.totalExecutions) revert ERC20RecurringPaymentProxy__IndexOutOfBounds(); | ||
|
|
||
| uint256 execTime = uint256(p.firstExec) + uint256(index - 1) * p.periodSeconds; | ||
| if (block.timestamp < execTime) revert ERC20RecurringPaymentProxy__NotDueYet(); | ||
|
|
||
| uint256 mask = 1 << index; | ||
| uint256 word = executedBitmap[digest]; | ||
| if (word & mask != 0) revert ERC20RecurringPaymentProxy__AlreadyPaid(); | ||
| executedBitmap[digest] = word | mask; | ||
|
|
||
| uint256 total = p.amount + p.feeAmount + p.gasFee; | ||
|
|
||
| IERC20 token = IERC20(p.token); | ||
| token.safeTransferFrom(p.subscriber, address(this), total); | ||
|
|
||
| /* USDT-safe zero-approve then set allowance */ | ||
| token.safeApprove(address(erc20FeeProxy), 0); | ||
| token.safeApprove(address(erc20FeeProxy), p.amount + p.feeAmount); | ||
|
|
||
| _proxyTransfer(p, paymentReference); | ||
|
|
||
| if (p.gasFee != 0) { | ||
| token.safeTransfer(msg.sender, p.gasFee); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider emitting events for better observability
The contract doesn't emit any events, making it difficult to track recurring payment executions off-chain. Consider adding events for key operations.
Add event definitions and emit them in the execute function:
event RecurringPaymentExecuted(
bytes32 indexed scheduleHash,
uint8 indexed paymentIndex,
address indexed token,
address subscriber,
address recipient,
uint256 amount,
uint256 feeAmount,
uint256 gasFee
);Then emit the event after successful execution:
if (p.gasFee != 0) {
token.safeTransfer(msg.sender, p.gasFee);
}
+
+ emit RecurringPaymentExecuted(
+ digest,
+ index,
+ p.token,
+ p.subscriber,
+ p.recipient,
+ p.amount,
+ p.feeAmount,
+ p.gasFee
+ );🤖 Prompt for AI Agents
In packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol around
lines 92 to 133, the execute function lacks event emissions, reducing off-chain
observability of recurring payment executions. Define an event named
RecurringPaymentExecuted with parameters for scheduleHash, paymentIndex, token,
subscriber, recipient, amount, feeAmount, and gasFee. Then, emit this event at
the end of the execute function after all state changes and transfers succeed,
passing the appropriate values from the function arguments and local variables
to enable tracking of each executed payment.
Comment on lines
+73
to
+79
| function _hashSchedule(SchedulePermit calldata p) private view returns (bytes32) { | ||
| SchedulePermit memory m = p; | ||
|
|
||
| bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, m)); | ||
|
|
||
| return _hashTypedDataV4(structHash); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize gas by avoiding unnecessary memory copy
Creating a memory copy of the calldata parameter is unnecessary and wastes gas.
Apply this optimization:
function _hashSchedule(SchedulePermit calldata p) private view returns (bytes32) {
- SchedulePermit memory m = p;
-
- bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, m));
+ bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, p));
return _hashTypedDataV4(structHash);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function _hashSchedule(SchedulePermit calldata p) private view returns (bytes32) { | |
| SchedulePermit memory m = p; | |
| bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, m)); | |
| return _hashTypedDataV4(structHash); | |
| } | |
| function _hashSchedule(SchedulePermit calldata p) private view returns (bytes32) { | |
| bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, p)); | |
| return _hashTypedDataV4(structHash); | |
| } |
🤖 Prompt for AI Agents
In packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol around
lines 73 to 79, avoid creating a memory copy of the calldata parameter 'p' by
using it directly in the keccak256 encoding. Remove the line that copies 'p'
into memory and pass 'p' directly to abi.encode to reduce gas consumption.
Comment on lines
+239
to
+284
| * @param overrides - Optional transaction overrides (gas price, limit etc) | ||
| * | ||
| * @returns A Promise resolving to the transaction receipt after the payment is confirmed | ||
| * | ||
| * @throws {Error} If the ERC20RecurringPaymentProxy is not deployed on the specified network | ||
| * @throws {Error} If the transaction fails (e.g. wrong index, expired permit, insufficient allowance) | ||
| * | ||
| * @remarks | ||
| * • The function waits for the transaction to be mined before returning | ||
| * • The signer must have been granted EXECUTOR_ROLE by the proxy admin | ||
| * • Make sure all preconditions are met (allowance, balance, timing) before calling | ||
| */ | ||
| export async function executeRecurringPayment({ | ||
| permitTuple, | ||
| permitSignature, | ||
| paymentIndex, | ||
| paymentReference, | ||
| signer, | ||
| network, | ||
| }: { | ||
| permitTuple: PaymentTypes.SchedulePermit; | ||
| permitSignature: string; | ||
| paymentIndex: number; | ||
| paymentReference: string; | ||
| signer: Signer; | ||
| network: CurrencyTypes.EvmChainName; | ||
| }): Promise<providers.TransactionReceipt> { | ||
| const proxyAddress = getRecurringPaymentProxyAddress(network); | ||
|
|
||
| const data = encodeRecurringPaymentExecution({ | ||
| permitTuple, | ||
| permitSignature, | ||
| paymentIndex, | ||
| paymentReference, | ||
| network, | ||
| provider: signer, | ||
| }); | ||
|
|
||
| const tx = await signer.sendTransaction({ | ||
| to: proxyAddress, | ||
| data, | ||
| value: 0, | ||
| }); | ||
|
|
||
| return tx.wait(); | ||
| } |
There was a problem hiding this comment.
Add missing transaction overrides parameter.
The documentation mentions an overrides parameter for transaction settings, but it's not included in the function signature or implementation.
export async function executeRecurringPayment({
permitTuple,
permitSignature,
paymentIndex,
paymentReference,
signer,
network,
+ overrides = {},
}: {
permitTuple: PaymentTypes.SchedulePermit;
permitSignature: string;
paymentIndex: number;
paymentReference: string;
signer: Signer;
network: CurrencyTypes.EvmChainName;
+ overrides?: providers.TransactionRequest;
}): Promise<providers.TransactionReceipt> {
const proxyAddress = getRecurringPaymentProxyAddress(network);
const data = encodeRecurringPaymentExecution({
permitTuple,
permitSignature,
paymentIndex,
paymentReference,
network,
provider: signer,
});
const tx = await signer.sendTransaction({
to: proxyAddress,
data,
value: 0,
+ ...overrides,
});
return tx.wait();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * @param overrides - Optional transaction overrides (gas price, limit etc) | |
| * | |
| * @returns A Promise resolving to the transaction receipt after the payment is confirmed | |
| * | |
| * @throws {Error} If the ERC20RecurringPaymentProxy is not deployed on the specified network | |
| * @throws {Error} If the transaction fails (e.g. wrong index, expired permit, insufficient allowance) | |
| * | |
| * @remarks | |
| * • The function waits for the transaction to be mined before returning | |
| * • The signer must have been granted EXECUTOR_ROLE by the proxy admin | |
| * • Make sure all preconditions are met (allowance, balance, timing) before calling | |
| */ | |
| export async function executeRecurringPayment({ | |
| permitTuple, | |
| permitSignature, | |
| paymentIndex, | |
| paymentReference, | |
| signer, | |
| network, | |
| }: { | |
| permitTuple: PaymentTypes.SchedulePermit; | |
| permitSignature: string; | |
| paymentIndex: number; | |
| paymentReference: string; | |
| signer: Signer; | |
| network: CurrencyTypes.EvmChainName; | |
| }): Promise<providers.TransactionReceipt> { | |
| const proxyAddress = getRecurringPaymentProxyAddress(network); | |
| const data = encodeRecurringPaymentExecution({ | |
| permitTuple, | |
| permitSignature, | |
| paymentIndex, | |
| paymentReference, | |
| network, | |
| provider: signer, | |
| }); | |
| const tx = await signer.sendTransaction({ | |
| to: proxyAddress, | |
| data, | |
| value: 0, | |
| }); | |
| return tx.wait(); | |
| } | |
| * @param overrides - Optional transaction overrides (gas price, limit etc) | |
| * | |
| * @returns A Promise resolving to the transaction receipt after the payment is confirmed | |
| * | |
| * @throws {Error} If the ERC20RecurringPaymentProxy is not deployed on the specified network | |
| * @throws {Error} If the transaction fails (e.g. wrong index, expired permit, insufficient allowance) | |
| * | |
| * @remarks | |
| * • The function waits for the transaction to be mined before returning | |
| * • The signer must have been granted EXECUTOR_ROLE by the proxy admin | |
| * • Make sure all preconditions are met (allowance, balance, timing) before calling | |
| */ | |
| export async function executeRecurringPayment({ | |
| permitTuple, | |
| permitSignature, | |
| paymentIndex, | |
| paymentReference, | |
| signer, | |
| network, | |
| overrides = {}, | |
| }: { | |
| permitTuple: PaymentTypes.SchedulePermit; | |
| permitSignature: string; | |
| paymentIndex: number; | |
| paymentReference: string; | |
| signer: Signer; | |
| network: CurrencyTypes.EvmChainName; | |
| overrides?: providers.TransactionRequest; | |
| }): Promise<providers.TransactionReceipt> { | |
| const proxyAddress = getRecurringPaymentProxyAddress(network); | |
| const data = encodeRecurringPaymentExecution({ | |
| permitTuple, | |
| permitSignature, | |
| paymentIndex, | |
| paymentReference, | |
| network, | |
| provider: signer, | |
| }); | |
| const tx = await signer.sendTransaction({ | |
| to: proxyAddress, | |
| data, | |
| value: 0, | |
| ...overrides, | |
| }); | |
| return tx.wait(); | |
| } |
🤖 Prompt for AI Agents
In packages/payment-processor/src/payment/erc20-recurring-payment-proxy.ts
around lines 239 to 284, the function executeRecurringPayment is missing the
transaction overrides parameter mentioned in the documentation. Add an optional
overrides parameter to the function signature and include it in the
sendTransaction call to allow customization of gas price, gas limit, or other
transaction settings as described.
Comment on lines
+105
to
+160
| /** | ||
| * Encodes the transaction data to decrease or revoke allowance for the ERC20RecurringPaymentProxy. | ||
| * Tries different revocation methods in order of preference: | ||
| * 1. decreaseAllowance (OpenZeppelin standard) | ||
| * 2. decreaseApproval (older OpenZeppelin) | ||
| * 3. approve(0) (ERC20 standard fallback) | ||
| * | ||
| * @param tokenAddress - The ERC20 token contract address | ||
| * @param amount - The amount to decrease the allowance by, as a BigNumberish value | ||
| * @param provider - Web3 provider or signer to interact with the blockchain | ||
| * @param network - The EVM chain name where the proxy is deployed | ||
| * | ||
| * @returns The encoded function data as a hex string, ready to be used in a transaction | ||
| * | ||
| * @throws {Error} If the ERC20RecurringPaymentProxy is not deployed on the specified network | ||
| * @throws {Error} If none of the decrease/revoke methods are available on the token contract | ||
| * | ||
| * @remarks | ||
| * • The function attempts multiple decrease methods to support different ERC20 implementations | ||
| * • If no decrease method is available, falls back to completely revoking the allowance with approve(0) | ||
| * • The proxy address is fetched from the artifact to ensure consistency across deployments | ||
| * • The returned bytes can be used as the `data` field in an ethereum transaction | ||
| */ | ||
| export function encodeRecurringPaymentAllowanceDecrease({ | ||
| tokenAddress, | ||
| amount, | ||
| provider, | ||
| network, | ||
| }: { | ||
| tokenAddress: string; | ||
| amount: BigNumberish; | ||
| provider: providers.Provider | Signer; | ||
| network: CurrencyTypes.EvmChainName; | ||
| }): string { | ||
| const erc20RecurringPaymentProxy = erc20RecurringPaymentProxyArtifact.connect(network, provider); | ||
|
|
||
| if (!erc20RecurringPaymentProxy.address) { | ||
| throw new Error(`ERC20RecurringPaymentProxy not found on ${network}`); | ||
| } | ||
|
|
||
| const paymentTokenContract = ERC20__factory.connect(tokenAddress, provider); | ||
|
|
||
| try { | ||
| // Try decreaseAllowance first (OpenZeppelin standard) | ||
| return paymentTokenContract.interface.encodeFunctionData('decreaseAllowance', [ | ||
| erc20RecurringPaymentProxy.address, | ||
| amount, | ||
| ]); | ||
| } catch { | ||
| // Fallback to approve(0) if neither decrease method is supported | ||
| return paymentTokenContract.interface.encodeFunctionData('approve', [ | ||
| erc20RecurringPaymentProxy.address, | ||
| 0, // Complete revocation | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementation doesn't match documentation for decrease methods.
Similar to the approval function, the documentation mentions trying decreaseApproval but the implementation skips it.
try {
// Try decreaseAllowance first (OpenZeppelin standard)
return paymentTokenContract.interface.encodeFunctionData('decreaseAllowance', [
erc20RecurringPaymentProxy.address,
amount,
]);
} catch {
- // Fallback to approve(0) if neither decrease method is supported
- return paymentTokenContract.interface.encodeFunctionData('approve', [
- erc20RecurringPaymentProxy.address,
- 0, // Complete revocation
- ]);
+ try {
+ // Try decreaseApproval (older OpenZeppelin)
+ return paymentTokenContract.interface.encodeFunctionData('decreaseApproval', [
+ erc20RecurringPaymentProxy.address,
+ amount,
+ ]);
+ } catch {
+ // Fallback to approve(0) if neither decrease method is supported
+ return paymentTokenContract.interface.encodeFunctionData('approve', [
+ erc20RecurringPaymentProxy.address,
+ 0, // Complete revocation
+ ]);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Encodes the transaction data to decrease or revoke allowance for the ERC20RecurringPaymentProxy. | |
| * Tries different revocation methods in order of preference: | |
| * 1. decreaseAllowance (OpenZeppelin standard) | |
| * 2. decreaseApproval (older OpenZeppelin) | |
| * 3. approve(0) (ERC20 standard fallback) | |
| * | |
| * @param tokenAddress - The ERC20 token contract address | |
| * @param amount - The amount to decrease the allowance by, as a BigNumberish value | |
| * @param provider - Web3 provider or signer to interact with the blockchain | |
| * @param network - The EVM chain name where the proxy is deployed | |
| * | |
| * @returns The encoded function data as a hex string, ready to be used in a transaction | |
| * | |
| * @throws {Error} If the ERC20RecurringPaymentProxy is not deployed on the specified network | |
| * @throws {Error} If none of the decrease/revoke methods are available on the token contract | |
| * | |
| * @remarks | |
| * • The function attempts multiple decrease methods to support different ERC20 implementations | |
| * • If no decrease method is available, falls back to completely revoking the allowance with approve(0) | |
| * • The proxy address is fetched from the artifact to ensure consistency across deployments | |
| * • The returned bytes can be used as the `data` field in an ethereum transaction | |
| */ | |
| export function encodeRecurringPaymentAllowanceDecrease({ | |
| tokenAddress, | |
| amount, | |
| provider, | |
| network, | |
| }: { | |
| tokenAddress: string; | |
| amount: BigNumberish; | |
| provider: providers.Provider | Signer; | |
| network: CurrencyTypes.EvmChainName; | |
| }): string { | |
| const erc20RecurringPaymentProxy = erc20RecurringPaymentProxyArtifact.connect(network, provider); | |
| if (!erc20RecurringPaymentProxy.address) { | |
| throw new Error(`ERC20RecurringPaymentProxy not found on ${network}`); | |
| } | |
| const paymentTokenContract = ERC20__factory.connect(tokenAddress, provider); | |
| try { | |
| // Try decreaseAllowance first (OpenZeppelin standard) | |
| return paymentTokenContract.interface.encodeFunctionData('decreaseAllowance', [ | |
| erc20RecurringPaymentProxy.address, | |
| amount, | |
| ]); | |
| } catch { | |
| // Fallback to approve(0) if neither decrease method is supported | |
| return paymentTokenContract.interface.encodeFunctionData('approve', [ | |
| erc20RecurringPaymentProxy.address, | |
| 0, // Complete revocation | |
| ]); | |
| } | |
| } | |
| export function encodeRecurringPaymentAllowanceDecrease({ | |
| tokenAddress, | |
| amount, | |
| provider, | |
| network, | |
| }: { | |
| tokenAddress: string; | |
| amount: BigNumberish; | |
| provider: providers.Provider | Signer; | |
| network: CurrencyTypes.EvmChainName; | |
| }): string { | |
| const erc20RecurringPaymentProxy = erc20RecurringPaymentProxyArtifact.connect(network, provider); | |
| if (!erc20RecurringPaymentProxy.address) { | |
| throw new Error(`ERC20RecurringPaymentProxy not found on ${network}`); | |
| } | |
| const paymentTokenContract = ERC20__factory.connect(tokenAddress, provider); | |
| try { | |
| // Try decreaseAllowance first (OpenZeppelin standard) | |
| return paymentTokenContract.interface.encodeFunctionData('decreaseAllowance', [ | |
| erc20RecurringPaymentProxy.address, | |
| amount, | |
| ]); | |
| } catch { | |
| try { | |
| // Try decreaseApproval (older OpenZeppelin) | |
| return paymentTokenContract.interface.encodeFunctionData('decreaseApproval', [ | |
| erc20RecurringPaymentProxy.address, | |
| amount, | |
| ]); | |
| } catch { | |
| // Fallback to approve(0) if neither decrease method is supported | |
| return paymentTokenContract.interface.encodeFunctionData('approve', [ | |
| erc20RecurringPaymentProxy.address, | |
| 0, // Complete revocation | |
| ]); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/payment-processor/src/payment/erc20-recurring-payment-proxy.ts
between lines 105 and 160, the implementation of
encodeRecurringPaymentAllowanceDecrease only tries the decreaseAllowance method
and then falls back to approve(0), but the documentation states it should also
try decreaseApproval as a fallback method before approve(0). To fix this, update
the function to first try decreaseAllowance, then catch and try
decreaseApproval, and only if both fail, fallback to approve(0). This ensures
the implementation matches the documented behavior of trying all three methods
in order.
Comment on lines
+49
to
+103
| /** | ||
| * Encodes the transaction data to approve or increase allowance for the ERC20RecurringPaymentProxy. | ||
| * Tries different approval methods in order of preference: | ||
| * 1. increaseAllowance (OpenZeppelin standard) | ||
| * 2. increaseApproval (older OpenZeppelin) | ||
| * 3. approve (ERC20 standard fallback) | ||
| * | ||
| * @param tokenAddress - The ERC20 token contract address | ||
| * @param amount - The amount to approve, as a BigNumberish value | ||
| * @param provider - Web3 provider or signer to interact with the blockchain | ||
| * @param network - The EVM chain name where the proxy is deployed | ||
| * | ||
| * @returns The encoded function data as a hex string, ready to be used in a transaction | ||
| * | ||
| * @throws {Error} If the ERC20RecurringPaymentProxy is not deployed on the specified network | ||
| * @throws {Error} If none of the approval methods are available on the token contract | ||
| * | ||
| * @remarks | ||
| * • The function attempts multiple approval methods to support different ERC20 implementations | ||
| * • The proxy address is fetched from the artifact to ensure consistency across deployments | ||
| * • The returned bytes can be used as the `data` field in an ethereum transaction | ||
| */ | ||
| export function encodeRecurringPaymentApproval({ | ||
| tokenAddress, | ||
| amount, | ||
| provider, | ||
| network, | ||
| }: { | ||
| tokenAddress: string; | ||
| amount: BigNumberish; | ||
| provider: providers.Provider | Signer; | ||
| network: CurrencyTypes.EvmChainName; | ||
| }): string { | ||
| const erc20RecurringPaymentProxy = erc20RecurringPaymentProxyArtifact.connect(network, provider); | ||
|
|
||
| if (!erc20RecurringPaymentProxy.address) { | ||
| throw new Error(`ERC20RecurringPaymentProxy not found on ${network}`); | ||
| } | ||
|
|
||
| const paymentTokenContract = ERC20__factory.connect(tokenAddress, provider); | ||
|
|
||
| try { | ||
| // Try increaseAllowance first (OpenZeppelin standard) | ||
| return paymentTokenContract.interface.encodeFunctionData('increaseAllowance', [ | ||
| erc20RecurringPaymentProxy.address, | ||
| amount, | ||
| ]); | ||
| } catch { | ||
| // Fallback to approve if neither increase method is supported | ||
| return paymentTokenContract.interface.encodeFunctionData('approve', [ | ||
| erc20RecurringPaymentProxy.address, | ||
| amount, | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementation doesn't match documentation for approval methods.
The documentation mentions trying increaseApproval (older OpenZeppelin) as the second method, but the implementation skips it and goes directly from increaseAllowance to approve.
try {
// Try increaseAllowance first (OpenZeppelin standard)
return paymentTokenContract.interface.encodeFunctionData('increaseAllowance', [
erc20RecurringPaymentProxy.address,
amount,
]);
} catch {
- // Fallback to approve if neither increase method is supported
- return paymentTokenContract.interface.encodeFunctionData('approve', [
- erc20RecurringPaymentProxy.address,
- amount,
- ]);
+ try {
+ // Try increaseApproval (older OpenZeppelin)
+ return paymentTokenContract.interface.encodeFunctionData('increaseApproval', [
+ erc20RecurringPaymentProxy.address,
+ amount,
+ ]);
+ } catch {
+ // Fallback to approve if neither increase method is supported
+ return paymentTokenContract.interface.encodeFunctionData('approve', [
+ erc20RecurringPaymentProxy.address,
+ amount,
+ ]);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Encodes the transaction data to approve or increase allowance for the ERC20RecurringPaymentProxy. | |
| * Tries different approval methods in order of preference: | |
| * 1. increaseAllowance (OpenZeppelin standard) | |
| * 2. increaseApproval (older OpenZeppelin) | |
| * 3. approve (ERC20 standard fallback) | |
| * | |
| * @param tokenAddress - The ERC20 token contract address | |
| * @param amount - The amount to approve, as a BigNumberish value | |
| * @param provider - Web3 provider or signer to interact with the blockchain | |
| * @param network - The EVM chain name where the proxy is deployed | |
| * | |
| * @returns The encoded function data as a hex string, ready to be used in a transaction | |
| * | |
| * @throws {Error} If the ERC20RecurringPaymentProxy is not deployed on the specified network | |
| * @throws {Error} If none of the approval methods are available on the token contract | |
| * | |
| * @remarks | |
| * • The function attempts multiple approval methods to support different ERC20 implementations | |
| * • The proxy address is fetched from the artifact to ensure consistency across deployments | |
| * • The returned bytes can be used as the `data` field in an ethereum transaction | |
| */ | |
| export function encodeRecurringPaymentApproval({ | |
| tokenAddress, | |
| amount, | |
| provider, | |
| network, | |
| }: { | |
| tokenAddress: string; | |
| amount: BigNumberish; | |
| provider: providers.Provider | Signer; | |
| network: CurrencyTypes.EvmChainName; | |
| }): string { | |
| const erc20RecurringPaymentProxy = erc20RecurringPaymentProxyArtifact.connect(network, provider); | |
| if (!erc20RecurringPaymentProxy.address) { | |
| throw new Error(`ERC20RecurringPaymentProxy not found on ${network}`); | |
| } | |
| const paymentTokenContract = ERC20__factory.connect(tokenAddress, provider); | |
| try { | |
| // Try increaseAllowance first (OpenZeppelin standard) | |
| return paymentTokenContract.interface.encodeFunctionData('increaseAllowance', [ | |
| erc20RecurringPaymentProxy.address, | |
| amount, | |
| ]); | |
| } catch { | |
| // Fallback to approve if neither increase method is supported | |
| return paymentTokenContract.interface.encodeFunctionData('approve', [ | |
| erc20RecurringPaymentProxy.address, | |
| amount, | |
| ]); | |
| } | |
| } | |
| export function encodeRecurringPaymentApproval({ | |
| tokenAddress, | |
| amount, | |
| provider, | |
| network, | |
| }: { | |
| tokenAddress: string; | |
| amount: BigNumberish; | |
| provider: providers.Provider | Signer; | |
| network: CurrencyTypes.EvmChainName; | |
| }): string { | |
| const erc20RecurringPaymentProxy = erc20RecurringPaymentProxyArtifact.connect(network, provider); | |
| if (!erc20RecurringPaymentProxy.address) { | |
| throw new Error(`ERC20RecurringPaymentProxy not found on ${network}`); | |
| } | |
| const paymentTokenContract = ERC20__factory.connect(tokenAddress, provider); | |
| try { | |
| // Try increaseAllowance first (OpenZeppelin standard) | |
| return paymentTokenContract.interface.encodeFunctionData('increaseAllowance', [ | |
| erc20RecurringPaymentProxy.address, | |
| amount, | |
| ]); | |
| } catch { | |
| try { | |
| // Try increaseApproval (older OpenZeppelin) | |
| return paymentTokenContract.interface.encodeFunctionData('increaseApproval', [ | |
| erc20RecurringPaymentProxy.address, | |
| amount, | |
| ]); | |
| } catch { | |
| // Fallback to approve if neither increase method is supported | |
| return paymentTokenContract.interface.encodeFunctionData('approve', [ | |
| erc20RecurringPaymentProxy.address, | |
| amount, | |
| ]); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/payment-processor/src/payment/erc20-recurring-payment-proxy.ts
between lines 49 and 103, the implementation tries only two approval methods:
increaseAllowance and approve, but the documentation states it should try three
methods in order: increaseAllowance, increaseApproval, then approve. To fix
this, add a try-catch block to attempt encoding increaseApproval after failing
increaseAllowance and before falling back to approve, ensuring the code matches
the documented sequence of approval methods.
…d streamline test suite for clarity
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🔄 Add ERC20 Recurring Payment Proxy Support
📝 Description
Introduces a new proxy contract and supporting infrastructure for handling recurring ERC20 payments in the Request Network ecosystem.
🔍 Key Changes
🧪 Testing
🔒 Security Considerations
Summary by CodeRabbit
New Features
Tests
Chores