feat: Develop SingleRequestProxy Smart Contracts
#1453
Conversation
WalkthroughThe changes introduce three new smart contracts: Changes
Assessment against linked 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
…arity in SingleRequestProxyFactory contract
…equestproxy-erc20singlerequestproxy-singlerequestproxyfactory
|
@coderabbitai full review |
Actions performedFull review 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: 13
Outside diff range comments (2)
packages/smart-contracts/scripts-create2/transfer-ownership.ts (1)
Incomplete implementation of required contracts
The contracts
NativeSingleRequestProxyandERC20SingleRequestProxyare not referenced intransfer-ownership.ts. WhileERC20SingleRequestProxyexists in test files, it hasn't been integrated into the main implementation or deployment scripts. Please ensure all contracts mentioned in the PR objectives are properly addressed and integrated.Analysis chain
Line range hint
1-30: Consider addressing all contracts mentioned in PR objectivesWhile the addition of
SingleRequestProxyFactoryis correct, the PR objectives also mentionNativeSingleRequestProxyandERC20SingleRequestProxy. These contracts are not addressed in this file. Additionally, the PR objectives indicate that SDK smart-contract scripts for deployment and administration are still pending.Let's check if these contracts are handled elsewhere in the codebase:
Please ensure all objectives are met before finalizing this PR.
Let's address the issues identified in the previous verification step by running updated shell scripts to ensure all required contracts are properly referenced.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for NativeSingleRequestProxy and ERC20SingleRequestProxy in the codebase echo "Searching for NativeSingleRequestProxy:" rg --type typescript "NativeSingleRequestProxy" echo "Searching for ERC20SingleRequestProxy:" rg --type typescript "ERC20SingleRequestProxy" echo "Checking for deployment scripts:" fd -e ts -e js deployLength of output: 1905
Script:
#!/bin/bash # Description: Search for NativeSingleRequestProxy and ERC20SingleRequestProxy in .ts and .js files echo "Searching for NativeSingleRequestProxy:" rg "NativeSingleRequestProxy" --glob "*.ts" --glob "*.js" echo "Searching for ERC20SingleRequestProxy:" rg "ERC20SingleRequestProxy" --glob "*.ts" --glob "*.js" echo "Checking for deployment scripts containing 'deploy':" fd "deploy" -e ts -e jsLength of output: 3195
packages/smart-contracts/scripts-create2/compute-one-address.ts (1)
Line range hint
1-85: Consider addressing remaining PR objectivesWhile the addition of the 'SingleRequestProxyFactory' case in this file is correct and aligns with the PR objectives, it's important to note that there are still outstanding tasks mentioned in the PR description.
Ensure that the
NativeSingleRequestProxyandERC20SingleRequestProxycontracts are also properly integrated into the deployment process if necessary.Remember to develop the SDK smart-contract scripts for deployment and administration as mentioned in the PR description. These scripts are crucial for the overall functionality and usability of the smart contracts within the Request Network ecosystem.
Consider updating the documentation to reflect these new changes and additions.
To track these remaining tasks, it might be helpful to:
- Create separate issues for each remaining task if they don't already exist.
- Update the main issue (Develop smart contracts: NativeSingleRequestProxy, ERC20SingleRequestProxy, SingleRequestProxyFactory #1394) to reflect the current progress and remaining work.
This will help ensure all aspects of the "Definition of Done" are addressed before the PR is finalized.
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (6)
- 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/transfer-ownership.ts (1 hunks)
- packages/smart-contracts/scripts-create2/utils.ts (1 hunks)
- packages/smart-contracts/src/contracts/SingleRequestProxyFactory.sol (1 hunks)
- packages/smart-contracts/test/contracts/SingleRequestProxyFactory.test.ts (1 hunks)
Additional comments not posted (4)
packages/smart-contracts/scripts-create2/transfer-ownership.ts (1)
15-16: LGTM! Addition of SingleRequestProxyFactory is consistent.The addition of
SingleRequestProxyFactoryto the switch statement is correct and aligns with the PR objectives. It follows the existing pattern for contract ownership transfer.Let's verify if
SingleRequestProxyFactoryis consistently handled in other parts of the codebase:packages/smart-contracts/scripts-create2/utils.ts (1)
23-23: LGTM: Addition of 'SingleRequestProxyFactory' to deployment listThe addition of 'SingleRequestProxyFactory' to the
create2ContractDeploymentListis consistent with the PR objectives and the existing structure of the array.Suggestion: Update
getArtifactfunctionTo ensure proper functionality, please add a case for 'SingleRequestProxyFactory' in the
getArtifactfunction.Apply this diff to update the
getArtifactfunction:export const getArtifact = (contract: string): artifacts.ContractArtifact<Contract> => { switch (contract) { // ... existing cases ... case 'ERC20TransferableReceivable': return artifacts.erc20TransferableReceivableArtifact; + case 'SingleRequestProxyFactory': + return artifacts.singleRequestProxyFactoryArtifact; default: throw new Error('Contract unknown'); } };Verify artifact import
Please ensure that the
singleRequestProxyFactoryArtifactis imported from the artifacts library.Run the following script to verify the artifact import:
packages/smart-contracts/scripts-create2/compute-one-address.ts (1)
67-68: LGTM: Addition of SingleRequestProxyFactory caseThe addition of the 'SingleRequestProxyFactory' case to the switch statement is correct and aligns with the PR objectives. The implementation follows the existing pattern for other contract types, which is good for consistency.
To ensure completeness, let's verify if any related files need updates due to this addition:
Please review the results of these checks to ensure all necessary updates have been made across the codebase.
Verification successful
Verified: Addition of SingleRequestProxyFactory case
The addition of the
'SingleRequestProxyFactory'case has been successfully verified across the codebase. All necessary imports, definitions, and type definitions are correctly updated, and no related TODOs or FIXMEs are present.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential places that might need updates due to the new SingleRequestProxyFactory # Test 1: Check if SingleRequestProxyFactory is properly imported/defined echo "Checking for SingleRequestProxyFactory definition or import:" rg --type typescript "SingleRequestProxyFactory" # Test 2: Check for any TODOs or FIXMEs related to SingleRequestProxyFactory echo "Checking for TODOs or FIXMEs related to SingleRequestProxyFactory:" rg --type typescript -i "TODO|FIXME" -C 3 | rg -i "SingleRequestProxyFactory" # Test 3: Check if SingleRequestProxyFactory is added to relevant type definitions or interfaces echo "Checking if SingleRequestProxyFactory is added to relevant type definitions:" rg --type typescript "type|interface" -A 10 | rg "SingleRequestProxyFactory"Length of output: 745
Script:
#!/bin/bash # Description: Check for potential places that might need updates due to the new SingleRequestProxyFactory # Test 1: Check if SingleRequestProxyFactory is properly imported/defined echo "Checking for SingleRequestProxyFactory definition or import:" rg --glob "*.ts" "SingleRequestProxyFactory" # Test 2: Check for any TODOs or FIXMEs related to SingleRequestProxyFactory echo "Checking for TODOs or FIXMEs related to SingleRequestProxyFactory:" rg --glob "*.ts" -i "TODO|FIXME" -C 3 | rg -i "SingleRequestProxyFactory" # Test 3: Check if SingleRequestProxyFactory is added to relevant type definitions or interfaces echo "Checking if SingleRequestProxyFactory is added to relevant type definitions:" rg --glob "*.ts" "type|interface" -A 10 | rg "SingleRequestProxyFactory"Length of output: 3734
packages/smart-contracts/scripts-create2/constructor-args.ts (1)
70-80: LGTM! Consider a minor improvement for consistency.The implementation for the
SingleRequestProxyFactorycase looks good and aligns with the PR objectives. It correctly retrieves the necessary proxy addresses based on the provided network.For consistency with other cases, consider extracting the error message to a constant at the top of the file. This would make it easier to maintain and update error messages across all cases. For example:
const NETWORK_REQUIRED_ERROR = (contract: string) => `${contract} requires network parameter to get correct proxy addresses`; // Then in the case: if (!network) { throw new Error(NETWORK_REQUIRED_ERROR('SingleRequestProxyFactory')); }To ensure the correct order of returned addresses, please confirm:
| _feeAddress, | ||
| _feeAmount | ||
| ); | ||
| emit EtheruemSingleRequestProxyCreated(address(proxy), _payee, _paymentReference); |
There was a problem hiding this comment.
Fix the typo in the event emission 'EtheruemSingleRequestProxyCreated'.
Update the event emission to use the corrected event name EthereumSingleRequestProxyCreated.
Apply this diff to correct the event emission:
-emit EtheruemSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
+emit EthereumSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);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.
| emit EtheruemSingleRequestProxyCreated(address(proxy), _payee, _paymentReference); | |
| emit EthereumSingleRequestProxyCreated(address(proxy), _payee, _paymentReference); |
| event EtheruemSingleRequestProxyCreated( | ||
| address indexed proxyAddress, | ||
| address indexed payee, | ||
| bytes indexed paymentReference | ||
| ); |
There was a problem hiding this comment.
Fix the typo in the event name 'EtheruemSingleRequestProxyCreated'.
The event name EtheruemSingleRequestProxyCreated has a typo. It should be EthereumSingleRequestProxyCreated to correctly spell "Ethereum".
Apply this diff to correct the event name:
-event EtheruemSingleRequestProxyCreated(
+event EthereumSingleRequestProxyCreated(
address indexed proxyAddress,
address indexed payee,
bytes indexed paymentReference
);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.
| event EtheruemSingleRequestProxyCreated( | |
| address indexed proxyAddress, | |
| address indexed payee, | |
| bytes indexed paymentReference | |
| ); | |
| event EthereumSingleRequestProxyCreated( | |
| address indexed proxyAddress, | |
| address indexed payee, | |
| bytes indexed paymentReference | |
| ); |
| constructor(address _ethereumFeeProxy, address _erc20FeeProxy) Ownable() { | ||
| ethereumFeeProxy = _ethereumFeeProxy; | ||
| erc20FeeProxy = _erc20FeeProxy; | ||
| } |
There was a problem hiding this comment.
Add validation for fee proxy addresses in the constructor.
To prevent misconfiguration, add checks to ensure _ethereumFeeProxy and _erc20FeeProxy are not zero addresses when initializing the contract.
Add require statements in the constructor:
constructor(address _ethereumFeeProxy, address _erc20FeeProxy) Ownable() {
+ require(_ethereumFeeProxy != address(0), "ethereumFeeProxy address cannot be zero");
+ require(_erc20FeeProxy != address(0), "erc20FeeProxy address cannot be zero");
ethereumFeeProxy = _ethereumFeeProxy;
erc20FeeProxy = _erc20FeeProxy;
}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.
| constructor(address _ethereumFeeProxy, address _erc20FeeProxy) Ownable() { | |
| ethereumFeeProxy = _ethereumFeeProxy; | |
| erc20FeeProxy = _erc20FeeProxy; | |
| } | |
| constructor(address _ethereumFeeProxy, address _erc20FeeProxy) Ownable() { | |
| require(_ethereumFeeProxy != address(0), "ethereumFeeProxy address cannot be zero"); | |
| require(_erc20FeeProxy != address(0), "erc20FeeProxy address cannot be zero"); | |
| ethereumFeeProxy = _ethereumFeeProxy; | |
| erc20FeeProxy = _erc20FeeProxy; | |
| } |
| function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner { | ||
| erc20FeeProxy = _newERC20FeeProxy; | ||
| } |
There was a problem hiding this comment.
Add validation for new ERC20FeeProxy address in 'setERC20FeeProxy'.
To prevent setting an invalid address, add a check to ensure _newERC20FeeProxy is not the zero address before updating erc20FeeProxy.
Implement the validation:
function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner {
+ require(_newERC20FeeProxy != address(0), "New ERC20FeeProxy address cannot be zero");
erc20FeeProxy = _newERC20FeeProxy;
}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 setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner { | |
| erc20FeeProxy = _newERC20FeeProxy; | |
| } | |
| function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner { | |
| require(_newERC20FeeProxy != address(0), "New ERC20FeeProxy address cannot be zero"); | |
| erc20FeeProxy = _newERC20FeeProxy; | |
| } |
Consider emitting an event when updating 'erc20FeeProxy'.
To enhance transparency and enable off-chain monitoring, emit an event when the erc20FeeProxy address is updated.
Define an event and emit it:
+ event ERC20FeeProxyUpdated(address indexed oldAddress, address indexed newAddress);
function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner {
require(_newERC20FeeProxy != address(0), "New ERC20FeeProxy address cannot be zero");
+ emit ERC20FeeProxyUpdated(erc20FeeProxy, _newERC20FeeProxy);
erc20FeeProxy = _newERC20FeeProxy;
}Committable suggestion was skipped due to low confidence.
| function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner { | ||
| ethereumFeeProxy = _newEthereumFeeProxy; | ||
| } |
There was a problem hiding this comment.
Add validation for new EthereumFeeProxy address in 'setEthereumFeeProxy'.
Ensure that _newEthereumFeeProxy is not the zero address before updating ethereumFeeProxy to avoid misconfiguration.
Implement the validation:
function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner {
+ require(_newEthereumFeeProxy != address(0), "New EthereumFeeProxy address cannot be zero");
ethereumFeeProxy = _newEthereumFeeProxy;
}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 setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner { | |
| ethereumFeeProxy = _newEthereumFeeProxy; | |
| } | |
| function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner { | |
| require(_newEthereumFeeProxy != address(0), "New EthereumFeeProxy address cannot be zero"); | |
| ethereumFeeProxy = _newEthereumFeeProxy; | |
| } |
Consider emitting an event when updating 'ethereumFeeProxy'.
To improve transparency and facilitate monitoring, emit an event when the ethereumFeeProxy address is updated.
Define an event and emit it:
+ event EthereumFeeProxyUpdated(address indexed oldAddress, address indexed newAddress);
function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner {
require(_newEthereumFeeProxy != address(0), "New EthereumFeeProxy address cannot be zero");
+ emit EthereumFeeProxyUpdated(ethereumFeeProxy, _newEthereumFeeProxy);
ethereumFeeProxy = _newEthereumFeeProxy;
}Committable suggestion was skipped due to low confidence.
| function createERC20SingleRequestProxy( | ||
| address _payee, | ||
| address _tokenAddress, | ||
| bytes memory _paymentReference, | ||
| address _feeAddress, | ||
| uint256 _feeAmount | ||
| ) external returns (address) { | ||
| ERC20SingleRequestProxy proxy = new ERC20SingleRequestProxy( | ||
| _payee, | ||
| _tokenAddress, | ||
| _feeAddress, | ||
| _feeAmount, | ||
| _paymentReference, | ||
| erc20FeeProxy | ||
| ); | ||
|
|
||
| emit ERC20SingleRequestProxyCreated(address(proxy), _payee, _tokenAddress, _paymentReference); | ||
| return address(proxy); | ||
| } |
There was a problem hiding this comment.
Add input validation for function parameters in 'createERC20SingleRequestProxy'.
To ensure robustness, add validations for critical input parameters. Ensure _payee, _tokenAddress, _feeAddress, and erc20FeeProxy are not zero addresses.
Include require statements at the start of the function:
function createERC20SingleRequestProxy(
address _payee,
address _tokenAddress,
bytes memory _paymentReference,
address _feeAddress,
uint256 _feeAmount
) external returns (address) {
+ require(_payee != address(0), "Payee address cannot be zero");
+ require(_tokenAddress != address(0), "Token address cannot be zero");
+ require(_feeAddress != address(0), "Fee address cannot be zero");
+ require(erc20FeeProxy != address(0), "erc20FeeProxy address not set");
ERC20SingleRequestProxy proxy = new ERC20SingleRequestProxy(
_payee,
_tokenAddress,
_feeAddress,
_feeAmount,
_paymentReference,
erc20FeeProxy
);
emit ERC20SingleRequestProxyCreated(address(proxy), _payee, _tokenAddress, _paymentReference);
return address(proxy);
}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 createERC20SingleRequestProxy( | |
| address _payee, | |
| address _tokenAddress, | |
| bytes memory _paymentReference, | |
| address _feeAddress, | |
| uint256 _feeAmount | |
| ) external returns (address) { | |
| ERC20SingleRequestProxy proxy = new ERC20SingleRequestProxy( | |
| _payee, | |
| _tokenAddress, | |
| _feeAddress, | |
| _feeAmount, | |
| _paymentReference, | |
| erc20FeeProxy | |
| ); | |
| emit ERC20SingleRequestProxyCreated(address(proxy), _payee, _tokenAddress, _paymentReference); | |
| return address(proxy); | |
| } | |
| function createERC20SingleRequestProxy( | |
| address _payee, | |
| address _tokenAddress, | |
| bytes memory _paymentReference, | |
| address _feeAddress, | |
| uint256 _feeAmount | |
| ) external returns (address) { | |
| require(_payee != address(0), "Payee address cannot be zero"); | |
| require(_tokenAddress != address(0), "Token address cannot be zero"); | |
| require(_feeAddress != address(0), "Fee address cannot be zero"); | |
| require(erc20FeeProxy != address(0), "erc20FeeProxy address not set"); | |
| ERC20SingleRequestProxy proxy = new ERC20SingleRequestProxy( | |
| _payee, | |
| _tokenAddress, | |
| _feeAddress, | |
| _feeAmount, | |
| _paymentReference, | |
| erc20FeeProxy | |
| ); | |
| emit ERC20SingleRequestProxyCreated(address(proxy), _payee, _tokenAddress, _paymentReference); | |
| return address(proxy); | |
| } |
| expect(receipt.events?.[0]?.event).to.equal('EtheruemSingleRequestProxyCreated'); | ||
|
|
There was a problem hiding this comment.
Correct the typo in the event name 'EtheruemSingleRequestProxyCreated'
The event name 'EtheruemSingleRequestProxyCreated' is misspelled in multiple places. It should be 'EthereumSingleRequestProxyCreated'. This typo may cause the event matching to fail in the tests.
Apply this diff to correct the typo:
- expect(receipt.events?.[0]?.event).to.equal('EtheruemSingleRequestProxyCreated');
+ expect(receipt.events?.[0]?.event).to.equal('EthereumSingleRequestProxyCreated');
...
- await expect(tx)
- .to.emit(singleRequestProxyFactory, 'EtheruemSingleRequestProxyCreated')
+ await expect(tx)
+ .to.emit(singleRequestProxyFactory, 'EthereumSingleRequestProxyCreated')Also applies to: 86-88
| it('should revert when called by non-owner', async () => { | ||
| const newERC20FeeProxy = await (await ethers.getContractFactory('ERC20FeeProxy')).deploy(); | ||
| await newERC20FeeProxy.deployed(); | ||
|
|
||
| await expect( | ||
| singleRequestProxyFactory.connect(user).setERC20FeeProxy(newERC20FeeProxy.address), | ||
| ).to.be.revertedWith('Ownable: caller is not the owner'); | ||
| }); |
There was a problem hiding this comment.
Clarify the test description for non-owner action
The test description 'should revert when called by non-owner' is too generic and doesn't specify which function call is expected to revert. Making the description more specific improves readability and maintainability.
Consider updating the test description to:
- it('should revert when called by non-owner', async () => {
+ it('should revert when non-owner tries to set ERC20FeeProxy address', async () => {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.
| it('should revert when called by non-owner', async () => { | |
| const newERC20FeeProxy = await (await ethers.getContractFactory('ERC20FeeProxy')).deploy(); | |
| await newERC20FeeProxy.deployed(); | |
| await expect( | |
| singleRequestProxyFactory.connect(user).setERC20FeeProxy(newERC20FeeProxy.address), | |
| ).to.be.revertedWith('Ownable: caller is not the owner'); | |
| }); | |
| it('should revert when non-owner tries to set ERC20FeeProxy address', async () => { | |
| const newERC20FeeProxy = await (await ethers.getContractFactory('ERC20FeeProxy')).deploy(); | |
| await newERC20FeeProxy.deployed(); | |
| await expect( | |
| singleRequestProxyFactory.connect(user).setERC20FeeProxy(newERC20FeeProxy.address), | |
| ).to.be.revertedWith('Ownable: caller is not the owner'); | |
| }); |
| it('should revert when called by non-owner', async () => { | ||
| const newEthereumFeeProxy = await ( | ||
| await ethers.getContractFactory('EthereumFeeProxy') | ||
| ).deploy(); | ||
| await newEthereumFeeProxy.deployed(); | ||
|
|
||
| await expect( | ||
| singleRequestProxyFactory.connect(user).setEthereumFeeProxy(newEthereumFeeProxy.address), | ||
| ).to.be.revertedWith('Ownable: caller is not the owner'); | ||
| }); |
There was a problem hiding this comment.
Clarify the test description for non-owner action
Following the previous issue regarding the nested it block, once it's moved outside, the test description 'should revert when called by non-owner' can be made more specific to indicate which function is being tested.
Update the test description to:
- it('should revert when called by non-owner', async () => {
+ it('should revert when non-owner tries to set EthereumFeeProxy address', async () => {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.
| it('should revert when called by non-owner', async () => { | |
| const newEthereumFeeProxy = await ( | |
| await ethers.getContractFactory('EthereumFeeProxy') | |
| ).deploy(); | |
| await newEthereumFeeProxy.deployed(); | |
| await expect( | |
| singleRequestProxyFactory.connect(user).setEthereumFeeProxy(newEthereumFeeProxy.address), | |
| ).to.be.revertedWith('Ownable: caller is not the owner'); | |
| }); | |
| it('should revert when non-owner tries to set EthereumFeeProxy address', async () => { | |
| const newEthereumFeeProxy = await ( | |
| await ethers.getContractFactory('EthereumFeeProxy') | |
| ).deploy(); | |
| await newEthereumFeeProxy.deployed(); | |
| await expect( | |
| singleRequestProxyFactory.connect(user).setEthereumFeeProxy(newEthereumFeeProxy.address), | |
| ).to.be.revertedWith('Ownable: caller is not the owner'); | |
| }); |
| it('should update EthereumFeeProxy address when called by owner', async () => { | ||
| const newEthereumFeeProxy = await ( | ||
| await ethers.getContractFactory('EthereumFeeProxy') | ||
| ).deploy(); | ||
| await newEthereumFeeProxy.deployed(); | ||
|
|
||
| await singleRequestProxyFactory.setEthereumFeeProxy(newEthereumFeeProxy.address); | ||
| expect(await singleRequestProxyFactory.ethereumFeeProxy()).to.equal( | ||
| newEthereumFeeProxy.address, | ||
| ); | ||
|
|
||
| it('should revert when called by non-owner', async () => { | ||
| const newEthereumFeeProxy = await ( | ||
| await ethers.getContractFactory('EthereumFeeProxy') | ||
| ).deploy(); | ||
| await newEthereumFeeProxy.deployed(); | ||
|
|
||
| await expect( | ||
| singleRequestProxyFactory.connect(user).setEthereumFeeProxy(newEthereumFeeProxy.address), | ||
| ).to.be.revertedWith('Ownable: caller is not the owner'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Fix the nested 'it' block
The it block starting at line 166 is nested within another it block, which is not supported by Mocha. Test cases (it blocks) should not be nested inside each other. This can lead to unexpected behavior or the test not running at all.
To fix this issue, move the nested it block outside of the parent it block:
it('should update EthereumFeeProxy address when called by owner', async () => {
// ...existing code...
expect(await singleRequestProxyFactory.ethereumFeeProxy()).to.equal(
newEthereumFeeProxy.address,
);
+ });
+
+ it('should revert when called by non-owner', async () => {
+ const newEthereumFeeProxy = await (
+ await ethers.getContractFactory('EthereumFeeProxy')
+ ).deploy();
+ await newEthereumFeeProxy.deployed();
+
+ await expect(
+ singleRequestProxyFactory.connect(user).setEthereumFeeProxy(newEthereumFeeProxy.address),
+ ).to.be.revertedWith('Ownable: caller is not the owner');
});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.
| it('should update EthereumFeeProxy address when called by owner', async () => { | |
| const newEthereumFeeProxy = await ( | |
| await ethers.getContractFactory('EthereumFeeProxy') | |
| ).deploy(); | |
| await newEthereumFeeProxy.deployed(); | |
| await singleRequestProxyFactory.setEthereumFeeProxy(newEthereumFeeProxy.address); | |
| expect(await singleRequestProxyFactory.ethereumFeeProxy()).to.equal( | |
| newEthereumFeeProxy.address, | |
| ); | |
| it('should revert when called by non-owner', async () => { | |
| const newEthereumFeeProxy = await ( | |
| await ethers.getContractFactory('EthereumFeeProxy') | |
| ).deploy(); | |
| await newEthereumFeeProxy.deployed(); | |
| await expect( | |
| singleRequestProxyFactory.connect(user).setEthereumFeeProxy(newEthereumFeeProxy.address), | |
| ).to.be.revertedWith('Ownable: caller is not the owner'); | |
| }); | |
| }); | |
| it('should update EthereumFeeProxy address when called by owner', async () => { | |
| const newEthereumFeeProxy = await ( | |
| await ethers.getContractFactory('EthereumFeeProxy') | |
| ).deploy(); | |
| await newEthereumFeeProxy.deployed(); | |
| await singleRequestProxyFactory.setEthereumFeeProxy(newEthereumFeeProxy.address); | |
| expect(await singleRequestProxyFactory.ethereumFeeProxy()).to.equal( | |
| newEthereumFeeProxy.address, | |
| ); | |
| }); | |
| it('should revert when called by non-owner', async () => { | |
| const newEthereumFeeProxy = await ( | |
| await ethers.getContractFactory('EthereumFeeProxy') | |
| ).deploy(); | |
| await newEthereumFeeProxy.deployed(); | |
| await expect( | |
| singleRequestProxyFactory.connect(user).setEthereumFeeProxy(newEthereumFeeProxy.address), | |
| ).to.be.revertedWith('Ownable: caller is not the owner'); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 27
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (12)
- 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/transfer-ownership.ts (1 hunks)
- packages/smart-contracts/scripts-create2/utils.ts (1 hunks)
- packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol (1 hunks)
- packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol (1 hunks)
- packages/smart-contracts/src/contracts/SingleRequestProxyFactory.sol (1 hunks)
- packages/smart-contracts/src/contracts/test/EthereumFeeProxyMock.sol (1 hunks)
- packages/smart-contracts/src/contracts/test/TestToken.sol (1 hunks)
- packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts (1 hunks)
- packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts (1 hunks)
- packages/smart-contracts/test/contracts/SingleRequestProxyFactory.test.ts (1 hunks)
Additional context used
Biome
packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts
[error] 19-19: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 20-20: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 21-21: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 23-25: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
Additional comments not posted (29)
packages/smart-contracts/src/contracts/test/TestToken.sol (2)
1-9: LGTM: Contract structure and inheritance are well-defined.The TestToken contract correctly inherits from ERC20, Ownable, and ERC20Permit. The use of OpenZeppelin contracts enhances security and follows best practices. The SPDX license identifier and pragma solidity version are appropriately specified.
1-15: Overall, the TestToken contract is well-implemented and secure.The contract effectively uses OpenZeppelin's ERC20, Ownable, and ERC20Permit contracts, following best practices for Solidity development. It provides basic ERC20 functionality with additional features like ownership control and permit capabilities, making it suitable for testing purposes.
While the current implementation is solid, consider the following enhancements for improved flexibility and observability:
- Parameterize the token name and symbol in the constructor.
- Add event emissions for important state changes, such as minting.
These improvements would make the contract more versatile and easier to integrate with external systems.
packages/smart-contracts/scripts-create2/transfer-ownership.ts (2)
Line range hint
1-30: Consider including other mentioned contractsWhile the addition of
SingleRequestProxyFactoryis correct, the PR objectives also mentioned the development ofNativeSingleRequestProxyandERC20SingleRequestProxy. These contracts are not reflected in the current changes to this file.To ensure all required contracts are properly handled, please run the following verification:
#!/bin/bash # Description: Check for the presence and handling of all mentioned contract types echo "Checking for NativeSingleRequestProxy:" rg --type typescript --type solidity 'NativeSingleRequestProxy' echo "Checking for ERC20SingleRequestProxy:" rg --type typescript --type solidity 'ERC20SingleRequestProxy' echo "Verifying if these contracts need ownership transfer:" rg --type typescript 'case .*(NativeSingleRequestProxy|ERC20SingleRequestProxy)'If these contracts require ownership transfer, consider adding them to the switch statement in this file. If they don't require ownership transfer or are handled differently, please clarify in the PR description or comments.
15-16: LGTM! Verify consistency with project objectives.The addition of
SingleRequestProxyFactoryto the switch statement is consistent with the existing pattern for contract types that require ownership transfer. This change correctly applies theupdateOwnerfunction to the new contract type.To ensure this change aligns with the overall project structure and requirements, please run the following verification:
This script will help confirm that the
SingleRequestProxyFactorycontract is properly integrated into the project structure.packages/smart-contracts/scripts-create2/utils.ts (1)
Line range hint
1-91: Ensure comprehensive testing of the deployment processWhile the addition of 'SingleRequestProxyFactory' to the deployment list is straightforward, it's crucial to ensure that the entire deployment process works correctly with this new contract.
Please run comprehensive tests to verify:
- The CREATE2 deployment process succeeds for all contracts, including 'SingleRequestProxyFactory'.
- The
isContractDeployedfunction correctly identifies the deployment status of 'SingleRequestProxyFactory'.Consider running the following script to check for any deployment-related functions or tests that might need updating:
This will help ensure that all necessary updates have been made to support the new contract deployment.
Verification successful
** Comprehensive deployment process confirmed**
The addition of
SingleRequestProxyFactoryto thecreate2ContractDeploymentListhas been successfully integrated into the deployment process. Deployment scripts and corresponding tests accurately handle the new contract, ensuring it is deployed using the CREATE2 scheme and its deployment status is correctly identified.
- Deployment Scripts:
SingleRequestProxyFactoryis present in multiple deployment scripts.- Test Coverage: Comprehensive tests verify the deployment and initialization of
SingleRequestProxyFactory.Please ensure that all tests pass and consider adding any additional test cases if necessary to maintain robust coverage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for deployment-related functions and tests echo "Searching for deployment-related code:" rg --type typescript "(deploy|create2|isContractDeployed)" packages/smart-contracts echo "\nSearching for test files related to deployment:" fd -e ts -e js test packages/smart-contracts | xargs rg "(deploy|create2|isContractDeployed)"Length of output: 29352
Script:
#!/bin/bash # Search for deployment-related functions and tests using specific file extensions echo "Searching for deployment-related code:" rg "(deploy|create2|isContractDeployed)" packages/smart-contracts --glob '*.ts' --glob '*.tsx' echo "\nSearching for test files related to deployment:" fd -e ts -e js test packages/smart-contracts | xargs rg "(deploy|create2|isContractDeployed)"Length of output: 94916
packages/smart-contracts/scripts-create2/compute-one-address.ts (2)
Line range hint
1-85: Summary: Successful integration of SingleRequestProxyFactoryThe changes made to this file are minimal and focused, successfully integrating the SingleRequestProxyFactory into the existing structure for computing deployment addresses. This addition aligns well with the PR objectives and maintains the file's overall integrity and functionality.
As a follow-up, ensure that the SingleRequestProxyFactory contract is fully implemented, tested, and properly integrated into the project's ecosystem as per the PR objectives.
67-68: LGTM: Addition of SingleRequestProxyFactory caseThe addition of the 'SingleRequestProxyFactory' case to the switch statement is consistent with the existing structure and aligns with the PR objectives. This change allows for the computation of deployment addresses for the SingleRequestProxyFactory contract.
To ensure consistency across the codebase, let's verify the implementation of the SingleRequestProxyFactory:
This script will help verify the proper implementation and testing of the SingleRequestProxyFactory across the project.
Verification successful
Verified: SingleRequestProxyFactory implementation confirmed
The
SingleRequestProxyFactorycontract and its corresponding tests are present in the codebase, ensuring that the addition to the switch statement is properly supported.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of SingleRequestProxyFactory across the codebase # Test 1: Check for the contract definition echo "Searching for SingleRequestProxyFactory contract definition:" ast-grep --lang solidity --pattern 'contract SingleRequestProxyFactory { $$$ }' # Test 2: Check for imports or uses of SingleRequestProxyFactory echo "Searching for imports or uses of SingleRequestProxyFactory:" rg --type typescript --type solidity 'SingleRequestProxyFactory' # Test 3: Check for any related test files echo "Searching for related test files:" fd -e ts -e sol 'SingleRequestProxyFactory.*test'Length of output: 763
Script:
#!/bin/bash # Description: Verify the implementation of SingleRequestProxyFactory across the codebase with corrected commands # Test 1: Check for the contract definition using rg instead of ast-grep echo "Searching for SingleRequestProxyFactory contract definition using rg:" rg 'contract SingleRequestProxyFactory\s*\{' -t sol # Test 2: Check for imports or uses of SingleRequestProxyFactory with corrected type echo "Searching for imports or uses of SingleRequestProxyFactory:" rg 'SingleRequestProxyFactory' --type ts --type sol # Test 3: Check for any related test files echo "Searching for related test files:" fd -e ts -e sol 'SingleRequestProxyFactory.*test'Length of output: 642
Script:
#!/bin/bash # Description: Verify the implementation of SingleRequestProxyFactory across the codebase with adjusted commands # Test 1: Check for the contract definition using rg without type filter echo "Searching for SingleRequestProxyFactory contract definition using rg without type filter:" rg 'contract\s+SingleRequestProxyFactory\s*\{' # Test 2: Check for imports or uses of SingleRequestProxyFactory without type filter echo "Searching for imports or uses of SingleRequestProxyFactory without type filter:" rg 'SingleRequestProxyFactory' # Test 3: Locate all Solidity files and search within them for the contract definition echo "Locating all Solidity files and searching for SingleRequestProxyFactory contract definition:" fd -e sol | xargs rg 'contract\s+SingleRequestProxyFactory\s*\{' # Test 4: Confirm existence of related test files echo "Searching for related test files:" fd -e ts -e sol 'SingleRequestProxyFactory.*test'Length of output: 3077
packages/smart-contracts/scripts-create2/constructor-args.ts (1)
70-80: Implementation aligns well with PR objectives.The addition of the
SingleRequestProxyFactorycase in thegetConstructorArgsfunction is consistent with the PR objectives to implement theSingleRequestProxyFactorysmart contract. This change provides the necessary support for deploying and initializing the new smart contract.To ensure that this implementation is correctly utilized, let's verify the usage of
SingleRequestProxyFactoryin the project:This script will help verify the existence of the
SingleRequestProxyFactorycontract file and its usage across the project, ensuring that the changes made in this file are properly integrated.Verification successful
SingleRequestProxyFactory integration verified successfully.
The
SingleRequestProxyFactorycontract is present and correctly utilized across the project, including in test contracts and deployment scripts. No issues were found with the current implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of SingleRequestProxyFactory in the project # Test 1: Check for SingleRequestProxyFactory contract file echo "Checking for SingleRequestProxyFactory contract file:" fd --type f --glob '*SingleRequestProxyFactory.sol' # Test 2: Search for SingleRequestProxyFactory usage in TypeScript files echo "Searching for SingleRequestProxyFactory usage in TypeScript files:" rg --type typescript 'SingleRequestProxyFactory' # Test 3: Check for any deployment scripts using SingleRequestProxyFactory echo "Checking for deployment scripts using SingleRequestProxyFactory:" rg --type typescript 'SingleRequestProxyFactory.*deploy'Length of output: 699
Script:
#!/bin/bash # Description: Re-verify the usage of SingleRequestProxyFactory in the project with corrected file types # Test 1: Check for SingleRequestProxyFactory contract file echo "Checking for SingleRequestProxyFactory contract file:" fd --type f --glob '*SingleRequestProxyFactory.sol' # Test 2: Search for SingleRequestProxyFactory usage in TypeScript files using correct file type echo "Searching for SingleRequestProxyFactory usage in TypeScript files:" rg --type ts 'SingleRequestProxyFactory' # Test 3: Check for any deployment scripts using SingleRequestProxyFactory echo "Checking for deployment scripts using SingleRequestProxyFactory:" rg --type ts 'SingleRequestProxyFactory.*deploy'Length of output: 3043
packages/smart-contracts/test/contracts/ERC20SingleRequestProxy.test.ts (5)
1-15: LGTM: Imports and constant declaration look good.The necessary dependencies are imported, and the
BASE_DECIMALconstant is correctly defined for handling token decimals.
58-60: LGTM: Basic deployment test is in place.This test case correctly verifies that the ERC20SingleRequestProxy contract is deployed by checking its address against the zero address.
62-69: LGTM: Comprehensive initial values verification.This test case thoroughly checks all the important initial parameters of the contract, ensuring that it's initialized correctly. This is crucial for the proper functioning of the contract.
98-139: LGTM: Edge cases are well covered.These test cases effectively cover important edge cases:
- The contract correctly reverts when called with a non-zero value.
- The contract handles zero fee amount correctly, including proper event emission and balance updates.
These tests significantly contribute to the robustness of the contract by ensuring it handles special cases correctly.
1-152: Overall, excellent test coverage with minor suggestions for improvement.This test suite for the ERC20SingleRequestProxy contract is comprehensive and well-structured. It covers all major functionalities, including deployment, initial state verification, payment processing, and important edge cases. The suggested improvements are minor and aimed at enhancing the robustness and readability of the tests.
Great job on writing thorough tests for this smart contract!
Tools
Biome
[error] 19-19: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 20-20: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 21-21: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 23-25: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol (1)
5-5: Verify the correct import ofIERC20FeeProxyinterfaceEnsure that the
IERC20FeeProxyinterface is correctly defined and imported. The filenameERC20FeeProxy.solsuggests it might contain a contract instead of an interface.Run the following script to verify the interface definition:
Verification successful
IERC20FeeProxy interface is correctly defined and imported
The
IERC20FeeProxyinterface is correctly defined ininterfaces/ERC20FeeProxy.soland properly imported inERC20SingleRequestProxy.sol.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the definition of IERC20FeeProxy interface # Test: Search for the interface declaration rg --type solidity -A 3 'interface IERC20FeeProxy'Length of output: 85
Script:
#!/bin/bash # Description: Check for the definition of IERC20FeeProxy interface using ast-grep # Search for the interface declaration in Solidity files ast-grep --lang solidity --pattern 'interface IERC20FeeProxy { $$$ }'Length of output: 188
Script:
#!/bin/bash # Description: Locate IERC20FeeProxy interface definition in Solidity files # Find all .sol files and search for the interface declaration fd --extension sol --exec grep -H 'interface IERC20FeeProxy' {}Length of output: 164
packages/smart-contracts/src/contracts/EthereumSingleRequestProxy.sol (2)
11-81: LGTM!Overall, the contract logic is sound, and the implementation aligns with the intended functionality as described.
59-60: Check for potential issues with forwarding funds to the original sender.Ensure that forwarding Ether using
callis secure and that the original sender cannot execute malicious code upon receiving Ether.Run the following script to search for non-standard
receiveorfallbackfunctions in the codebase that could be called during fund transfers:packages/smart-contracts/src/contracts/SingleRequestProxyFactory.sol (5)
29-32: Constructor initializes fee proxy addresses correctly.The constructor properly sets the
ethereumFeeProxyanderc20FeeProxyaddresses upon deployment.
42-57:createEthereumSingleRequestProxyfunction is correctly implemented.The function successfully creates a new
EthereumSingleRequestProxyinstance and emits the appropriate event.
68-86:createERC20SingleRequestProxyfunction is correctly implemented.The function successfully creates a new
ERC20SingleRequestProxyinstance and emits the appropriate event.
92-94:setERC20FeeProxyfunction is properly restricted.The function is correctly restricted to the owner using the
onlyOwnermodifier.
100-102:setEthereumFeeProxyfunction is properly restricted.The function is correctly restricted to the owner using the
onlyOwnermodifier.packages/smart-contracts/test/contracts/EthereumSingleRequestProxy.test.ts (1)
31-36: Ensure constructor parameters have correct typesVerify that the
feeAmountpassed to the contract constructor is of the correct type expected by the contract (likely auint256). Passing a string might cause issues during deployment.Run the following script to check the expected parameter types in the contract constructor:
packages/smart-contracts/test/contracts/SingleRequestProxyFactory.test.ts (7)
30-57: Initialization setup is correctThe
beforeEachhook properly deploys all necessary contracts and initializes variables, ensuring a clean state for each test.
59-64: Deployment test verifies initial state accuratelyThe test correctly checks that the
SingleRequestProxyFactorycontract is deployed with the expected initial values, ensuring that the addresses and owner are set properly.
101-136: ERC20 Single Request Proxy creation test is comprehensiveThe test thoroughly validates the creation of an
ERC20SingleRequestProxy, including event emission and the correct initialization of contract properties such as payee, token address, payment reference, fee proxy address, fee recipient, and fee amount.
138-144: Owner can update ERC20FeeProxy addressThe test confirms that the contract owner can successfully update the
ERC20FeeProxyaddress, and the new address is stored correctly.
178-181: Ownership transfer functionality works as expectedThe test verifies that the owner can transfer ownership to another address, and the
owner()function reflects the change appropriately.
183-187: Ownership renouncement by new owner functions correctlyAfter ownership transfer, the test checks that the new owner can renounce ownership, effectively setting the owner to the zero address, and the contract reflects this change.
189-193: Access control enforced for non-owner attempting ownership transferThe test appropriately ensures that a non-owner cannot transfer ownership, and the transaction reverts with the expected error message.
| import '@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol'; | ||
|
|
||
| contract TestToken is ERC20, Ownable, ERC20Permit { | ||
| constructor(address initialOwner) ERC20('TestToken', 'TTK') Ownable() ERC20Permit('TestToken') {} |
There was a problem hiding this comment.
Constructor looks good, but consider parameterizing token name and symbol.
The constructor correctly initializes all inherited contracts and allows for flexible ownership assignment. However, hardcoding the token name and symbol might limit the contract's reusability.
Consider parameterizing the token name and symbol:
-constructor(address initialOwner) ERC20('TestToken', 'TTK') Ownable() ERC20Permit('TestToken') {}
+constructor(
+ string memory name,
+ string memory symbol,
+ address initialOwner
+) ERC20(name, symbol) Ownable() ERC20Permit(name) {}This change would make the contract more flexible for different use cases.
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.
| constructor(address initialOwner) ERC20('TestToken', 'TTK') Ownable() ERC20Permit('TestToken') {} | |
| constructor( | |
| string memory name, | |
| string memory symbol, | |
| address initialOwner | |
| ) ERC20(name, symbol) Ownable() ERC20Permit(name) {} |
| function mint(address to, uint256 amount) public onlyOwner { | ||
| _mint(to, amount); | ||
| } |
There was a problem hiding this comment.
Mint function is correct, but consider adding an event emission.
The mint function is correctly implemented with the onlyOwner modifier for access control. It uses the internal _mint function as expected.
Consider adding an event emission after minting for better off-chain tracking:
function mint(address to, uint256 amount) public onlyOwner {
_mint(to, amount);
+ emit TokensMinted(to, amount);
}
+event TokensMinted(address indexed to, uint256 amount);This addition would improve the contract's observability and make it easier for external systems to track minting operations.
Committable suggestion was skipped due to low confidence.
| require(success, 'Failed to send funds back'); | ||
| } | ||
|
|
||
| receive() external payable {} |
There was a problem hiding this comment.
LGTM! Consider adding an event emission for better testability and transparency.
The receive function is correctly implemented to allow the contract to accept Ether transfers without calling a specific function. This is appropriate for a mock contract.
To enhance testability and transparency, consider emitting an event when Ether is received. Here's a suggested implementation:
+ event EtherReceived(address sender, uint256 amount);
receive() external payable {
+ emit EtherReceived(msg.sender, msg.value);
}This addition would make it easier to track and verify Ether transfers to the contract during testing.
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.
| receive() external payable {} | |
| event EtherReceived(address sender, uint256 amount); | |
| receive() external payable { | |
| emit EtherReceived(msg.sender, msg.value); | |
| } |
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| contract MockEthereumFeeProxy { | ||
| function transferWithReferenceAndFee( | ||
| address payable _to, | ||
| bytes calldata _paymentReference, | ||
| uint256 _feeAmount, | ||
| address payable _feeAddress | ||
| ) external payable { | ||
| // Do nothing, just accept the funds | ||
| } |
There was a problem hiding this comment.
LGTM! Consider adding an event emission for better testability.
The MockEthereumFeeProxy contract and the transferWithReferenceAndFee function are correctly implemented for a mock contract. The function appropriately accepts parameters without using them, which is suitable for testing scenarios.
To enhance testability, consider emitting an event in the transferWithReferenceAndFee function. This would allow test cases to verify that the function was called with the correct parameters. Here's a suggested implementation:
+ event TransferWithReferenceAndFeeCalled(address to, bytes paymentReference, uint256 feeAmount, address feeAddress, uint256 value);
function transferWithReferenceAndFee(
address payable _to,
bytes calldata _paymentReference,
uint256 _feeAmount,
address payable _feeAddress
) external payable {
// Do nothing, just accept the funds
+ emit TransferWithReferenceAndFeeCalled(_to, _paymentReference, _feeAmount, _feeAddress, msg.value);
}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.
| // SPDX-License-Identifier: MIT | |
| pragma solidity ^0.8.0; | |
| contract MockEthereumFeeProxy { | |
| function transferWithReferenceAndFee( | |
| address payable _to, | |
| bytes calldata _paymentReference, | |
| uint256 _feeAmount, | |
| address payable _feeAddress | |
| ) external payable { | |
| // Do nothing, just accept the funds | |
| } | |
| // SPDX-License-Identifier: MIT | |
| pragma solidity ^0.8.0; | |
| contract MockEthereumFeeProxy { | |
| event TransferWithReferenceAndFeeCalled(address to, bytes paymentReference, uint256 feeAmount, address feeAddress, uint256 value); | |
| function transferWithReferenceAndFee( | |
| address payable _to, | |
| bytes calldata _paymentReference, | |
| uint256 _feeAmount, | |
| address payable _feeAddress | |
| ) external payable { | |
| // Do nothing, just accept the funds | |
| emit TransferWithReferenceAndFeeCalled(_to, _paymentReference, _feeAmount, _feeAddress, msg.value); | |
| } |
| function sendFundsBack(address payable _to, uint256 _amount) external { | ||
| (bool success, ) = _to.call{value: _amount}(''); | ||
| require(success, 'Failed to send funds back'); | ||
| } |
There was a problem hiding this comment.
LGTM! Consider implementing checks-effects-interactions pattern.
The sendFundsBack function correctly uses a low-level call to transfer Ether and includes a require statement to ensure the transfer's success. This is a good practice for handling Ether transfers.
To enhance security and prevent potential reentrancy attacks, consider implementing the checks-effects-interactions pattern. Here's a suggested implementation:
+ uint256 private _balance;
+ function deposit() external payable {
+ _balance += msg.value;
+ }
function sendFundsBack(address payable _to, uint256 _amount) external {
+ require(_balance >= _amount, "Insufficient balance");
+ _balance -= _amount;
(bool success, ) = _to.call{value: _amount}('');
require(success, 'Failed to send funds back');
}This implementation separates the state changes from the external call, reducing the risk of reentrancy attacks.
Committable suggestion was skipped due to low confidence.
| it('should handle funds sent back from EthereumFeeProxy', async () => { | ||
| const MockEthereumFeeProxyFactory = await ethers.getContractFactory('MockEthereumFeeProxy'); | ||
| const mockEthereumFeeProxy = await MockEthereumFeeProxyFactory.deploy(); | ||
| await mockEthereumFeeProxy.deployed(); | ||
|
|
||
| const newEthereumSingleRequestProxyFactory = await ethers.getContractFactory( | ||
| 'EthereumSingleRequestProxy', | ||
| ); | ||
| const newEthereumSingleRequestProxy = await newEthereumSingleRequestProxyFactory.deploy( | ||
| payeeAddress, | ||
| paymentReference, | ||
| mockEthereumFeeProxy.address, | ||
| feeRecipientAddress, | ||
| feeAmount, | ||
| ); | ||
| await newEthereumSingleRequestProxy.deployed(); | ||
|
|
||
| const paymentAmount = ethers.utils.parseEther('1'); | ||
| const totalAmount = paymentAmount.add(feeAmount); | ||
|
|
||
| await owner.sendTransaction({ | ||
| to: newEthereumSingleRequestProxy.address, | ||
| value: totalAmount, | ||
| }); | ||
|
|
||
| expect(await ethers.provider.getBalance(newEthereumSingleRequestProxy.address)).to.equal(0); | ||
| expect(await ethers.provider.getBalance(mockEthereumFeeProxy.address)).to.equal(totalAmount); | ||
|
|
||
| await expect(() => | ||
| mockEthereumFeeProxy.sendFundsBack(newEthereumSingleRequestProxy.address, totalAmount), | ||
| ).to.changeEtherBalances( | ||
| [owner, newEthereumSingleRequestProxy, mockEthereumFeeProxy], | ||
| [totalAmount, 0, totalAmount.mul(-1)], | ||
| ); | ||
|
|
||
| expect(await ethers.provider.getBalance(newEthereumSingleRequestProxy.address)).to.equal(0); | ||
| expect(await ethers.provider.getBalance(mockEthereumFeeProxy.address)).to.equal(0); | ||
| }); |
There was a problem hiding this comment.
Include tests for edge cases and failure scenarios
To ensure robustness, add tests that cover edge cases such as insufficient funds, invalid addresses, or unauthorized access attempts.
| await expect( | ||
| owner.sendTransaction({ | ||
| to: ethereumSingleRequestProxy.address, | ||
| value: totalAmount, | ||
| }), | ||
| ) | ||
| .to.emit(ethereumFeeProxy, 'TransferWithReferenceAndFee') | ||
| .withArgs(payeeAddress, paymentAmount, paymentReference, feeAmount, feeRecipientAddress); | ||
|
|
There was a problem hiding this comment.
Correct the usage of expect in event emission assertion
Similarly, the expect function should wrap the transaction in a function without an await. This ensures that the assertion correctly captures the promise.
Apply this diff to fix the syntax:
-await expect(
- owner.sendTransaction({
+await expect(() =>
+ owner.sendTransaction({
to: ethereumSingleRequestProxy.address,
value: totalAmount,
}),
-)
+})
.to.emit(ethereumFeeProxy, 'TransferWithReferenceAndFee')
.withArgs(payeeAddress, paymentAmount, paymentReference, feeAmount, feeRecipientAddress);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.
| await expect( | |
| owner.sendTransaction({ | |
| to: ethereumSingleRequestProxy.address, | |
| value: totalAmount, | |
| }), | |
| ) | |
| .to.emit(ethereumFeeProxy, 'TransferWithReferenceAndFee') | |
| .withArgs(payeeAddress, paymentAmount, paymentReference, feeAmount, feeRecipientAddress); | |
| await expect(() => | |
| owner.sendTransaction({ | |
| to: ethereumSingleRequestProxy.address, | |
| value: totalAmount, | |
| }), | |
| ) | |
| .to.emit(ethereumFeeProxy, 'TransferWithReferenceAndFee') | |
| .withArgs(payeeAddress, paymentAmount, paymentReference, feeAmount, feeRecipientAddress); |
| await expect(() => | ||
| mockEthereumFeeProxy.sendFundsBack(newEthereumSingleRequestProxy.address, totalAmount), | ||
| ).to.changeEtherBalances( | ||
| [owner, newEthereumSingleRequestProxy, mockEthereumFeeProxy], | ||
| [totalAmount, 0, totalAmount.mul(-1)], | ||
| ); |
There was a problem hiding this comment.
Correct the function wrapping in balance change assertion
When using changeEtherBalances, the function should be wrapped in a callback without immediately invoking it.
Apply this diff to fix the syntax:
-await expect(() =>
- mockEthereumFeeProxy.sendFundsBack(newEthereumSingleRequestProxy.address, totalAmount),
-).to.changeEtherBalances(
+await expect(
+ () => mockEthereumFeeProxy.sendFundsBack(newEthereumSingleRequestProxy.address, totalAmount),
+).to.changeEtherBalances(
[owner, newEthereumSingleRequestProxy, mockEthereumFeeProxy],
[totalAmount, 0, totalAmount.mul(-1)],
);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.
| await expect(() => | |
| mockEthereumFeeProxy.sendFundsBack(newEthereumSingleRequestProxy.address, totalAmount), | |
| ).to.changeEtherBalances( | |
| [owner, newEthereumSingleRequestProxy, mockEthereumFeeProxy], | |
| [totalAmount, 0, totalAmount.mul(-1)], | |
| ); | |
| await expect( | |
| () => mockEthereumFeeProxy.sendFundsBack(newEthereumSingleRequestProxy.address, totalAmount), | |
| ).to.changeEtherBalances( | |
| [owner, newEthereumSingleRequestProxy, mockEthereumFeeProxy], | |
| [totalAmount, 0, totalAmount.mul(-1)], | |
| ); |
| const paymentReference: string = ethers.utils.formatBytes32String('payment_reference'); | ||
| const feeAmount: string = ethers.utils.parseEther('0.1').toString(); |
There was a problem hiding this comment.
Avoid unnecessary conversion to string for feeAmount
The feeAmount is parsed to a BigNumber and then converted to a string, which is unnecessary and may lead to type inconsistencies.
Consider keeping feeAmount as a BigNumber:
-const feeAmount: string = ethers.utils.parseEther('0.1').toString();
+const feeAmount = ethers.utils.parseEther('0.1');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 paymentReference: string = ethers.utils.formatBytes32String('payment_reference'); | |
| const feeAmount: string = ethers.utils.parseEther('0.1').toString(); | |
| const paymentReference: string = ethers.utils.formatBytes32String('payment_reference'); | |
| const feeAmount = ethers.utils.parseEther('0.1'); |
| await expect( | ||
| await owner.sendTransaction({ | ||
| to: ethereumSingleRequestProxy.address, | ||
| value: totalAmount, | ||
| }), | ||
| ).to.changeEtherBalances( | ||
| [owner, payee, feeRecipient], | ||
| [totalAmount.mul(-1), paymentAmount, feeAmount], | ||
| ); |
There was a problem hiding this comment.
Correct the usage of await with expect in balance change assertion
The await is incorrectly placed when asserting balance changes. The expect function should receive a function that returns a promise, not the resolved promise itself.
Apply this diff to fix the syntax:
-await expect(
- await owner.sendTransaction({
+await expect(() =>
+ owner.sendTransaction({
to: ethereumSingleRequestProxy.address,
value: totalAmount,
}),
-).to.changeEtherBalances(
+).to.changeEtherBalances(
[owner, payee, feeRecipient],
[totalAmount.mul(-1), paymentAmount, 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.
| await expect( | |
| await owner.sendTransaction({ | |
| to: ethereumSingleRequestProxy.address, | |
| value: totalAmount, | |
| }), | |
| ).to.changeEtherBalances( | |
| [owner, payee, feeRecipient], | |
| [totalAmount.mul(-1), paymentAmount, feeAmount], | |
| ); | |
| await expect(() => | |
| owner.sendTransaction({ | |
| to: ethereumSingleRequestProxy.address, | |
| value: totalAmount, | |
| }), | |
| ).to.changeEtherBalances( | |
| [owner, payee, feeRecipient], | |
| [totalAmount.mul(-1), paymentAmount, feeAmount], | |
| ); |
| 'transferWithReferenceAndFee(address,bytes,uint256,address)', | ||
| payable(payee), | ||
| paymentReference, | ||
| feeAmount, |
There was a problem hiding this comment.
In both ERC20SingleRequestProxy and EthereumSingleRequestProxy the fees amount are paid every time the receive method is called.
It won't be possible for a user to pay his requests with multiple payment as he'll need to pay for the fees several times. Is it the intended behavior ?
There was a problem hiding this comment.
Yes, that is the intended behavior. The same thing happens when a request is created with feeAddress and feeAmount using the SDK; the fee is paid every time there is a payment.
This is a test request we did to see the behavior of the partial payments with fees , you can see the service fee being paid every time:
https://scan.request.network/request/0118bc428d569e48a62a64625bbd836dfc00c935caae8a201daf308438fce3ddd9
There was a problem hiding this comment.
The fees aren't paid necessarely every time there is a payment. For example if I have a request where I need to pay 100$ as well as 2$ of fees, I can:
- Make on payment of 50$ and 2$ fees.
- Another payment of 50$ and 0$ of fees
We'll detect both of these payments and the request will be considered paid, but with the current implementation of the contract you'd have to pay twice the 2$ of fees if you want to make partial payments.
Maybe the feeAmount should be updated after each payment ?
I don't understand the link you shared, the payment are not partial, each of them account for the full amount of the request (0.003 ETH)
Edit:
Using the SDK I might not be able to perform the two payments as I described them - though it's doable if I interact directly with the contract, and the request will be considered paid.
But I believe this is because the payment-processor package does not support well partial payment yet.
I don't think paying several times the fees in case of partial payment should be enforced at the contract level
Resolves #1394
Summary by CodeRabbit
Release Notes
New Features
ERC20SingleRequestProxyfor streamlined ERC20 token transfers with fee management.EthereumSingleRequestProxyfor handling single payment requests with integrated fee processing.SingleRequestProxyFactoryto create instances of the above proxies, enhancing modularity in payment processing.Bug Fixes
Tests
ERC20SingleRequestProxy,EthereumSingleRequestProxy, andSingleRequestProxyFactoryto validate functionality and ensure reliability.