refactor(core-token-vesting-v2)!: remove recipient #139
Conversation
WalkthroughThe changes involve refining the functionality of the Changes
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
k-yang
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- contracts/core-token-vesting-v2/src/contract.rs (7 hunks)
- contracts/core-token-vesting-v2/src/msg.rs (1 hunks)
- contracts/core-token-vesting-v2/src/testing.rs (4 hunks)
Additional comments: 3
contracts/core-token-vesting-v2/src/msg.rs (1)
- 35-45: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [80-81]
The removal of the
recipientfield from theClaimandWithdrawvariants in theExecuteMsgenum aligns with the PR's objectives to simplify the contract's logic. Ensure that this change has been thoroughly tested, especially in scenarios where the sender's defaulting as the recipient could affect the contract's intended functionality or security.contracts/core-token-vesting-v2/src/contract.rs (2)
- 90-104: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [80-114]
The changes in the
withdrawfunction, including usinginfo.senderas the recipient and the security checks, align with the PR's objectives to simplify the contract's logic. Ensure that these changes have been thoroughly tested, especially the admin check and the calculation of the maximum amount that can be withdrawn, to guarantee the function's robustness and correctness.
- 353-366: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [357-400]
The updates to the
claimfunction, including usinginfo.senderas the recipient and simplifying the handling of vesting accounts, align with the PR's objectives. Ensure that these changes have been thoroughly tested, particularly in handling edge cases related to vesting accounts, to ensure the function's correctness and robustness.
|
|
||
| // unauthorized sender | ||
| let msg = ExecuteMsg::Withdraw { | ||
| recipient: "addr0000".to_string(), | ||
| amount: Uint128::new(1000), | ||
| }; | ||
| require_error( |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [429-465]
The test test_withdraw correctly reflects the changes made to the ExecuteMsg::Withdraw message by removing the recipient parameter. However, it's important to ensure that the test cases cover scenarios where the sender is implicitly the recipient of the withdrawal. This is a key part of the PR's objectives, and the tests should explicitly verify that the funds are indeed sent to the info.sender address.
Additionally, the test includes a scenario where an unauthorized sender attempts to withdraw funds, which is good for verifying access control. However, it would be beneficial to add a test case that verifies the correct behavior when the sender attempts to withdraw an amount greater than their unallocated balance, ensuring that the contract logic correctly handles such cases.
Consider adding explicit assertions to verify that the withdrawn funds are sent to the info.sender address, as this is a critical aspect of the PR's objectives. Also, ensure that edge cases, such as attempting to withdraw more than the available unallocated balance, are adequately tested.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [467-567]
The claim_native test function has been updated to reflect the removal of the recipient parameter from the ExecuteMsg::Claim message. This test function is crucial for verifying that the claim functionality works as expected, particularly that the sender (who is also the claimant in this context) receives the claimed funds.
While the test does cover scenarios where the claim is made at different times to verify the amount claimable based on the vesting schedule, it would be beneficial to explicitly assert that the funds are sent to the info.sender address. Given the PR's objectives, ensuring that the claim functionality defaults the recipient to the sender is essential.
Enhance the claim_native test by adding explicit assertions to confirm that the claimed funds are sent to the info.sender address. This will ensure that the test suite fully covers the changes made to the claim functionality as per the PR's objectives.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
|
| ExecuteMsg::Withdraw { amount, recipient } => { | ||
| withdraw(deps, env, info, amount, recipient) | ||
| } | ||
| ExecuteMsg::Claim { denoms: _denoms } => claim(deps, env, info), |
There was a problem hiding this comment.
If we do breaking changes for the interface we can get rid of the denom parameter also
There was a problem hiding this comment.
Removing denoms is a breaking change for the UI because it's a mandatory field, but removing recipient is not because it's an Optional field, meaning the client doesn't have to send it.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.lockis excluded by:!**/*.lockcontracts/core-token-vesting-v2/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (3)
- contracts/core-token-vesting-v2/src/contract.rs (8 hunks)
- contracts/core-token-vesting-v2/src/msg.rs (2 hunks)
- contracts/core-token-vesting-v2/src/testing.rs (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- contracts/core-token-vesting-v2/src/contract.rs
- contracts/core-token-vesting-v2/src/msg.rs
- contracts/core-token-vesting-v2/src/testing.rs
matthiasmatt
deleted the
refactor/core-token-vesting-v2/remove-recipient
branch
Claimmsg and default to senderWithdrawmsg and default to sender (who is also the admin)BREAKING CHANGE: API breaking for
Withdraw, but not forClaimbecauserecipientwas an Optional field. Doesn't matter forWithdrawbecause only the admin calls it on the backend.Summary by CodeRabbit