feat(Escrow): add custom buyer option #117
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for kleros-escrow-v2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughA new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant EscrowUniversal
participant ERC20Token
Caller->>ERC20Token: approve(EscrowUniversal, amount)
Caller->>EscrowUniversal: createERC20TransactionCustomBuyer(amount, token, deadline, uri, buyer, seller)
EscrowUniversal->>ERC20Token: transferFrom(Caller, EscrowUniversal, amount)
EscrowUniversal-->>Caller: returns transactionID
EscrowUniversal-->>All: emit ERC20TransactionCreated(transactionID, buyer, seller, ...)
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/src/EscrowUniversal.sol (1)
189-217: Consider access control implications of custom buyer functionality.The current implementation allows any caller to create a transaction with arbitrary buyer and seller addresses. This means:
- Unauthorized transaction creation: Anyone can create transactions assigning buyer/seller roles to any addresses
- Potential confusion: The buyer might not be aware a transaction was created on their behalf
- Token responsibility: The caller provides tokens but loses control to the designated buyer
Consider implementing one of these approaches:
Option 1: Require buyer authorization
mapping(address => mapping(address => bool)) public authorizedCreators; function authorizeBuyerCreation(address _creator, bool _authorized) external { authorizedCreators[msg.sender][_creator] = _authorized; } // In createERC20TransactionCustomBuyer: if (_buyer != msg.sender && !authorizedCreators[_buyer][msg.sender]) { revert UnauthorizedBuyerCreation(); }Option 2: Restrict to specific roles
mapping(address => bool) public trustedCreators; modifier onlyTrustedCreator() { if (!trustedCreators[msg.sender]) revert NotTrustedCreator(); _; } // Add modifier to function function createERC20TransactionCustomBuyer(...) external onlyTrustedCreator shouldNotExceedCap(_token, _amount) returns (uint256 transactionID) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/foundry.toml(1 hunks)contracts/src/EscrowUniversal.sol(1 hunks)contracts/test/EscrowUniversal.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Redirect rules - kleros-escrow-v2
- GitHub Check: Header rules - kleros-escrow-v2
- GitHub Check: Redirect rules - kleros-escrow-v2
- GitHub Check: Header rules - kleros-escrow-v2
- GitHub Check: Pages changed - kleros-escrow-v2
- GitHub Check: Pages changed - kleros-escrow-v2
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
contracts/foundry.toml (1)
5-5: Verify the necessity of enabling IR compilation.The
via_ir = truesetting enables intermediate representation compilation mode. Please confirm whether this change is required for the newcreateERC20TransactionCustomBuyerfunctionality or if it addresses a specific compilation issue.#!/bin/bash # Check if there are any compilation issues without via_ir flag cd contracts # Temporarily remove via_ir setting sed -i.bak 's/via_ir = true/# via_ir = true/' foundry.toml # Try to compile forge build 2>&1 | grep -E "(error|Error|ERROR)" || echo "Compilation successful without via_ir" # Restore original mv foundry.toml.bak foundry.tomlcontracts/test/EscrowUniversal.t.sol (1)
420-424: Well-structured test with correct balance assertions.The test correctly verifies that:
- The escrow contract receives the tokens
- The transaction creator (buyer) retains remaining balance
- The custom buyer has no token balance (as expected since they didn't provide tokens)
- All transaction fields are properly set
Comment on lines
+189
to
+217
| function createERC20TransactionCustomBuyer( | ||
| uint256 _amount, | ||
| IERC20 _token, | ||
| uint256 _deadline, | ||
| string memory _transactionUri, | ||
| address payable _buyer, | ||
| address payable _seller | ||
| ) external shouldNotExceedCap(_token, _amount) returns (uint256 transactionID) { | ||
| // Transfers token from sender wallet to contract. Note that the amount is provided by the caller, not by the buyer. | ||
| if (!_token.safeTransferFrom(msg.sender, address(this), _amount)) revert TokenTransferFailed(); | ||
| Transaction storage transaction = transactions.push(); | ||
| transaction.buyer = _buyer; | ||
| transaction.seller = _seller; | ||
| transaction.amount = _amount; | ||
| transaction.token = _token; | ||
| transaction.deadline = _deadline; | ||
|
|
||
| transactionID = transactions.length - 1; | ||
|
|
||
| emit ERC20TransactionCreated( | ||
| transactionID, | ||
| _transactionUri, | ||
| _buyer, | ||
| _seller, | ||
| _token, | ||
| _amount, | ||
| transaction.deadline | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for custom buyer address and consider security implications.
The new function allows specifying a custom buyer address, which introduces several considerations:
- Zero address validation: The function should validate that
_buyeris not the zero address - Same address validation: Consider whether
_buyerand_sellershould be allowed to be the same address - Documentation: The function lacks proper NatSpec documentation explaining the custom buyer functionality
Apply this diff to add necessary validations and documentation:
+ /// @dev Creates an ERC20 escrow transaction with custom buyer and seller addresses.
+ /// @param _amount The amount of tokens to be escrowed.
+ /// @param _token The ERC20 token contract.
+ /// @param _deadline The deadline for the transaction.
+ /// @param _transactionUri The URI for transaction metadata.
+ /// @param _buyer The address that will be set as the buyer (different from caller).
+ /// @param _seller The address that will be set as the seller.
+ /// @return transactionID The ID of the created transaction.
function createERC20TransactionCustomBuyer(
uint256 _amount,
IERC20 _token,
uint256 _deadline,
string memory _transactionUri,
address payable _buyer,
address payable _seller
) external shouldNotExceedCap(_token, _amount) returns (uint256 transactionID) {
+ if (_buyer == address(0)) revert ZeroBuyerAddress();
+ if (_seller == address(0)) revert ZeroSellerAddress();
+ if (_buyer == _seller) revert BuyerAndSellerCannotBeSame();
// Transfers token from sender wallet to contract. Note that the amount is provided by the caller, not by the buyer.Also, add the corresponding error definitions at the top of the contract:
+ error ZeroBuyerAddress();
+ error ZeroSellerAddress();
+ error BuyerAndSellerCannotBeSame();🤖 Prompt for AI Agents
In contracts/src/EscrowUniversal.sol around lines 189 to 217, the
createERC20TransactionCustomBuyer function lacks validation for the _buyer
address and proper documentation. Add a require statement to ensure _buyer is
not the zero address, and consider adding a check or comment regarding whether
_buyer and _seller can be the same address based on your security model. Also,
add NatSpec comments above the function to explain its purpose and parameters.
Finally, define any new error types used for validation at the top of the
contract.
Comment on lines
+383
to
+424
| function test_createERC20TransactionCustomBuyer() public { | ||
| uint256 deadline = block.timestamp + txTimeout; | ||
| uint256 txId; | ||
| vm.prank(buyer); | ||
| vm.expectEmit(true, true, true, true); | ||
| // Pay a bit less to differ between tests | ||
| emit ERC20TransactionCreated(txId, txUri, other, seller, escrowToken, 0.4 ether, deadline); | ||
| escrow.createERC20TransactionCustomBuyer(0.4 ether, escrowToken, deadline, txUri, payable(other), payable(seller)); | ||
|
|
||
| ( | ||
| address escrowBuyer, | ||
| address escrowSeller, | ||
| uint256 amount, | ||
| uint256 settlementBuyer, | ||
| uint256 settlementSeller, | ||
| uint256 escrowdeadline, | ||
| uint256 disputeId, | ||
| uint256 buyerFee, | ||
| uint256 sellerFee, | ||
| uint256 lastFeePaymentTime, | ||
| Status status, | ||
| IERC20 token | ||
| ) = escrow.transactions(txId); | ||
|
|
||
| assertEq(escrowBuyer, other, "Wrong custom buyer address"); | ||
| assertEq(escrowSeller, seller, "Wrong seller address"); | ||
| assertEq(amount, 0.4 ether, "Wrong escrow amount"); | ||
| assertEq(settlementBuyer, 0, "settlementBuyer should be 0"); | ||
| assertEq(settlementSeller, 0, "settlementSeller should be 0"); | ||
| assertEq(escrowdeadline, block.timestamp + 1000, "Wrong deadline"); | ||
| assertEq(disputeId, 0, "disputeId should be 0"); | ||
| assertEq(sellerFee, 0, "sellerFee should be 0"); | ||
| assertEq(lastFeePaymentTime, 0, "lastFeePaymentTime should be 0"); | ||
| assertEq(uint256(status), uint256(Status.NoDispute), "Wrong status"); | ||
| assertEq(address(token), address(escrowToken), "Wrong token address"); | ||
| assertEq(escrow.getTransactionCount(), 1, "Wrong transaction count"); | ||
|
|
||
| assertEq(escrowToken.balanceOf(address(escrow)), 0.4 ether, "Wrong token balance of the escrow"); | ||
| assertEq(escrowToken.balanceOf(buyer), 0.6 ether, "Wrong token balance of buyer"); // tx creator | ||
| assertEq(escrowToken.balanceOf(seller), 0, "Balance of seller should be 0"); | ||
| assertEq(escrowToken.balanceOf(other), 0, "Balance of custom buyer should be 0"); // custom buyer did not have balance of his own | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for edge cases and security scenarios.
The test covers the basic happy path but lacks coverage for important edge cases:
Add these additional test cases to ensure comprehensive coverage:
function test_createERC20TransactionCustomBuyer_zeroAddresses() public {
uint256 deadline = block.timestamp + txTimeout;
// Test zero buyer address
vm.expectRevert(IEscrow.ZeroBuyerAddress.selector);
vm.prank(buyer);
escrow.createERC20TransactionCustomBuyer(
0.4 ether,
escrowToken,
deadline,
txUri,
payable(address(0)),
payable(seller)
);
// Test zero seller address
vm.expectRevert(IEscrow.ZeroSellerAddress.selector);
vm.prank(buyer);
escrow.createERC20TransactionCustomBuyer(
0.4 ether,
escrowToken,
deadline,
txUri,
payable(other),
payable(address(0))
);
}
function test_createERC20TransactionCustomBuyer_sameBuyerSeller() public {
uint256 deadline = block.timestamp + txTimeout;
vm.expectRevert(IEscrow.BuyerAndSellerCannotBeSame.selector);
vm.prank(buyer);
escrow.createERC20TransactionCustomBuyer(
0.4 ether,
escrowToken,
deadline,
txUri,
payable(other),
payable(other)
);
}🤖 Prompt for AI Agents
In contracts/test/EscrowUniversal.t.sol around lines 383 to 424, the existing
test only covers the basic successful scenario for
createERC20TransactionCustomBuyer. To improve test coverage and security, add
two new test functions: one to check that the function reverts when the buyer or
seller address is zero, and another to verify it reverts when the buyer and
seller addresses are the same. Use vm.expectRevert with the appropriate error
selectors before calling createERC20TransactionCustomBuyer with these invalid
inputs to ensure the contract correctly handles these edge cases.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR-Codex overview
This PR introduces a new function
createERC20TransactionCustomBuyerin theEscrowUniversalcontract, allowing a custom buyer to create ERC20 transactions. It also updates thefoundry.tomlconfiguration and adds a corresponding test case.Detailed summary
via_ir = trueinfoundry.toml.createERC20TransactionCustomBuyerfunction inEscrowUniversal.sol.test_createERC20TransactionCustomBuyerinEscrowUniversal.t.solto verify the new functionality.Summary by CodeRabbit