feat: Develop SingleRequestProxy Smart Contracts
#1453
Conversation
WalkthroughThe changes introduce the Changes
Assessment against linked issues
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
aimensahnoun
changed the title
SingleRequestProxy Smart ContractsSingleRequestProxy Smart Contracts
…equestproxy-erc20singlerequestproxy-singlerequestproxyfactory
github-actions
bot
requested review from
alexandre-abrioux,
kevindavee,
MantisClone and
sstefdev
…rc20singlerequestproxy-singlerequestproxyfactory' of github.com:RequestNetwork/requestNetwork into 1394-develop-smart-contracts-nativesinglerequestproxy-erc20singlerequestproxy-singlerequestproxyfactory
…in create2 script
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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`.
packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts
Outdated
Show resolved
Hide resolved
| const totalAmount = BigNumber.from(paymentAmount).add(BigNumber.from(feeAmount)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify totalAmount calculation by removing unnecessary BigNumber.from wrappers
Since paymentAmount and feeAmount are already instances of BigNumber, it's unnecessary to wrap them again with BigNumber.from(). You can simplify the calculation by using the add method directly on paymentAmount.
Apply this diff to streamline the calculation:
-const totalAmount = BigNumber.from(paymentAmount).add(BigNumber.from(feeAmount));
+const totalAmount = paymentAmount.add(feeAmount);📝 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.
| const totalAmount = BigNumber.from(paymentAmount).add(BigNumber.from(feeAmount)); | |
| const totalAmount = paymentAmount.add(feeAmount); |
There was a problem hiding this comment.
@aimensahnoun Is this true? If so, please make the change.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol (1 hunks)
- packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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`.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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`.packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol (5)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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`.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.
🪛 Biome
packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts
[error] 22-22: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 23-23: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 24-24: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
🔇 Additional comments (1)
packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts (1)
154-198: LGTM: Comprehensive test for non-standard ERC20 tokensThe test case for handling non-standard ERC20 tokens like USDT is well-implemented. It ensures compatibility with tokens that have different behaviors, verifying that the contract processes payments correctly.
| expect(await erc20SingleRequestProxy.erc20FeeProxy()).to.equal(erc20FeeProxy.address); | ||
| }); | ||
|
|
||
| it('should process a payment correctly via recieve', async () => { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Typo in test description: 'recieve' should be 'receive'
The test description on line 79 contains a typo. It should read 'should process a payment correctly via receive'.
There was a problem hiding this comment.
This is true 👍 There's a typo - receive is mispelled.
There was a problem hiding this comment.
@MantisClone, glad we caught that typo! 😄
(_/)
(='.'=)
(")(")
🧠 Learnings used
Learnt from: aimensahnoun
PR: RequestNetwork/requestNetwork#1453
File: packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts:0-0
Timestamp: 2024-10-17T16:01:47.326Z
Learning: The `rescueFunds` function in `EthereumSingleRequestProxy` does not emit any events.
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol (1 hunks)
- packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol (1 hunks)
- packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts (1 hunks)
- packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts (1 hunks)
🧰 Additional context used
📓 Learnings (5)
📓 Common learnings
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1453 File: packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts:0-0 Timestamp: 2024-10-17T18:33:41.124Z Learning: In the `EthereumSingleRequestProxy` contract, the custom reentrancy guard is nearly identical to OpenZeppelin's ReentrancyGuard, except that it allows the first reentrancy call from the `ERC20FeeProxy`. We assume it works correctly without needing further tests.Learnt from: MantisClone 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#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1453 File: packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol:19-23 Timestamp: 2024-10-05T14:43:07.965Z Learning: In the `EthereumSingleRequestProxy` contract, a custom reentrancy guard is used instead of OpenZeppelin's `ReentrancyGuard`, with minor edits to allow reentrancy by the `ethereumFeeProxy`.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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`.packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol (5)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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`.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol (7)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1453 File: packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol:19-23 Timestamp: 2024-10-05T14:43:07.965Z Learning: In the `EthereumSingleRequestProxy` contract, a custom reentrancy guard is used instead of OpenZeppelin's `ReentrancyGuard`, with minor edits to allow reentrancy by the `ethereumFeeProxy`.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1453 File: packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts:0-0 Timestamp: 2024-10-17T18:33:41.124Z Learning: In the `EthereumSingleRequestProxy` contract, the custom reentrancy guard is nearly identical to OpenZeppelin's ReentrancyGuard, except that it allows the first reentrancy call from the `ERC20FeeProxy`. We assume it works correctly without needing further tests.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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`.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#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.packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.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#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.packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts (3)
Learnt from: aimensahnoun PR: RequestNetwork/requestNetwork#1453 File: packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts:0-0 Timestamp: 2024-10-17T16:01:47.326Z Learning: The `rescueFunds` function in `EthereumSingleRequestProxy` does not emit any events.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#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.
🪛 Biome
packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts
[error] 22-22: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 23-23: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 24-24: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
🔇 Additional comments (10)
packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol (1)
55-55:⚠️ Potential issueCorrect the Use of
SafeERC20.safeApproveAt line 55,
SafeERC20.safeApprovefrom OpenZeppelin's library does not return a boolean value but reverts on failure. Assigning its return value to a boolean and usingrequireis unnecessary and incorrect.Apply this diff to fix the issue:
- require(SafeERC20.safeApprove(token, address(erc20FeeProxy), balance), 'Approval failed'); + SafeERC20.safeApprove(token, address(erc20FeeProxy), balance);Likely invalid or redundant comment.
packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol (8)
1-7: File header and imports are correctly defined.The SPDX license identifier, pragma version, and import statements are appropriate and correctly specified.
9-11: Contract documentation is clear and informative.The contract title and notice provide a clear description of the contract's purpose.
13-17: State variables are appropriately declared and visible.The public state variables are necessary for external access, aligning with the contract's design and retrieved learnings.
19-29: Custom reentrancy guard variables are correctly implemented.The custom reentrancy guard allows reentrancy from
ethereumFeeProxywhile preventing other forms of reentrancy, as intended. This aligns with the retrieved learnings regarding the necessity of a custom guard.
30-43: Constructor initializes all state variables properly.All constructor parameters are correctly assigned to the state variables, ensuring the contract is properly initialized.
45-58: CustomnonReentrantmodifier is correctly implemented.The
nonReentrantmodifier accurately implements the intended reentrancy protection, allowing controlled callbacks fromethereumFeeProxywhile safeguarding against attacks.
60-87:receivefunction logic is correctly implemented.The function correctly handles incoming Ether, managing funds based on the sender. It ensures proper forwarding of funds and maintains state integrity with appropriate reentrancy protection.
93-99:rescueNativeFundsfunction is correctly implemented.The function appropriately allows anyone to rescue trapped Ether by sending it to the payee, ensuring no funds remain stuck in the contract.
packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts (1)
1-168: Overall, the test suite is comprehensive and well-structuredThe test cases effectively cover various scenarios, ensuring that the
EthereumSingleRequestProxycontract functions as expected. Good job on implementing thorough unit tests.
| function rescueNativeFunds() external { | ||
| uint256 balance = address(this).balance; | ||
| require(balance > 0, 'No funds to rescue'); | ||
|
|
||
| (bool success, ) = payable(payee).call{value: balance}(''); | ||
| require(success, 'Rescue failed'); | ||
| } |
There was a problem hiding this comment.
Add Reentrancy Guard to rescueNativeFunds Function
The rescueNativeFunds() function allows anyone to transfer Ether held by the contract to the payee. Without reentrancy protection, this function may be vulnerable to reentrancy attacks.
Add a reentrancy guard to this function to enhance security.
Apply this diff to add the guard:
function rescueNativeFunds() external {
+ require(!locked, "Reentrant call detected");
+ locked = true;
uint256 balance = address(this).balance;
require(balance > 0, 'No funds to rescue');
(bool success, ) = payable(payee).call{value: balance}('');
require(success, 'Rescue failed');
+ locked = false;
}Committable suggestion was skipped due to low confidence.
| function _processPayment() internal { | ||
| IERC20 token = IERC20(tokenAddress); | ||
| uint256 balance = token.balanceOf(address(this)); | ||
| uint256 paymentAmount = balance; | ||
| if (feeAmount > 0 && feeAddress != address(0)) { | ||
| require(balance > feeAmount, 'Insufficient balance to cover fee'); | ||
| paymentAmount = balance - feeAmount; | ||
| } | ||
|
|
||
| require(SafeERC20.safeApprove(token, address(erc20FeeProxy), balance), 'Approval failed'); | ||
|
|
||
| erc20FeeProxy.transferFromWithReferenceAndFee( | ||
| tokenAddress, | ||
| payee, | ||
| paymentAmount, | ||
| paymentReference, | ||
| feeAmount, | ||
| feeAddress | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add Reentrancy Guard to _processPayment Function
The _processPayment() function involves external calls and token transfers without any reentrancy protection. This could expose the contract to reentrancy attacks, especially since it interacts with external contracts.
Consider implementing a reentrancy guard to secure this function. You can use a custom reentrancy guard similar to the one used in the EthereumSingleRequestProxy contract, which allows reentrancy from trusted contracts if necessary.
Apply this diff to add a reentrancy guard:
+ bool private locked;
function _processPayment() internal {
+ require(!locked, "Reentrant call detected");
+ locked = true;
IERC20 token = IERC20(tokenAddress);
uint256 balance = token.balanceOf(address(this));
uint256 paymentAmount = balance;
if (feeAmount > 0 && feeAddress != address(0)) {
require(balance > feeAmount, 'Insufficient balance to cover fee');
paymentAmount = balance - feeAmount;
}
SafeERC20.safeApprove(token, address(erc20FeeProxy), balance);
erc20FeeProxy.transferFromWithReferenceAndFee(
tokenAddress,
payee,
paymentAmount,
paymentReference,
feeAmount,
feeAddress
);
+ locked = false;
}Committable suggestion was skipped due to low confidence.
| function rescueERC20Funds(address _tokenAddress) external { | ||
| require(_tokenAddress != address(0), 'Invalid token address'); | ||
| IERC20 token = IERC20(_tokenAddress); | ||
| uint256 balance = token.balanceOf(address(this)); | ||
| require(balance > 0, 'No funds to rescue'); | ||
| bool success = SafeERC20.safeTransfer(token, payee, balance); | ||
| require(success, 'ERC20 rescue failed'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add Reentrancy Guard to rescueERC20Funds Function
The rescueERC20Funds() function can be called by anyone and performs an external token transfer. Without reentrancy protection, this function could be exploited through reentrancy attacks.
Implement a reentrancy guard to secure this function. You can reuse the same guard applied in _processPayment().
Apply this diff to add the guard:
function rescueERC20Funds(address _tokenAddress) external {
+ require(!locked, "Reentrant call detected");
+ locked = true;
require(_tokenAddress != address(0), 'Invalid token address');
IERC20 token = IERC20(_tokenAddress);
uint256 balance = token.balanceOf(address(this));
require(balance > 0, 'No funds to rescue');
SafeERC20.safeTransfer(token, payee, balance);
+ locked = false;
}Committable suggestion was skipped due to low confidence.
| function rescueERC20Funds(address _tokenAddress) external nonReentrant { | ||
| require(_tokenAddress != address(0), 'Invalid token address'); | ||
| IERC20 token = IERC20(_tokenAddress); | ||
| uint256 balance = token.balanceOf(address(this)); | ||
| require(balance > 0, 'No funds to rescue'); | ||
| bool success = SafeERC20.safeTransfer(token, payee, balance); | ||
| require(success, 'Rescue failed'); | ||
| } |
There was a problem hiding this comment.
Fix incorrect usage of SafeERC20.safeTransfer; it does not return a boolean.
The safeTransfer function from SafeERC20 does not return a boolean value; it reverts upon failure. Assigning its result to bool success will cause a compilation error. The subsequent require statement is unnecessary as any failure will already revert the transaction.
Apply this diff to fix the code:
- bool success = SafeERC20.safeTransfer(token, payee, balance);
- require(success, 'Rescue failed');
+ SafeERC20.safeTransfer(token, payee, balance);📝 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.
| function rescueERC20Funds(address _tokenAddress) external nonReentrant { | |
| require(_tokenAddress != address(0), 'Invalid token address'); | |
| IERC20 token = IERC20(_tokenAddress); | |
| uint256 balance = token.balanceOf(address(this)); | |
| require(balance > 0, 'No funds to rescue'); | |
| bool success = SafeERC20.safeTransfer(token, payee, balance); | |
| require(success, 'Rescue failed'); | |
| } | |
| function rescueERC20Funds(address _tokenAddress) external nonReentrant { | |
| require(_tokenAddress != address(0), 'Invalid token address'); | |
| IERC20 token = IERC20(_tokenAddress); | |
| uint256 balance = token.balanceOf(address(this)); | |
| require(balance > 0, 'No funds to rescue'); | |
| SafeERC20.safeTransfer(token, payee, balance); | |
| } |
| await ethereumSingleRequestProxy.rescueERC20Funds(testToken.address); | ||
|
|
||
| const contractBalanceAfter = await testToken.balanceOf(ethereumSingleRequestProxy.address); | ||
| ethereumSingleRequestProxy.address, expect(contractBalanceAfter).to.equal(0); |
There was a problem hiding this comment.
Fix syntax error in assertion
There is a syntax error at line 163 where ethereumSingleRequestProxy.address, is incorrectly placed before the expect statement. This may lead to a compilation error.
Apply this diff to correct the syntax:
-ethereumSingleRequestProxy.address, expect(contractBalanceAfter).to.equal(0);
+expect(contractBalanceAfter).to.equal(0);📝 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.
| ethereumSingleRequestProxy.address, expect(contractBalanceAfter).to.equal(0); | |
| expect(contractBalanceAfter).to.equal(0); |
| }); | ||
|
|
||
| it('should rescue ERC20 funds', async () => { | ||
| const rescueAmount = BN.from(100).mul(18); |
There was a problem hiding this comment.
Correct token amount calculation for rescueAmount
The calculation of rescueAmount might not represent the intended token amount correctly. Multiplying by 18 does not account for the token's decimal places properly. If the token has 18 decimals, consider using ethers.utils.parseUnits('100', 18) to accurately represent 100 tokens.
Apply this diff to fix the calculation:
-const rescueAmount = BN.from(100).mul(18);
+const rescueAmount = ethers.utils.parseUnits('100', 18);📝 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.
| const rescueAmount = BN.from(100).mul(18); | |
| const rescueAmount = ethers.utils.parseUnits('100', 18); |
| it('should process a payment correctly via receive', async () => { | ||
| const paymentAmount = BN.from(100).mul(BASE_DECIMAL); | ||
| const totalAmount = paymentAmount.add(feeAmount); | ||
|
|
||
| await testToken.connect(user1).transfer(erc20SingleRequestProxy.address, totalAmount); | ||
|
|
||
| const erc20SingleRequestProxyBalanceBefore = await testToken.balanceOf( | ||
| erc20SingleRequestProxy.address, | ||
| ); | ||
| expect(erc20SingleRequestProxyBalanceBefore).to.equal(totalAmount); | ||
|
|
||
| await expect( | ||
| user1.sendTransaction({ | ||
| to: erc20SingleRequestProxy.address, | ||
| value: 0, | ||
| }), | ||
| ) | ||
| .to.emit(erc20FeeProxy, 'TransferWithReferenceAndFee') | ||
| .withArgs( | ||
| testToken.address, | ||
| user2Addr, | ||
| paymentAmount, | ||
| paymentReference, | ||
| feeAmount, | ||
| feeRecipientAddr, | ||
| ); | ||
|
|
||
| const erc20SingleRequestProxyBalanceAfter = await testToken.balanceOf( | ||
| erc20SingleRequestProxy.address, | ||
| ); | ||
| const user2BalanceAfter = await testToken.balanceOf(user2Addr); | ||
| const feeRecipientBalanceAfter = await testToken.balanceOf(feeRecipientAddr); | ||
|
|
||
| expect(erc20SingleRequestProxyBalanceAfter).to.equal(0); | ||
| expect(user2BalanceAfter).to.equal(paymentAmount); | ||
| expect(feeRecipientBalanceAfter).to.equal(feeAmount); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Refactoring Repetitive Test Logic into Helper Functions
The test cases for processing payments ('should process a payment correctly via receive', 'should process a payment correctly via triggerERC20Payment', and 'should process a payment with a non-standard ERC20') contain similar code segments for:
- Transferring tokens to the proxy contract.
- Verifying contract balances before the payment.
- Triggering the payment and asserting emitted events.
- Checking balances after the payment.
To enhance maintainability and reduce code duplication, consider refactoring this repeated logic into reusable helper functions or hooks. This will make the tests cleaner and make it easier to update shared logic in the future.
Also applies to: 117-148, 154-198
| it('should rescue native funds', async () => { | ||
| const paymentAmount = ethers.utils.parseEther('1'); | ||
| const totalAmount = paymentAmount.add(feeAmount); | ||
|
|
||
| const ForceSendFactory = await ethers.getContractFactory('ForceSend'); | ||
| const forceSend = await ForceSendFactory.deploy(); | ||
| await forceSend.deployed(); | ||
|
|
||
| await forceSend.forceSend(erc20SingleRequestProxy.address, { value: totalAmount }); | ||
|
|
||
| const contractBalanceBefore = await ethers.provider.getBalance(erc20SingleRequestProxy.address); | ||
| expect(contractBalanceBefore).to.gt(0); | ||
|
|
||
| await erc20SingleRequestProxy.rescueNativeFunds(); | ||
|
|
||
| const contractBalanceAfter = await ethers.provider.getBalance(erc20SingleRequestProxy.address); | ||
| expect(contractBalanceAfter).to.equal(0); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add Comments Explaining the Use of the ForceSend Contract
In the test case 'should rescue native funds', you're deploying and using a ForceSend contract to send Ether to the erc20SingleRequestProxy contract, which doesn't typically accept Ether. Adding a comment explaining the purpose of the ForceSend contract will improve code readability and help other developers understand that this setup is used to simulate forcibly sending Ether to a contract that doesn't accept direct transfers (e.g., via selfdestruct).
| it('should handle zero fee amount correctly', async () => { | ||
| const zeroFeeProxy = await new ERC20SingleRequestProxy__factory(deployer).deploy( | ||
| user2Addr, | ||
| testToken.address, | ||
| feeRecipientAddr, | ||
| 0, | ||
| paymentReference, | ||
| erc20FeeProxy.address, | ||
| ); | ||
|
|
||
| const paymentAmount = BN.from(100).mul(BASE_DECIMAL); | ||
| await testToken.connect(user1).transfer(zeroFeeProxy.address, paymentAmount); | ||
|
|
||
| await expect( | ||
| user1.sendTransaction({ | ||
| to: zeroFeeProxy.address, | ||
| value: 0, | ||
| }), | ||
| ) | ||
| .to.emit(erc20FeeProxy, 'TransferWithReferenceAndFee') | ||
| .withArgs( | ||
| testToken.address, | ||
| user2Addr, | ||
| paymentAmount, | ||
| ethers.utils.keccak256(paymentReference), | ||
| 0, | ||
| feeRecipientAddr, | ||
| ); | ||
|
|
||
| expect(await testToken.balanceOf(zeroFeeProxy.address)).to.equal(0); | ||
| expect(await testToken.balanceOf(user2Addr)).to.equal(paymentAmount); | ||
| expect(await testToken.balanceOf(feeRecipientAddr)).to.equal(0); | ||
| }); |
There was a problem hiding this comment.
Ensure Consistent Usage of paymentReference in Event Assertions
In the test 'should handle zero fee amount correctly', you are using ethers.utils.keccak256(paymentReference) when asserting the emitted event (line 233):
.withArgs(
testToken.address,
user2Addr,
paymentAmount,
ethers.utils.keccak256(paymentReference),
0,
feeRecipientAddr,
);In other tests, such as 'should process a payment correctly via receive', the raw paymentReference is used directly without hashing. This inconsistency might lead to confusion or incorrect test validations.
Please verify whether the TransferWithReferenceAndFee event emits the hashed or raw paymentReference. If it emits the raw paymentReference, consider updating line 233 to use paymentReference directly for consistency. Alternatively, if the event emits the hashed paymentReference, update the other tests to hash the paymentReference as well.
| let user1: Signer, user1Addr: string; | ||
| let user2: Signer, user2Addr: string; | ||
| let feeRecipient: Signer, feeRecipientAddr: string; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Declare Variables Separately for Improved Readability
The variables user1, user1Addr, user2, user2Addr, feeRecipient, and feeRecipientAddr are declared using combined declarations:
let user1: Signer, user1Addr: string;
let user2: Signer, user2Addr: string;
let feeRecipient: Signer, feeRecipientAddr: string;For better readability and to adhere to standard coding practices, consider declaring each variable on a separate line:
let user1: Signer;
let user1Addr: string;
let user2: Signer;
let user2Addr: string;
let feeRecipient: Signer;
let feeRecipientAddr: string;This approach makes the code cleaner and facilitates easier modifications in the future.
🧰 Tools
🪛 Biome
[error] 22-22: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 23-23: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 24-24: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
…o 1394-develop-smart-contracts-nativesinglerequestproxy-erc20singlerequestproxy-singlerequestproxyfactory
aimensahnoun
deleted the
1394-develop-smart-contracts-nativesinglerequestproxy-erc20singlerequestproxy-singlerequestproxyfactory
branch
Resolves #1394
Summary by CodeRabbit
Release Notes
New Features
ERC20SingleRequestProxyfor streamlined ERC20 token transfers with fee management.EthereumSingleRequestProxyfor single payment requests with fee handling.Bug Fixes
Tests
ERC20SingleRequestProxyandEthereumSingleRequestProxyto validate functionality and ensure reliability.