feat: add duration param to approve method #1
Conversation
WalkthroughThe changes involve adding a new Changes
Sequence DiagramsequenceDiagram
participant Caller
participant ExchangeProxy
participant Exchange
Caller->>ExchangeProxy: approve(msgType, spendLimit, duration)
ExchangeProxy->>Exchange: approve(address, methods, spendLimit, duration)
Exchange-->>ExchangeProxy: authorization result
ExchangeProxy-->>Caller: success status
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 (
|
![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 comments (1)
ExchangeProxy.sol (1)
Line range hint
16-30: Add duration parameter validation and specific error handling.The implementation should validate the duration parameter and handle duration-specific errors separately.
function approve( ExchangeTypes.MsgType msgType, Cosmos.Coin[] memory spendLimit, uint256 duration ) external returns (bool success) { + require(duration > 0, "Duration must be greater than 0"); + require(duration <= 365 days, "Duration cannot exceed 365 days"); + ExchangeTypes.MsgType[] memory methods = new ExchangeTypes.MsgType[](1); methods[0] = msgType; try exchange.approve(address(this), methods, spendLimit, duration) returns (bool approved) { return approved; } catch Error(string memory reason) { + if (bytes(reason).length > 0) { + revert(reason); + } revert("error approving msg with spend limit"); } }
🧹 Nitpick comments (1)
Exchange.sol (1)
13-22: Add validation hints for the duration parameter.The documentation should specify:
- Whether duration is relative to current block time
- Any minimum/maximum constraints on the duration value
- What happens when duration expires (e.g., automatic revocation)
/// @dev Approves a list of Cosmos messages. /// @param grantee The account address which will be authorized to spend the origin's funds. /// @param methods The message type URLs of the methods to approve. /// @param spendLimit The spend limit for the methods. - /// @param duration The time period for which the authorization is valid (in seconds). + /// @param duration The time period in seconds, added to current block time, for which the authorization remains valid. + /// Must be greater than 0 and less than MAX_DURATION (e.g., 365 days). + /// The authorization is automatically revoked after expiration. /// @return approved Boolean value to indicate if the approval was successful.
| /// @param duration The time period for which the authorization is valid (in seconds). | ||
| /// @return success Boolean value to indicate if the approval was successful. | ||
| function approve( | ||
| ExchangeTypes.MsgType msgType, | ||
| Cosmos.Coin[] memory spendLimit | ||
| Cosmos.Coin[] memory spendLimit, | ||
| uint256 duration | ||
| ) external returns (bool success) { | ||
| ExchangeTypes.MsgType[] memory methods = new ExchangeTypes.MsgType[](1); | ||
| methods[0] = msgType; | ||
|
|
||
| try exchange.approve(address(this), methods, spendLimit) returns (bool approved) { | ||
| try exchange.approve(address(this), methods, spendLimit, duration) returns (bool approved) { | ||
| return approved; | ||
| } catch { | ||
| revert("error approving msg with spend limit"); |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage for duration parameter is missing and poses security risks
The codebase analysis confirms there are no test cases covering the duration parameter in the approval mechanism. This is a security concern as it leaves authorization expiration and validation untested.
Required test coverage:
- Authorization expiration scenarios
- Invalid duration values (zero, negative, extremely large)
- Attempts to use expired authorizations
- Edge cases around duration boundaries
🔗 Analysis chain
Add test coverage for duration parameter scenarios.
The test implementation should include test cases for:
- Valid and invalid duration values
- Authorization expiration
- Behavior when using expired authorizations
Run this script to check current test coverage:
Would you like me to generate comprehensive test cases for the duration parameter?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for duration parameter
# Look for test cases related to duration
rg -l "test.*duration"
rg -l "test.*expir"
# Look for duration validation tests
rg -l "test.*invalid.*duration"
Length of output: 80
Script:
#!/bin/bash
# Find all test files
echo "=== Test Files ==="
fd -e sol -e js -e ts ".*[tT]est.*"
echo -e "\n=== Approve Tests ==="
# Search for approve function tests with context
rg -A 10 "function.*test.*approve"
echo -e "\n=== Duration Usage ==="
# Look for duration parameter usage in tests
rg "duration.*approve|approve.*duration"
Length of output: 538
Adds the
durationparameter to theapprovemethods.This is expressed in seconds to add to the current block time
Summary by CodeRabbit
New Features
approvefunctionDocumentation
approveandallowancefunctionsdurationparameterRefactor