EscrowView contract and payments refactor #102
Conversation
WalkthroughThe pull request introduces significant modifications to the escrow contract system. The Changes
Possibly related PRs
Suggested labels
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
contracts/src/EscrowUniversal.sol (3)
474-477: Check_rulingtoPartycasting logic for out-of-range values.
Casting_rulingtoPartymay introduce unexpected results if_rulingis outside recognized enum values. Ensure the call site or prior checks safeguard_rulingfrom invalid values.
486-491: Consider reentrancy protection or explicit error handling.
While sending ETH to the addresses is a valid approach, reentrancy concerns can arise. Consider documenting that this contract intentionally uses.sendto avoid blocking in fallback functions, or add a reentrancy guard if that risk is not acceptable.
511-576: Ensure business logic matches the documentation.
ThegetPayoutsfunction encapsulates ruling-based payouts correctly and matches the comment descriptions. Carefully verify that partial settlement amounts are honored correctly (e.g. ifsettlementSelleris zero, default to the fullamount).contracts/src/EscrowView.sol (3)
16-20: Clarify post-dispute payoff logic.
The doc comments are helpful, but you could emphasize that payouts remain subject to final arbitration fees. Consider highlighting that these values may be approximations if arbitration fees are not yet paid in full.
21-29: Leverage struct retrieval for better clarity.
When accessingescrow.transactions(_transactionID), consider using a struct or named variables to reduce indexing. This can help avoid confusion about parameter order and future changes to theTransactionstruct.
111-158: Formatting logic for tokens is consistent, but verify trailing zero approach.
If the resulting decimal portion is fully zero, you correctly revert to whole tokens plus symbol. This is good, but ensure you confirm stakeholder preferences for removing trailing zeros (some might prefer consistent decimal presentation).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/EscrowUniversal.sol(4 hunks)contracts/src/EscrowView.sol(1 hunks)
🔇 Additional comments (7)
contracts/src/EscrowUniversal.sol (4)
13-13: Import usage confirmed.
The newly added import ofIERC20Metadatais appropriate for retrieving token decimals and symbol.
471-473: Avoid using.sendunless accepting reentrancy is intentional.
While these assignments look fine, be aware that usingbuyer.send(...)andseller.send(...)can lead to silent failures if the transfer is declined or runs out of gas. Consider using.transferor.call{value: ...}with appropriate reentrancy protection if you require guaranteed success or automatically reverting on failure.
492-496: Confirm token reverts on failure.
UsingsafeTransferis best practice for ERC20 tokens to revert on transfer failure. This logic is correct and consistent with the contract’s approach to handle tokens that follow ERC20’s standard.
510-510: No issues with getTransactionCount.
Returningtransactions.lengthis straightforward and correct.contracts/src/EscrowView.sol (3)
9-14: Constructor usage validated.
The immutableescrowreference is correctly set in the constructor, ensuring secure linking to theEscrowUniversalcontract.
30-50: ETH payout messages code.
The string concatenations are logically correct. Verify any potential confusion around who shoulders arbitration costs in each scenario to ensure the display text aligns with actual logic inEscrowUniversal.
51-70: ERC20 payout messages code.
This mirrors the ETH version for non-native tokens. Looks consistent.
| function formatEth(uint256 _amountWei) public pure returns (string memory) { | ||
| uint256 ethWhole = _amountWei / 1 ether; | ||
| uint256 ethFraction = (_amountWei % 1 ether) / 1e15; // Get the first 3 decimal digits | ||
|
|
||
| // Convert the whole and fractional parts to strings | ||
| string memory ethWholeStr = Strings.toString(ethWhole); | ||
|
|
||
| // If the fractional part is zero, return only the whole part | ||
| if (ethFraction == 0) { | ||
| return string.concat(ethWholeStr, " ETH"); | ||
| } | ||
|
|
||
| // Convert the fractional part to string with leading zeros if necessary | ||
| string memory ethFractionStr = Strings.toString(ethFraction); | ||
|
|
||
| // Pad the fractional part with leading zeros to ensure three digits | ||
| while (bytes(ethFractionStr).length < 3) { | ||
| ethFractionStr = string.concat("0", ethFractionStr); | ||
| } | ||
|
|
||
| // Remove trailing zeros from the fractional part | ||
| bytes memory fractionBytes = bytes(ethFractionStr); | ||
| uint256 fractionLength = fractionBytes.length; | ||
| while (fractionLength > 0 && fractionBytes[fractionLength - 1] == "0") { | ||
| fractionLength--; | ||
| } | ||
|
|
||
| if (fractionLength == 0) { | ||
| return ethWholeStr; | ||
| } else { | ||
| bytes memory fractionTrimmed = new bytes(fractionLength); | ||
| for (uint256 i = 0; i < fractionLength; i++) { | ||
| fractionTrimmed[i] = fractionBytes[i]; | ||
| } | ||
| return string.concat(ethWholeStr, ".", string(fractionTrimmed), " ETH"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: missing “ ETH” for zero-fraction scenarios that become zero after trimming.
When the fractional part is not zero initially but ends up fully trimmed, line 101 returns ethWholeStr without appending “ ETH,” conflicting with the behavior in line 82. Add “ ETH” to ensure consistent units.
Here is a suggested fix:
...
...
}
...Committable suggestion skipped: line range outside the PR's diff.
jaybuidl
force-pushed
the
feat/payouts-refactor
branch
from
ea3ac2d to
53e8a88
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
contracts/src/EscrowUniversal.sol (1)
533-546: Add explicit comments for settlement amount logic.The settlement amount logic in both buyer and seller winning scenarios could benefit from more explicit comments explaining the rationale behind the calculations.
Add detailed comments explaining the settlement logic:
if (_winningParty == Party.Buyer) { - // The Seller gets the settlement amount proposed by the Buyer if any, otherwise nothing. - // The Buyer gets the remaining amount of the transaction back if any. - // The Buyer gets the arbitration fee back. + // When buyer wins: + // 1. Seller receives the settlement amount that the buyer proposed (if any) + // This represents a "fair" amount the buyer was willing to pay + // 2. Buyer receives: + // - Their arbitration fee back (they won the dispute) + // - The remaining transaction amount (total - settlement) uint256 settledAmount = settlementBuyer; if (nativePayment) {} else if (_winningParty == Party.Seller) { - // The Seller gets his proposed settlement amount if any, otherwise the transaction amount. - // The Buyer gets the remaining amount of the transaction back if any. - // The Seller gets the arbitration fee back. + // When seller wins: + // 1. Seller receives: + // - Their proposed settlement amount (if they made a proposal) + // - Otherwise, the full transaction amount + // - Their arbitration fee back (they won the dispute) + // 2. Buyer receives any remaining amount if seller's settlement was less than total uint256 settledAmount = settlementSeller != 0 ? settlementSeller : amount; if (nativePayment) {Also applies to: 546-559
contracts/src/EscrowView.sol (2)
30-70: Optimize gas usage and improve message consistency.
- Consider extracting repeated string literals to constants to save gas.
- The message wording is inconsistent between ETH and token branches (e.g., "pays for arbitration" placement).
+ // Constants for repeated strings + string private constant BACK_AND_PAY = " back and pay half of the arbitration fees each."; + string private constant INSTEAD_OF = " instead of "; + string private constant BUYER_PAYS_ARBITRATION = ", Buyer pays for arbitration."; + string private constant SELLER_PAYS_ARBITRATION = ", Seller pays for arbitration."; if (token == NATIVE) { noWinner = string.concat( "Buyer and Seller get ", formatEth(noWinnerPayout), - " back and pay half of the arbitration fees each." + BACK_AND_PAY ); // ... similar changes for other messages }
1-159: Consider architectural improvements for better maintainability and gas efficiency.
- Move string constants and formatting logic to a separate library to improve maintainability and reusability.
- Consider implementing a caching mechanism for frequently accessed token metadata to reduce external calls.
Would you like me to provide an example implementation of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/EscrowUniversal.sol(3 hunks)contracts/src/EscrowView.sol(1 hunks)
🔇 Additional comments (4)
contracts/src/EscrowUniversal.sol (2)
510-575: LGTM! Well-structured payout calculation logic.The
getPayoutsfunction is well-designed with:
- Clear separation of native and token payment handling
- Comprehensive coverage of all winning scenarios
- Good handling of settlement amounts
- Proper implementation of the split logic for draws
563-566: Handle potential rounding losses in split calculations.When splitting amounts, any odd number will result in rounding down, effectively burning one unit of currency/token. This should be documented and potentially handled differently.
Consider adding explicit handling for the remainder:
- uint256 splitArbitrationFee = buyerFee / 2; // buyerFee equals sellerFee. - buyerPayout = splitArbitrationFee; - sellerPayout = splitArbitrationFee; - uint256 splitAmount = amount / 2; + uint256 splitArbitrationFee = buyerFee / 2; + uint256 arbitrationFeeRemainder = buyerFee % 2; + buyerPayout = splitArbitrationFee + arbitrationFeeRemainder; // Give remainder to buyer + sellerPayout = splitArbitrationFee; + uint256 splitAmount = amount / 2; + uint256 amountRemainder = amount % 2; + if (nativePayment) { + buyerPayout += splitAmount + amountRemainder; // Give remainder to buyer + sellerPayout += splitAmount; + } else { + buyerPayoutToken = splitAmount + amountRemainder; // Give remainder to buyer + sellerPayoutToken = splitAmount; + }contracts/src/EscrowView.sol (2)
1-14: Consider updating to the latest Solidity version.The contract is using Solidity 0.8.18, while the latest version is 0.8.24. Consider updating to benefit from the latest security fixes and features.
Otherwise, the contract setup looks good with appropriate imports and immutable state variable usage.
73-109: Fix ETH formatting issues and optimize gas usage.
The bug where " ETH" suffix is missing for zero-fraction scenarios is still present.
The string operations could be optimized for gas efficiency:
- while (bytes(ethFractionStr).length < 3) { - ethFractionStr = string.concat("0", ethFractionStr); + bytes memory fractionBytes = bytes(ethFractionStr); + bytes memory paddedFraction = new bytes(3); + uint256 padLength = 3 - fractionBytes.length; + for (uint256 i = 0; i < padLength; i++) { + paddedFraction[i] = bytes1("0"); + } + for (uint256 i = 0; i < fractionBytes.length; i++) { + paddedFraction[i + padLength] = fractionBytes[i]; } + ethFractionStr = string(paddedFraction);
- Add validation for potential overflow:
function formatEth(uint256 _amountWei) public pure returns (string memory) { + require(_amountWei <= type(uint256).max, "EscrowView: Amount too large"); uint256 ethWhole = _amountWei / 1 ether;
| if (buyerPayout > 0) { | ||
| buyer.send(buyerPayout); // It is the user responsibility to accept ETH. | ||
| } |
There was a problem hiding this comment.
Consider using transfer() instead of send() for native currency transfers.
The send() function is used for native currency transfers but its return value is not checked. This could silently fail without reverting the transaction.
Consider using transfer() instead, which automatically reverts on failure:
- buyer.send(buyerPayout); // It is the user responsibility to accept ETH.
+ (bool success, ) = buyer.call{value: buyerPayout}("");
+ require(success, "Native transfer to buyer failed");- seller.send(sellerPayout); // It is the user responsibility to accept ETH.
+ (bool success, ) = seller.call{value: sellerPayout}("");
+ require(success, "Native transfer to seller failed");Also applies to: 488-490
contracts/src/EscrowView.sol
Outdated
| /// @notice Get the payout messages for a transaction once a dispute is created. | ||
| /// @notice The amounts pre-dispute are imprecise as the arbitration fees are not paid yet by either or both parties. | ||
| function getPayoutMessages( | ||
| uint256 _transactionID | ||
| ) external view returns (string memory noWinner, string memory buyerWins, string memory sellerWins) { | ||
| (, , uint256 amount, , , , , , , , , IERC20 token) = escrow.transactions(_transactionID); | ||
|
|
There was a problem hiding this comment.
Add input validation and error handling.
The function should validate the transaction ID and handle non-existent transactions. Consider adding a check to verify that the transaction exists and is in a valid state.
function getPayoutMessages(
uint256 _transactionID
) external view returns (string memory noWinner, string memory buyerWins, string memory sellerWins) {
- (, , uint256 amount, , , , , , , , , IERC20 token) = escrow.transactions(_transactionID);
+ (,, uint256 amount,,,,, Status status,,,, IERC20 token) = escrow.transactions(_transactionID);
+ require(status != Status.NoDispute, "EscrowView: No dispute created yet");Committable suggestion skipped: line range outside the PR's diff.
contracts/src/EscrowView.sol
Outdated
| function formatToken(uint256 _amountWei, address _token) public view returns (string memory) { | ||
| IERC20Metadata token = IERC20Metadata(_token); | ||
| uint8 decimals = token.decimals(); | ||
| string memory symbol = token.symbol(); | ||
|
|
||
| uint256 tenToDecimals = uint256(10) ** uint256(decimals); | ||
| uint256 tokenWhole = _amountWei / tenToDecimals; | ||
|
|
||
| uint256 tokenFraction; | ||
| uint8 fractionDigits; | ||
|
|
||
| if (decimals >= 3) { | ||
| uint256 divider = uint256(10) ** uint256(decimals - 3); | ||
| tokenFraction = (_amountWei % tenToDecimals) / divider; | ||
| fractionDigits = 3; | ||
| } else { | ||
| tokenFraction = _amountWei % tenToDecimals; | ||
| fractionDigits = decimals; | ||
| } | ||
|
|
||
| string memory tokenWholeStr = Strings.toString(tokenWhole); | ||
|
|
||
| if (tokenFraction == 0) { | ||
| return string.concat(tokenWholeStr, " ", symbol); | ||
| } | ||
|
|
||
| string memory tokenFractionStr = Strings.toString(tokenFraction); | ||
|
|
||
| while (bytes(tokenFractionStr).length < fractionDigits) { | ||
| tokenFractionStr = string.concat("0", tokenFractionStr); | ||
| } | ||
|
|
||
| bytes memory fractionBytes = bytes(tokenFractionStr); | ||
| uint256 fractionLength = fractionBytes.length; | ||
| while (fractionLength > 0 && fractionBytes[fractionLength - 1] == bytes1("0")) { | ||
| fractionLength--; | ||
| } | ||
|
|
||
| if (fractionLength == 0) { | ||
| return string.concat(tokenWholeStr, " ", symbol); | ||
| } else { | ||
| bytes memory fractionTrimmed = new bytes(fractionLength); | ||
| for (uint256 i = 0; i < fractionLength; i++) { | ||
| fractionTrimmed[i] = fractionBytes[i]; | ||
| } | ||
| return string.concat(tokenWholeStr, ".", string(fractionTrimmed), " ", symbol); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add safety checks and error handling for token formatting.
Several improvements needed:
- Add null address check
- Handle potential failures in external calls
- Similar gas optimizations as formatEth
function formatToken(uint256 _amountWei, address _token) public view returns (string memory) {
+ require(_token != address(0), "EscrowView: Token address cannot be zero");
+ require(_token.code.length > 0, "EscrowView: Token must be a contract");
+
IERC20Metadata token = IERC20Metadata(_token);
- uint8 decimals = token.decimals();
- string memory symbol = token.symbol();
+
+ // Handle potential revert from non-compliant tokens
+ (bool success1, bytes memory data1) = _token.staticcall(
+ abi.encodeWithSelector(IERC20Metadata.decimals.selector)
+ );
+ require(success1 && data1.length >= 32, "EscrowView: Failed to get decimals");
+ uint8 decimals = abi.decode(data1, (uint8));
+
+ (bool success2, bytes memory data2) = _token.staticcall(
+ abi.encodeWithSelector(IERC20Metadata.symbol.selector)
+ );
+ require(success2 && data2.length > 0, "EscrowView: Failed to get symbol");
+ string memory symbol = abi.decode(data2, (string));📝 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 formatToken(uint256 _amountWei, address _token) public view returns (string memory) { | |
| IERC20Metadata token = IERC20Metadata(_token); | |
| uint8 decimals = token.decimals(); | |
| string memory symbol = token.symbol(); | |
| uint256 tenToDecimals = uint256(10) ** uint256(decimals); | |
| uint256 tokenWhole = _amountWei / tenToDecimals; | |
| uint256 tokenFraction; | |
| uint8 fractionDigits; | |
| if (decimals >= 3) { | |
| uint256 divider = uint256(10) ** uint256(decimals - 3); | |
| tokenFraction = (_amountWei % tenToDecimals) / divider; | |
| fractionDigits = 3; | |
| } else { | |
| tokenFraction = _amountWei % tenToDecimals; | |
| fractionDigits = decimals; | |
| } | |
| string memory tokenWholeStr = Strings.toString(tokenWhole); | |
| if (tokenFraction == 0) { | |
| return string.concat(tokenWholeStr, " ", symbol); | |
| } | |
| string memory tokenFractionStr = Strings.toString(tokenFraction); | |
| while (bytes(tokenFractionStr).length < fractionDigits) { | |
| tokenFractionStr = string.concat("0", tokenFractionStr); | |
| } | |
| bytes memory fractionBytes = bytes(tokenFractionStr); | |
| uint256 fractionLength = fractionBytes.length; | |
| while (fractionLength > 0 && fractionBytes[fractionLength - 1] == bytes1("0")) { | |
| fractionLength--; | |
| } | |
| if (fractionLength == 0) { | |
| return string.concat(tokenWholeStr, " ", symbol); | |
| } else { | |
| bytes memory fractionTrimmed = new bytes(fractionLength); | |
| for (uint256 i = 0; i < fractionLength; i++) { | |
| fractionTrimmed[i] = fractionBytes[i]; | |
| } | |
| return string.concat(tokenWholeStr, ".", string(fractionTrimmed), " ", symbol); | |
| } | |
| } | |
| function formatToken(uint256 _amountWei, address _token) public view returns (string memory) { | |
| require(_token != address(0), "EscrowView: Token address cannot be zero"); | |
| require(_token.code.length > 0, "EscrowView: Token must be a contract"); | |
| IERC20Metadata token = IERC20Metadata(_token); | |
| // Handle potential revert from non-compliant tokens | |
| (bool success1, bytes memory data1) = _token.staticcall( | |
| abi.encodeWithSelector(IERC20Metadata.decimals.selector) | |
| ); | |
| require(success1 && data1.length >= 32, "EscrowView: Failed to get decimals"); | |
| uint8 decimals = abi.decode(data1, (uint8)); | |
| (bool success2, bytes memory data2) = _token.staticcall( | |
| abi.encodeWithSelector(IERC20Metadata.symbol.selector) | |
| ); | |
| require(success2 && data2.length > 0, "EscrowView: Failed to get symbol"); | |
| string memory symbol = abi.decode(data2, (string)); | |
| uint256 tenToDecimals = uint256(10) ** uint256(decimals); | |
| uint256 tokenWhole = _amountWei / tenToDecimals; | |
| uint256 tokenFraction; | |
| uint8 fractionDigits; | |
| if (decimals >= 3) { | |
| uint256 divider = uint256(10) ** uint256(decimals - 3); | |
| tokenFraction = (_amountWei % tenToDecimals) / divider; | |
| fractionDigits = 3; | |
| } else { | |
| tokenFraction = _amountWei % tenToDecimals; | |
| fractionDigits = decimals; | |
| } | |
| string memory tokenWholeStr = Strings.toString(tokenWhole); | |
| if (tokenFraction == 0) { | |
| return string.concat(tokenWholeStr, " ", symbol); | |
| } | |
| string memory tokenFractionStr = Strings.toString(tokenFraction); | |
| while (bytes(tokenFractionStr).length < fractionDigits) { | |
| tokenFractionStr = string.concat("0", tokenFractionStr); | |
| } | |
| bytes memory fractionBytes = bytes(tokenFractionStr); | |
| uint256 fractionLength = fractionBytes.length; | |
| while (fractionLength > 0 && fractionBytes[fractionLength - 1] == bytes1("0")) { | |
| fractionLength--; | |
| } | |
| if (fractionLength == 0) { | |
| return string.concat(tokenWholeStr, " ", symbol); | |
| } else { | |
| bytes memory fractionTrimmed = new bytes(fractionLength); | |
| for (uint256 i = 0; i < fractionLength; i++) { | |
| fractionTrimmed[i] = fractionBytes[i]; | |
| } | |
| return string.concat(tokenWholeStr, ".", string(fractionTrimmed), " ", symbol); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
contracts/deploy/00-escrow.ts (1)
51-56: Optimize the gas limit for EscrowView deployment.The gas limit of 50000000 seems unnecessarily high for a view contract deployment, which typically requires much less gas.
await deploy("EscrowView", { from: deployer, args: [escrow.target], - gasLimit: 50000000, + gasLimit: 1000000, log: true, });contracts/scripts/getPayoutMessages.ts (3)
1-4: Remove commented import.The commented import statement should be either used or removed to maintain clean code.
-// import { BigNumber, utils } from "ethers"; import { task } from "hardhat/config"; import { EscrowUniversal, EscrowView } from "../typechain-types";
13-22: Parameterize hardcoded values.Consider extracting hardcoded values like the IPFS hash and ETH amount into configurable parameters.
+const DEFAULT_IPFS_HASH = "/ipfs/QmWRTngdTzTTq89L8r7XCwbRHrmuaNqzmvW7Cqk3s1FS9C"; +const DEFAULT_TRANSACTION_VALUE = "0.5"; try { await escrow.createNativeTransaction( deadline, - "/ipfs/QmWRTngdTzTTq89L8r7XCwbRHrmuaNqzmvW7Cqk3s1FS9C", + DEFAULT_IPFS_HASH, seller.address, { - value: ethers.parseEther("0.5"), + value: ethers.parseEther(DEFAULT_TRANSACTION_VALUE), } );
63-65: Enhance error handling.The current error handling simply logs the error. Consider adding error classification and appropriate handling strategies.
} catch (error) { - console.log(error); + if (error instanceof Error) { + console.error(`Error executing payout messages test: ${error.message}`); + if (error.stack) console.debug(error.stack); + } else { + console.error("An unknown error occurred:", error); + } + process.exit(1); }contracts/deployments/arbitrumSepoliaDevnet/DAI.json (1)
2-2: Verify test DAI deployment configuration.The test DAI contract is deployed with:
- Initial supply: 1,000,000 tokens
- Name and symbol both set to "DAI"
Consider using a different symbol (e.g., "tDAI") to clearly indicate this is a test token.Also applies to: 291-325
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.yarn/releases/yarn-4.2.2.cjsis excluded by!**/.yarn/**.yarn/releases/yarn-4.6.0.cjsis excluded by!**/.yarn/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.yarnrc.yml(1 hunks)contracts/deploy/00-escrow.ts(3 hunks)contracts/deployments/arbitrum/.chainId(1 hunks)contracts/deployments/arbitrum/DAI.json(1 hunks)contracts/deployments/arbitrum/WETH.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/DAI.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/WETH.json(1 hunks)contracts/hardhat.config.ts(6 hunks)contracts/package.json(2 hunks)contracts/scripts/getPayoutMessages.ts(1 hunks)contracts/src/EscrowView.sol(1 hunks)package.json(1 hunks)prettier-config/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- contracts/deployments/arbitrum/.chainId
- prettier-config/package.json
- .yarnrc.yml
- contracts/deployments/arbitrumSepoliaDevnet/WETH.json
🔇 Additional comments (11)
contracts/deploy/00-escrow.ts (2)
4-4: LGTM! Enhanced type safety with explicit import.The addition of the EscrowUniversal type import improves type checking and IDE support.
38-38: LGTM! Improved contract retrieval with type assertion.The change to use ethers.getContract with explicit type assertion enhances type safety.
contracts/hardhat.config.ts (2)
20-21: LGTM! Improved account management.The consolidated account configuration enhances maintainability and flexibility.
27-27: Verify impact of viaIR setting.The viaIR setting enables the new IR-based codegen pipeline, which might affect compilation results.
✅ Verification successful
viaIR setting is properly configured and safe to use
The viaIR setting is correctly configured alongside the optimizer in a stable Solidity version (0.8.18). This combination is recommended for production use as it can enable better optimization opportunities. The successful contract deployments confirm the compilation pipeline is working as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if viaIR affects contract size or gas usage # Compare contract sizes echo "Checking contract sizes..." npx hardhat size-contracts # Check gas usage echo "Checking gas usage..." REPORT_GAS=true npx hardhat testLength of output: 370
Script:
#!/bin/bash # Search for viaIR mentions and related optimization settings echo "Searching for viaIR mentions..." rg -i "viaIR" -A 3 -B 3 echo -e "\nChecking for optimization settings..." rg -i "optimization|optimizer" -A 3 -B 3 echo -e "\nChecking documentation files..." fd -e md -e txt -x cat {}Length of output: 77443
package.json (1)
26-29: LGTM! Consistent package manager configuration.The updates to packageManager and volta configuration maintain consistency in dependency management.
contracts/deployments/arbitrum/WETH.json (1)
2-2: Verify the WETH contract address on Arbitrum.The contract address matches the official Arbitrum WETH contract. However, let's verify its implementation and admin controls.
✅ Verification successful
WETH contract implementation and admin controls verified
The contract at 0x82aF49447D8a07e3bd95BD0d56f35241523fBab1 is the official Arbitrum WETH implementation:
- Uses standard WETH9 implementation with expected functionality
- Protected by transparent proxy pattern with standard admin controls
- Matches the canonical L2 bridge deployment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify WETH contract implementation and admin on Arbitrum # Check contract code and admin curl -s -X POST "https://arb1.arbitrum.io/rpc" \ -H "Content-Type: application/json" \ -d '{ "jsonrpc":"2.0", "method":"eth_getCode", "params":["0x82aF49447D8a07e3bd95BD0d56f35241523fBab1", "latest"], "id":1 }'Length of output: 4447
Script:
#!/bin/bash # Get implementation and admin addresses from storage slots # Implementation slot: 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc curl -s -X POST "https://arb1.arbitrum.io/rpc" \ -H "Content-Type: application/json" \ -d '{ "jsonrpc":"2.0", "method":"eth_getStorageAt", "params":["0x82aF49447D8a07e3bd95BD0d56f35241523fBab1", "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc", "latest"], "id":1 }' # Admin slot: 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103 curl -s -X POST "https://arb1.arbitrum.io/rpc" \ -H "Content-Type: application/json" \ -d '{ "jsonrpc":"2.0", "method":"eth_getStorageAt", "params":["0x82aF49447D8a07e3bd95BD0d56f35241523fBab1", "0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103", "latest"], "id":1 }'Length of output: 805
Script:
#!/bin/bash # Get implementation contract bytecode curl -s -X POST "https://arb1.arbitrum.io/rpc" \ -H "Content-Type: application/json" \ -d '{ "jsonrpc":"2.0", "method":"eth_getCode", "params":["0x8b194beae1d3e0788a1a35173978001acdfba668", "latest"], "id":1 }'Length of output: 17439
contracts/package.json (1)
14-14: Consider using a newer Node.js version.The Node.js version has been downgraded from 20.11.0 to 18.20.6. While Node.js 18 is still in LTS, consider using Node.js 20 for:
- Better performance
- Latest security updates
- Newer features
contracts/src/EscrowView.sol (3)
85-121: Bug: Missing " ETH" for zero-fraction scenarios.When the fractional part is not zero initially but ends up fully trimmed, line 113 returns
ethWholeStrwithout appending " ETH", conflicting with the behavior in line 94.
127-175: Add safety checks and error handling for token formatting.The function needs additional safety checks:
- Null address check
- Handle potential failures in external calls
- Similar gas optimizations as formatEth
26-80:⚠️ Potential issueAdd input validation and error handling for getPayoutMessages.
The function should validate the transaction ID and handle non-existent transactions. Also, consider adding a check for the dispute status.
Apply this diff to add validation:
function getPayoutMessages( uint256 _transactionID ) external view returns (string memory noWinner, string memory buyerWins, string memory sellerWins) { + (, , , , , , , Status status, , , , IERC20 token) = escrow.transactions(_transactionID); + require(status != Status.NoDispute, "EscrowView: No dispute created yet"); + require(status != Status.Resolved, "EscrowView: Dispute already resolved"); (, , uint256 amount, , , , , uint256 buyerFee, uint256 sellerFee, , , IERC20 token) = escrow.transactions( _transactionID );Likely invalid or redundant comment.
contracts/deployments/arbitrum/DAI.json (1)
2-2: Verify the DAI contract address on Arbitrum.The contract address matches the official Arbitrum DAI contract. Let's verify its implementation.
✅ Verification successful
✓ DAI contract address and implementation verified on Arbitrum
The address matches the official DAI token contract on Arbitrum, and bytecode analysis confirms this is the legitimate MakerDAO DAI implementation with all standard functionality and security features intact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify DAI contract implementation on Arbitrum # Check contract code and verify it's the official DAI implementation curl -s -X POST "https://arb1.arbitrum.io/rpc" \ -H "Content-Type: application/json" \ -d '{ "jsonrpc":"2.0", "method":"eth_getCode", "params":["0xDA10009cBd5D07dd0CeCc66161FC93D7c9000da1", "latest"], "id":1 }'Length of output: 10115
| console.log("Seller proposes settlement"); | ||
| await escrow.connect(seller).proposeSettlement(txId, ethers.parseEther("0.4")); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 3000)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace setTimeout with network time manipulation.
Using setTimeout for testing is unreliable. Consider using hardhat's time manipulation utilities instead.
- await new Promise((resolve) => setTimeout(resolve, 3000));
+ await ethers.provider.send("evm_increaseTime", [3]);
+ await ethers.provider.send("evm_mine", []);📝 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 new Promise((resolve) => setTimeout(resolve, 3000)); | |
| await ethers.provider.send("evm_increaseTime", [3]); | |
| await ethers.provider.send("evm_mine", []); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/scripts/generateDeploymentsMarkdown.sh (1)
9-10: LGTM! Consider grouping related artifacts.The addition of DAI and WETH to ignored artifacts is appropriate as they are external dependencies.
Consider grouping related artifacts together for better maintainability:
IGNORED_ARTIFACTS=( "CREATE3Factory.json" "MetaEvidence_*" "PNK.json" + # External token contracts "DAI.json" "WETH.json" "RandomizerOracle.json" "_Implementation.json" "_Proxy.json" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/README.md(1 hunks)contracts/scripts/generateDeploymentsMarkdown.sh(1 hunks)
🔇 Additional comments (1)
contracts/README.md (1)
19-20: LGTM! Verify contract deployments on Arbiscan.The contract addresses and links are correctly formatted. Let's verify the contract deployments:
PR-Codex overview
This PR introduces a new
EscrowViewcontract that provides a view for payout messages related to transactions in theEscrowUniversalcontract. It also refines the payout logic inEscrowUniversalto streamline the execution of rulings.Detailed summary
EscrowViewcontract with a constructor accepting anEscrowUniversaladdress.getPayoutMessagesfunction to retrieve payout messages based on transaction ID.formatEthandformatTokenfunctions for formatting amounts.executeRulinginEscrowUniversalto utilize new payout logic.getPayoutsfunction inEscrowUniversalfor calculating payouts based on the winning party.Summary by CodeRabbit
New Features
Improvements
EscrowUniversaland addedEscrowView.