Skip to content

feat(Escrow): add custom buyer option #117

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

feat(Escrow): add custom buyer option #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion contracts/foundry.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
[profile.default]
src = 'src'
out = 'out'
libs = ['../node_modules', 'lib']
libs = ['../node_modules', 'lib']
via_ir = true
30 changes: 30 additions & 0 deletions contracts/src/EscrowUniversal.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,36 @@ contract EscrowUniversal is IEscrow, IArbitrableV2 {
);
}

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
);
}
Comment on lines +189 to +217
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Zero address validation: The function should validate that _buyer is not the zero address
  2. Same address validation: Consider whether _buyer and _seller should be allowed to be the same address
  3. 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.


/// @inheritdoc IEscrow
function pay(uint256 _transactionID, uint256 _amount) external override {
Transaction storage transaction = transactions[_transactionID];
Expand Down
43 changes: 43 additions & 0 deletions contracts/test/EscrowUniversal.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,49 @@ contract EscrowUniversalTest is Test {
assertEq(escrowToken.balanceOf(seller), 0, "Balance of seller should be 0");
}

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
}
Comment on lines +383 to +424
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


function test_createNativeTransaction_approvalMissing() public {
uint256 deadline = block.timestamp + txTimeout;

Expand Down