-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Develop SingleRequestProxy
Smart Contracts
#1453
base: master
Are you sure you want to change the base?
Changes from all commits
e2567f4
c624474
917d472
d5e4728
0543092
2bd3495
19b5e40
c1d3a77
1376582
c2f55ee
bc922e8
8aef234
0769aa7
3bd9e38
ed429ae
d9a4484
a341ce1
10974b5
2cc82a0
bee1f58
aab6711
0c68eba
796f2b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ export const create2ContractDeploymentList = [ | |
'BatchConversionPayments', | ||
'ERC20EscrowToPay', | ||
'ERC20TransferableReceivable', | ||
'SingleRequestProxyFactory', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addition of 'SingleRequestProxyFactory' approved, but update required in The addition of 'SingleRequestProxyFactory' to the The case 'SingleRequestProxyFactory':
return artifacts.singleRequestProxyFactoryArtifact; Make sure to import the correct artifact from the artifacts file. |
||
]; | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,57 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// SPDX-License-Identifier: MIT | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pragma solidity 0.8.9; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import '@openzeppelin/contracts/token/ERC20/IERC20.sol'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import './interfaces/ERC20FeeProxy.sol'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @title ERC20SingleRequestProxy | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @notice This contract is used to send a single request to a payee with a fee sent to a third address for ERC20 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
contract ERC20SingleRequestProxy { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address public payee; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address public tokenAddress; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address public feeAddress; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
uint256 public feeAmount; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
leoslr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
bytes public paymentReference; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
IERC20FeeProxy public erc20FeeProxy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
constructor( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address _payee, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address _tokenAddress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address _feeAddress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
uint256 _feeAmount, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
bytes memory _paymentReference, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address _erc20FeeProxy | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
payee = _payee; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
tokenAddress = _tokenAddress; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
feeAddress = _feeAddress; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
feeAmount = _feeAmount; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
paymentReference = _paymentReference; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
erc20FeeProxy = IERC20FeeProxy(_erc20FeeProxy); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+20
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add input validation in the constructor Consider adding Apply this diff to add input validation: ) {
+ require(_payee != address(0), 'Invalid payee address');
+ require(_tokenAddress != address(0), 'Invalid token address');
+ require(_erc20FeeProxy != address(0), 'Invalid ERC20FeeProxy address');
payee = _payee;
tokenAddress = _tokenAddress;
feeAddress = _feeAddress;
feeAmount = _feeAmount;
paymentReference = _paymentReference;
erc20FeeProxy = IERC20FeeProxy(_erc20FeeProxy);
} Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
receive() external payable { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
require(msg.value == 0, 'This function is only for triggering the transfer'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+42
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adjust balance check to allow exact fee amount The condition Apply this diff to adjust the condition: - require(balance > feeAmount, 'Insufficient balance to cover fee');
+ require(balance >= feeAmount, 'Insufficient balance to cover fee'); Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
token.approve(address(erc20FeeProxy), balance); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Limit token approval to the required amount Approving the entire Apply this diff to limit the token approval: - token.approve(address(erc20FeeProxy), balance);
+ token.approve(address(erc20FeeProxy), paymentAmount + feeAmount); Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
erc20FeeProxy.transferFromWithReferenceAndFee( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
tokenAddress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
payee, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
paymentAmount, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
paymentReference, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
feeAmount, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
feeAddress | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+36
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace the Using the Apply this diff to replace the - receive() external payable {
+ function executePayment() external {
require(msg.value == 0, 'This function is only for triggering the transfer');
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');
+ require(balance >= feeAmount, 'Insufficient balance to cover fee');
paymentAmount = balance - feeAmount;
}
token.approve(address(erc20FeeProxy), balance);
erc20FeeProxy.transferFromWithReferenceAndFee(
tokenAddress,
payee,
paymentAmount,
paymentReference,
feeAmount,
feeAddress
);
} Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,81 @@ | ||||||||||||||||||||||||||||||||||||||||||
// SPDX-License-Identifier: MIT | ||||||||||||||||||||||||||||||||||||||||||
pragma solidity 0.8.9; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
import './interfaces/EthereumFeeProxy.sol'; | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verify the correct import path for Ensure that the Apply this diff if necessary: - import './interfaces/EthereumFeeProxy.sol';
+ import './interfaces/IEthereumFeeProxy.sol'; Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||
* @title EthereumSingleRequestProxy | ||||||||||||||||||||||||||||||||||||||||||
* @notice This contract is used to send a single request to a payee with a fee sent to a third address | ||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||
contract EthereumSingleRequestProxy { | ||||||||||||||||||||||||||||||||||||||||||
address public payee; | ||||||||||||||||||||||||||||||||||||||||||
bytes public paymentReference; | ||||||||||||||||||||||||||||||||||||||||||
address public feeAddress; | ||||||||||||||||||||||||||||||||||||||||||
uint256 public feeAmount; | ||||||||||||||||||||||||||||||||||||||||||
leoslr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||
IEthereumFeeProxy public ethereumFeeProxy; | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+11
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restrict visibility of state variables if not needed publicly. If external access to state variables is not required, consider changing their visibility to Apply this diff if external access is unnecessary: - address public payee;
- bytes public paymentReference;
- address public feeAddress;
- uint256 public feeAmount;
- IEthereumFeeProxy public ethereumFeeProxy;
+ address private payee;
+ bytes private paymentReference;
+ address private feeAddress;
+ uint256 private feeAmount;
+ IEthereumFeeProxy private ethereumFeeProxy; Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
address private originalSender; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// Reentrancy guard | ||||||||||||||||||||||||||||||||||||||||||
uint256 private constant _NOT_ENTERED = 1; | ||||||||||||||||||||||||||||||||||||||||||
uint256 private constant _ENTERED = 2; | ||||||||||||||||||||||||||||||||||||||||||
uint256 private _status; | ||||||||||||||||||||||||||||||||||||||||||
leoslr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+19
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use OpenZeppelin's Implementing a custom reentrancy guard can be error-prone. It is recommended to inherit from OpenZeppelin's Apply this diff to use +import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
-contract EthereumSingleRequestProxy {
+contract EthereumSingleRequestProxy is ReentrancyGuard {
address public payee;
bytes public paymentReference;
address public feeAddress;
uint256 public feeAmount;
IEthereumFeeProxy public ethereumFeeProxy;
- address private originalSender;
-
- // Reentrancy guard
- uint256 private constant _NOT_ENTERED = 1;
- uint256 private constant _ENTERED = 2;
- uint256 private _status;
constructor(
address _payee,
bytes memory _paymentReference,
address _ethereumFeeProxy,
address _feeAddress,
uint256 _feeAmount
) {
payee = _payee;
paymentReference = _paymentReference;
feeAddress = _feeAddress;
feeAmount = _feeAmount;
ethereumFeeProxy = IEthereumFeeProxy(_ethereumFeeProxy);
- _status = _NOT_ENTERED;
}
- modifier nonReentrant() {
- if (msg.sender != address(ethereumFeeProxy)) {
- require(_status != _ENTERED, 'ReentrancyGuard: reentrant call');
- _status = _ENTERED;
- }
- _;
- if (msg.sender != address(ethereumFeeProxy)) {
- _status = _NOT_ENTERED;
- }
- }
+ // Remove custom `nonReentrant` modifier and use the inherited one Also applies to: 39-51 |
||||||||||||||||||||||||||||||||||||||||||
constructor( | ||||||||||||||||||||||||||||||||||||||||||
address _payee, | ||||||||||||||||||||||||||||||||||||||||||
bytes memory _paymentReference, | ||||||||||||||||||||||||||||||||||||||||||
address _ethereumFeeProxy, | ||||||||||||||||||||||||||||||||||||||||||
address _feeAddress, | ||||||||||||||||||||||||||||||||||||||||||
uint256 _feeAmount | ||||||||||||||||||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||||||||||||||||||
payee = _payee; | ||||||||||||||||||||||||||||||||||||||||||
paymentReference = _paymentReference; | ||||||||||||||||||||||||||||||||||||||||||
feeAddress = _feeAddress; | ||||||||||||||||||||||||||||||||||||||||||
feeAmount = _feeAmount; | ||||||||||||||||||||||||||||||||||||||||||
ethereumFeeProxy = IEthereumFeeProxy(_ethereumFeeProxy); | ||||||||||||||||||||||||||||||||||||||||||
_status = _NOT_ENTERED; | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
modifier nonReentrant() { | ||||||||||||||||||||||||||||||||||||||||||
if (msg.sender != address(ethereumFeeProxy)) { | ||||||||||||||||||||||||||||||||||||||||||
// On the first call to nonReentrant, _status will be _NOT_ENTERED | ||||||||||||||||||||||||||||||||||||||||||
require(_status != _ENTERED, 'ReentrancyGuard: reentrant call'); | ||||||||||||||||||||||||||||||||||||||||||
// Any calls to nonReentrant after this point will fail | ||||||||||||||||||||||||||||||||||||||||||
_status = _ENTERED; | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
_; | ||||||||||||||||||||||||||||||||||||||||||
if (msg.sender != address(ethereumFeeProxy)) { | ||||||||||||||||||||||||||||||||||||||||||
// By storing the original value once again, a refund is triggered | ||||||||||||||||||||||||||||||||||||||||||
_status = _NOT_ENTERED; | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
receive() external payable nonReentrant { | ||||||||||||||||||||||||||||||||||||||||||
if (msg.sender == address(ethereumFeeProxy)) { | ||||||||||||||||||||||||||||||||||||||||||
// Funds are being sent back from EthereumFeeProxy | ||||||||||||||||||||||||||||||||||||||||||
require(originalSender != address(0), 'No original sender stored'); | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// Forward the funds to the original sender | ||||||||||||||||||||||||||||||||||||||||||
(bool forwardSuccess, ) = payable(originalSender).call{value: msg.value}(''); | ||||||||||||||||||||||||||||||||||||||||||
require(forwardSuccess, 'Forwarding to original sender failed'); | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// Clear the stored original sender | ||||||||||||||||||||||||||||||||||||||||||
originalSender = address(0); | ||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||
require(originalSender == address(0), 'Another request is in progress'); | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
originalSender = msg.sender; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
bytes memory data = abi.encodeWithSignature( | ||||||||||||||||||||||||||||||||||||||||||
'transferWithReferenceAndFee(address,bytes,uint256,address)', | ||||||||||||||||||||||||||||||||||||||||||
payable(payee), | ||||||||||||||||||||||||||||||||||||||||||
paymentReference, | ||||||||||||||||||||||||||||||||||||||||||
feeAmount, | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that is the intended behavior. The same thing happens when a request is created with 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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. 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: |
||||||||||||||||||||||||||||||||||||||||||
payable(feeAddress) | ||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
(bool callSuccess, ) = address(ethereumFeeProxy).call{value: msg.value}(data); | ||||||||||||||||||||||||||||||||||||||||||
require(callSuccess, 'Call to EthereumFeeProxy failed'); | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+77
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential failure of the external call gracefully. Consider providing more detailed error handling or reverting state changes if the call to
Comment on lines
+69
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Invoke Using the interface method directly enhances code readability and safety by leveraging Solidity's function call mechanisms and type checking. Apply this diff to update the function call: require(originalSender == address(0), 'Another request is in progress');
originalSender = msg.sender;
- bytes memory data = abi.encodeWithSignature(
- 'transferWithReferenceAndFee(address,bytes,uint256,address)',
- payable(payee),
- paymentReference,
- feeAmount,
- payable(feeAddress)
- );
-
- (bool callSuccess, ) = address(ethereumFeeProxy).call{value: msg.value}(data);
- require(callSuccess, 'Call to EthereumFeeProxy failed');
+ ethereumFeeProxy.transferWithReferenceAndFee{value: msg.value}(
+ payable(payee),
+ paymentReference,
+ feeAmount,
+ payable(feeAddress)
+ ); Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+53
to
+80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure reentrancy protection covers all code paths. By allowing calls from |
||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,103 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// SPDX-License-Identifier: MIT | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pragma solidity 0.8.9; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import '@openzeppelin/contracts/access/Ownable.sol'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import './ERC20SingleRequestProxy.sol'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import './EthereumSingleRequestProxy.sol'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @title SingleRequestProxyFactory | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @notice This contract is used to create SingleRequestProxy instances | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
contract SingleRequestProxyFactory is Ownable { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address public ethereumFeeProxy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address public erc20FeeProxy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add NatSpec comments for public variables. Consider adding NatSpec comments for the public variables |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
event EtheruemSingleRequestProxyCreated( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address indexed proxyAddress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address indexed payee, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
bytes indexed paymentReference | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+16
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix the typo in the event name 'EtheruemSingleRequestProxyCreated'. The event name Apply this diff to correct the event name: -event EtheruemSingleRequestProxyCreated(
+event EthereumSingleRequestProxyCreated(
address indexed proxyAddress,
address indexed payee,
bytes indexed paymentReference
); Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
event ERC20SingleRequestProxyCreated( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address indexed proxyAddress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address indexed payee, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address tokenAddress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
bytes indexed paymentReference | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
constructor(address _ethereumFeeProxy, address _erc20FeeProxy) Ownable() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ethereumFeeProxy = _ethereumFeeProxy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
erc20FeeProxy = _erc20FeeProxy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+29
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add validation for fee proxy addresses in the constructor. To prevent misconfiguration, add checks to ensure Add 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @notice Creates a new EthereumSingleRequestProxy instance | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _payee The address of the payee | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _paymentReference The payment reference | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _feeAddress The address of the fee recipient | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _feeAmount The fee amount | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @return The address of the newly created proxy | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function createEthereumSingleRequestProxy( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address _payee, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
bytes memory _paymentReference, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
address _feeAddress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
uint256 _feeAmount | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) external returns (address) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
EthereumSingleRequestProxy proxy = new EthereumSingleRequestProxy( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_payee, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_paymentReference, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ethereumFeeProxy, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_feeAddress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_feeAmount | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
emit EtheruemSingleRequestProxyCreated(address(proxy), _payee, _paymentReference); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix the typo in the event emission 'EtheruemSingleRequestProxyCreated'. Update the event emission to use the corrected event name Apply this diff to correct the event emission: -emit EtheruemSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
+emit EthereumSingleRequestProxyCreated(address(proxy), _payee, _paymentReference); Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return address(proxy); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+42
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add input validation for function parameters in 'createEthereumSingleRequestProxy'. It's recommended to validate critical input parameters to prevent errors or misuse. Add checks to ensure Include function createEthereumSingleRequestProxy(
address _payee,
bytes memory _paymentReference,
address _feeAddress,
uint256 _feeAmount
) external returns (address) {
+ require(_payee != address(0), "Payee address cannot be zero");
+ require(_feeAddress != address(0), "Fee address cannot be zero");
+ require(ethereumFeeProxy != address(0), "ethereumFeeProxy address not set");
EthereumSingleRequestProxy proxy = new EthereumSingleRequestProxy(
_payee,
_paymentReference,
ethereumFeeProxy,
_feeAddress,
_feeAmount
);
emit EthereumSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
return address(proxy);
} Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @notice Creates a new ERC20SingleRequestProxy instance | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _payee The address of the payee | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _tokenAddress The address of the token | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _paymentReference The payment reference | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _feeAddress The address of the fee recipient | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _feeAmount The fee amount | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @return The address of the newly created proxy | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+68
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add input validation for function parameters in 'createERC20SingleRequestProxy'. To ensure robustness, add validations for critical input parameters. Ensure Include 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @notice Updates the ERC20FeeProxy address | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _newERC20FeeProxy The new ERC20FeeProxy address | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
erc20FeeProxy = _newERC20FeeProxy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+92
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add validation for new ERC20FeeProxy address in 'setERC20FeeProxy'. To prevent setting an invalid address, add a check to ensure Implement the validation: function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner {
+ require(_newERC20FeeProxy != address(0), "New ERC20FeeProxy address cannot be zero");
erc20FeeProxy = _newERC20FeeProxy;
} Committable suggestion
Suggested change
Consider emitting an event when updating 'erc20FeeProxy'. To enhance transparency and enable off-chain monitoring, emit an event when the 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;
}
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @notice Updates the EthereumFeeProxy address | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @param _newEthereumFeeProxy The new EthereumFeeProxy address | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ethereumFeeProxy = _newEthereumFeeProxy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+100
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add validation for new EthereumFeeProxy address in 'setEthereumFeeProxy'. Ensure that Implement the validation: function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner {
+ require(_newEthereumFeeProxy != address(0), "New EthereumFeeProxy address cannot be zero");
ethereumFeeProxy = _newEthereumFeeProxy;
} Committable suggestion
Suggested change
Consider emitting an event when updating 'ethereumFeeProxy'. To improve transparency and facilitate monitoring, emit an event when the 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;
}
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,20 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+1
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM! Consider adding an event emission for better testability. The To enhance testability, consider emitting an event in the + 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function sendFundsBack(address payable _to, uint256 _amount) external { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
(bool success, ) = _to.call{value: _amount}(''); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
require(success, 'Failed to send funds back'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+14
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM! Consider implementing checks-effects-interactions pattern. The 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.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
receive() external payable {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM! Consider adding an event emission for better testability and transparency. The 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,15 @@ | ||||||||||||||
// SPDX-License-Identifier: MIT | ||||||||||||||
// Compatible with OpenZeppelin Contracts ^5.0.0 | ||||||||||||||
pragma solidity ^0.8.9; | ||||||||||||||
|
||||||||||||||
import '@openzeppelin/contracts/token/ERC20/ERC20.sol'; | ||||||||||||||
import '@openzeppelin/contracts/access/Ownable.sol'; | ||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||
|
||||||||||||||
function mint(address to, uint256 amount) public onlyOwner { | ||||||||||||||
_mint(to, amount); | ||||||||||||||
} | ||||||||||||||
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mint function is correct, but consider adding an event emission. The mint function is correctly implemented with the 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.
|
||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Consider adding explanatory comments.
The implementation for the
SingleRequestProxyFactory
case is well-structured and consistent with other cases in the file. It correctly handles the network parameter requirement and retrieves the necessary addresses.Consider adding a comment explaining the purpose of these addresses in the constructor, for example:
Also, please verify if the order of addresses in the return array is correct (ethereumFeeProxy first, then erc20FeeProxy) as intended for the
SingleRequestProxyFactory
constructor.