feat(Escrow): cap the amount #88
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for kleros-escrow-v2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (1)
contracts/src/EscrowUniversal.sol (1)
40-40: Consider adding an event for cap changes.While the mapping implementation is correct, emitting an event when caps are modified would improve transparency and make it easier for off-chain services to track changes.
// Add this event declaration +event AmountCapUpdated(IERC20 indexed token, uint256 oldCap, uint256 newCap);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/EscrowUniversal.sol(5 hunks)contracts/src/interfaces/IEscrow.sol(1 hunks)
🔇 Additional comments (3)
contracts/src/interfaces/IEscrow.sol (1)
183-183: LGTM! Error declaration follows interface conventions.
The new error AmountExceedsCap is appropriately named and placed in the errors section.
contracts/src/EscrowUniversal.sol (2)
139-139: LGTM! Consistent application of cap validation.
The ShouldNotExceedCap modifier is correctly applied to both transaction creation functions, ensuring consistent cap validation for both native and ERC20 transactions.
Also applies to: 166-166
51-54: Verify NATIVE constant implementation.
The modifier looks correct, but ensure that the NATIVE constant is properly defined and initialized, as it's used in createNativeTransaction but not visible in the provided code.
✅ Verification successful
Let me check the actual implementation of the NATIVE constant.
Let me verify how this NATIVE constant is used in the EscrowUniversal contract.
NATIVE constant is properly implemented and used correctly.
The NATIVE constant is correctly defined in Types.sol as IERC20 constant NATIVE = IERC20(address(0)) and is properly used throughout the EscrowUniversal contract for handling native currency transactions. The modifier ShouldNotExceedCap correctly handles both native and ERC20 token cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NATIVE constant definition
rg -l "NATIVE\s*=|constant\s+NATIVE|immutable\s+NATIVE"
Length of output: 157
Script:
#!/bin/bash
# Check the content of Types.sol for NATIVE constant definition
cat contracts/src/interfaces/Types.sol
Length of output: 1390
Script:
#!/bin/bash
# Search for NATIVE usage in EscrowUniversal.sol
cat contracts/src/EscrowUniversal.sol
Length of output: 25166
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
contracts/src/EscrowUniversal.sol (1)
40-40: Enhance mapping documentation for clarity.Consider adding more detailed documentation to explain the cap behavior, especially for edge cases like zero values.
- mapping(IERC20 => uint256) public amountCaps; // Caps the amount of the respective token for the Escrow transaction. + /// @notice Maps tokens to their maximum allowed transaction amounts + /// @dev A cap of 0 means no limit is enforced for that token + /// @return The maximum amount allowed for a given token in a single transaction + mapping(IERC20 => uint256) public amountCaps;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/EscrowUniversal.sol(5 hunks)
🔇 Additional comments (3)
contracts/src/EscrowUniversal.sol (3)
51-54: LGTM! Clean and efficient implementation.
The modifier correctly implements the capping logic with:
- Proper handling of uncapped tokens (zero cap value)
- Gas-efficient check
- Appropriate error usage
139-139: LGTM! Consistent implementation of caps across transaction types.
The cap enforcement is properly integrated into both native and ERC20 transaction creation functions, maintaining consistency in the capping behavior.
Also applies to: 166-166
126-128:
Add input validation and event emission for cap changes.
The governance function should include basic safety checks and emit events for transparency.
As mentioned in the previous review, please add:
- Input validation for the token address
- Event emission for cap changes
+ event AmountCapUpdated(IERC20 indexed token, uint256 oldCap, uint256 newCap);
+
function changeAmountCap(IERC20 _token, uint256 _amountCap) external onlyByGovernor {
+ if (address(_token) == address(0)) revert InvalidToken();
+ uint256 oldCap = amountCaps[_token];
amountCaps[_token] = _amountCap;
+ emit AmountCapUpdated(_token, oldCap, _amountCap);
}
Initially planned to create a separate contract for this feature, but since it's universal escrow contract it might be better to put it directly there as an option
PR-Codex overview
This PR introduces a new error handling mechanism and a cap on the amount of tokens that can be used in
EscrowUniversaltransactions. It adds functionality to manage these caps and ensures that transactions do not exceed specified limits.Detailed summary
AmountExceedsCap()error inIEscrow.sol.amountCapsmapping inEscrowUniversalto set caps for token amounts.shouldNotExceedCapmodifier to enforce the cap on token amounts during transactions.changeAmountCapfunction for updating the cap on specific tokens.shouldNotExceedCapmodifier topayableand token transfer functions to enforce limits.Summary by CodeRabbit
New Features
Bug Fixes