feat: remove vesting contract staking feature #554
Conversation
WalkthroughThe recent changes involve significant modifications to the Andromeda vesting contract, including the removal of delegation, voting, and staking functionalities. The version of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
User->>Contract: Create Batch
Contract-->>User: Batch Created
User->>Contract: Claim Batch
Contract-->>User: Batch Claimed
Possibly related PRs
Tip New featuresWalkthrough comment now includes:
Notes:
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- contracts/finance/andromeda-vesting/Cargo.toml (1 hunks)
- contracts/finance/andromeda-vesting/src/contract.rs (7 hunks)
- contracts/finance/andromeda-vesting/src/testing/tests.rs (26 hunks)
- packages/andromeda-finance/src/vesting.rs (3 hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional context used
Learnings (1)
contracts/finance/andromeda-vesting/src/contract.rs (1)
Learnt from: cowboy0015 PR: andromedaprotocol/andromeda-core#466 File: contracts/non-fungible-tokens/andromeda-crowdfund/src/contract.rs:0-0 Timestamp: 2024-05-20T19:05:20.474Z Learning: The `execute_claim` function in the Andromeda Crowdfund project has been refactored into `handle_successful_claim` and `handle_failed_claim` sub-functions to improve clarity and maintainability.
Additional comments not posted (25)
contracts/finance/andromeda-vesting/Cargo.toml (1)
3-3: Verify the impact of the version change on the dependent packages.The version change from
2.0.0to3.0.0indicates a major version update, which typically signifies breaking changes or significant enhancements in functionality. This aligns with the PR objective of removing the staking feature from the vesting contract.Ensure that the version change does not break any dependent packages.
Run the following script to verify the impact of the version change:
Verification successful
Version change impact on integration tests appears minimal.
The
andromeda-vestingpackage is referenced as a path dependency intests-integration/Cargo.toml, but no specific usage of the package was found in the integration tests. This suggests that the version change to3.0.0does not directly impact the integration tests. However, ensure that any indirect dependencies or behaviors are verified manually.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the version change on the dependent packages. # Test: Search for the usage of the `andromeda-vesting` package in the workspace. Expect: No breaking changes. rg --type toml $'andromeda-vesting'Length of output: 292
Script:
#!/bin/bash # Description: Search for the usage of the `andromeda-vesting` package in the integration tests to identify potential breaking changes. # Test: Search for references to `andromeda-vesting` in the `tests-integration` directory. rg 'andromeda-vesting' tests-integration/Length of output: 154
packages/andromeda-finance/src/vesting.rs (2)
Line range hint
23-46: LGTM!The changes to the
ExecuteMsgenum are approved. The removal of the variants and fields related to delegation and voting functionalities aligns with the PR objective of removing the staking feature from the vesting contract.
Line range hint
73-79: LGTM, but verify the impact on the codebase.The removal of the
unbonding_durationfield from theConfigstruct is approved, as it aligns with the PR objective of removing the staking feature from the vesting contract.However, ensure that the removal of this field does not break any existing functionality in the codebase.
Run the following script to verify the usage of the
unbonding_durationfield:contracts/finance/andromeda-vesting/src/contract.rs (2)
90-90: LGTM!The removal of the
validator_to_delegate_toparameter is consistent with the overall changes in the file, which involve the removal of delegation-related functionalities. This change simplifies theexecute_create_batchfunction by removing the delegation logic.
182-187: LGTM!The changes in the
execute_create_batchfunction are consistent with the removal of delegation-related functionalities. The function now focuses solely on creating a new batch and returning a response with relevant attributes. The changes simplify the function and improve its readability.contracts/finance/andromeda-vesting/src/testing/tests.rs (20)
6-8: Removal ofsample_validatoris approved.The function is not used in the remaining test functions, so its removal is safe.
6-8: Removal ofsample_delegationis approved.The function is not used in the remaining test functions, so its removal is safe.
6-8: Removal ofset_delegationis approved.The function is not used in the remaining test functions, so its removal is safe.
6-8: Removal oftest_create_batch_and_delegateis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_claim_batch_all_funds_delegatedis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_delegate_unauthorizedis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_delegate_no_fundsis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_delegateis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_delegate_more_than_balanceis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_redelegate_unauthorizedis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_redelegate_no_fundsis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_redelegateis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_redelegate_more_than_maxis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_undelegate_unauthorizedis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_undelegate_no_fundsis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_undelegateis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_undelegate_more_than_maxis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_withdraw_rewards_unauthorizedis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_vote_unauthorizedis approved.The function tests the voting functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
6-8: Removal oftest_withdraw_rewardsis approved.The function tests the delegation functionality, which has been removed from the contract. Therefore, its removal is consistent with the changes made to the contract.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments not posted (1)
CHANGELOG.md (1)
42-42: LGTM!The new changelog entry is clear, concise, and consistent with the format of the other entries. It accurately reflects the removal of staking from the vesting contract.
* fix: vesting claim precision issues * chore: version bump * chore: changelog
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
contracts/finance/andromeda-vesting/src/testing/tests.rs (4)
Line range hint
549-634: Review of New Test Case for Large Vesting AmountsThis test case introduces a scenario with a very large vesting amount and a complex release schedule. It's crucial to ensure that the logic handles large numbers and long durations correctly, especially with the use of
Decimalfor percentage calculations.
- Correctness: The use of
Decimal::from_ratiois appropriate for percentage calculations, ensuring precision.- Performance: Consider the impact of these calculations on performance, especially with large numbers and frequent claims.
- Maintainability: The test is somewhat complex; ensure it is well-documented to explain its purpose and the expected outcomes.
Consider adding more detailed comments within the test to explain the rationale behind specific values and expected outcomes, enhancing maintainability and readability.
Line range hint
549-634: Potential Issue with Time Manipulation in TestsThe tests manipulate time to simulate the passing of vesting periods. This is a common practice in contract testing, but it must be done carefully to avoid misleading results.
- Accuracy: Ensure that the time manipulation accurately reflects the intended testing scenarios.
- Robustness: Consider edge cases where time manipulation might not reflect realistic scenarios, such as leap years or daylight saving changes.
Add checks or comments to ensure that time manipulations are clear and justified, helping future maintainers understand the test setup.
Line range hint
549-634: Complexity in Handling Large NumbersThe handling of large numbers, especially with financial calculations, must be precise to avoid rounding errors or integer overflows.
- Safety: Use types and methods that are safe for handling large numbers, such as
Uint128from thecosmwasm_stdlibrary.- Testing: Include tests that specifically check for overflow conditions or rounding errors.
Refactor the test to include explicit checks for overflows and rounding errors, ensuring the robustness of financial calculations.
Line range hint
549-634: Enhance Test Coverage for New FunctionalityGiven the significant changes in functionality with the removal of staking features, it's important to ensure that the remaining functionalities are thoroughly tested.
- Coverage: Review the test coverage to ensure that all critical paths and error conditions are tested.
- Integration: Consider adding integration tests that cover interactions between different parts of the contract, especially with new configurations.
Would you like assistance in setting up additional tests or reviewing existing test coverage? I can help create a detailed test plan or review the current tests in more detail.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- contracts/finance/andromeda-vesting/Cargo.toml (1 hunks)
- contracts/finance/andromeda-vesting/src/contract.rs (11 hunks)
- contracts/finance/andromeda-vesting/src/testing/tests.rs (27 hunks)
- packages/std/src/common/withdraw.rs (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- contracts/finance/andromeda-vesting/Cargo.toml
Additional context used
Learnings (1)
contracts/finance/andromeda-vesting/src/contract.rs (1)
Learnt from: cowboy0015 PR: andromedaprotocol/andromeda-core#466 File: contracts/non-fungible-tokens/andromeda-crowdfund/src/contract.rs:0-0 Timestamp: 2024-05-20T19:05:20.474Z Learning: The `execute_claim` function in the Andromeda Crowdfund project has been refactored into `handle_successful_claim` and `handle_failed_claim` sub-functions to improve clarity and maintainability.
Additional comments not posted (8)
packages/std/src/common/withdraw.rs (2)
20-22: Approved: Enhanced precision in withdrawal calculations.The modification to return a decimal representation of the balance when no specific withdrawal type is provided enhances the precision of financial calculations, aligning with best practices in financial applications.
30-39: Approved: Accurate and safe arithmetic operations in withdrawal calculations.The updates to handle different withdrawal types using decimal calculations, including the safe use of
checked_mul, enhance the accuracy and safety of the financial computations.contracts/finance/andromeda-vesting/src/testing/tests.rs (1)
6-8: Review of Import ChangesThe imports have been adjusted to reflect the removal of staking features. Ensure that all necessary dependencies are still correctly imported for the remaining functionalities.
Verification successful
Imports are Correctly Adjusted
The imports in
contracts/finance/andromeda-vesting/src/testing/tests.rsare correctly adjusted to reflect the removal of staking features. There are no unused imports, and all necessary dependencies for the vesting functionalities are present.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that no unused imports remain and all necessary imports are present. rg --type rust "use" contracts/finance/andromeda-vesting/src/testing/tests.rsLength of output: 328
CHANGELOG.md (2)
42-42: Clear and concise update on staking removal.The entry accurately reflects the significant change in functionality by removing staking from the vesting contract and appropriately links to the pull request for more details.
50-50: Important update on precision fix.The entry effectively communicates the resolution of precision issues with the vestings claim batch method, enhancing the reliability of the contract. It also appropriately links to the pull request for more details.
contracts/finance/andromeda-vesting/src/contract.rs (3)
Line range hint
91-142: Simplified batch creation logic.The removal of the
validator_to_delegate_toparameter simplifies theexecute_create_batchfunction, aligning with the PR's objectives to reduce complexity and potential error sources in the contract. Ensure that all calls to this function have been updated to reflect the removal of this parameter.Run the following script to verify the function usage:
Verification successful
Function signature update verified.
The
execute_create_batchfunction is only used withincontracts/finance/andromeda-vesting/src/contract.rs, and its signature change does not affect other parts of the codebase. The removal of thevalidator_to_delegate_toparameter aligns with the PR's objective to simplify the contract. No further updates are necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `execute_create_batch` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'execute_create_batch'Length of output: 1176
325-329: Refined claim calculation logic.The modifications in the
claim_batchfunction improve the accuracy of the claim calculations, which is crucial for the integrity of financial transactions. Verify that the new calculations are correct and do not introduce rounding errors or other issues.Run the following script to verify the correctness of the calculations:
Verification successful
Verified: Comprehensive test coverage for
claim_batchfunction.The modifications in the
claim_batchfunction are thoroughly tested across various scenarios, ensuring the correctness of the calculations and handling of edge cases. The test cases cover unauthorized access, locked funds, no available funds, single and multiple claims, and edge cases with non-standard numbers, confirming the integrity of the new logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the claim calculations. # Test: Search for the function usage and test cases. Expect: Correct calculations and no rounding errors. rg --type rust -A 5 $'claim_batch'Length of output: 7543
Script:
#!/bin/bash # Extract the contents of the test functions related to claim_batch to verify their coverage. # Search for the test functions related to claim_batch and extract their contents. rg --type rust -A 20 'fn test_claim_batch_' contracts/finance/andromeda-vesting/src/testing/tests.rsLength of output: 6057
420-423: Consistent update in claim calculations.The modifications in the
get_batch_responsefunction ensure consistency with the changes in theclaim_batchfunction, maintaining the accuracy of the claim calculations. Verify that the new calculations are correct and do not introduce rounding errors or other issues.Run the following script to verify the correctness of the calculations:
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (1)
- CHANGELOG.md (2 hunks)
Additional comments not posted (2)
CHANGELOG.md (2)
42-42: Clear and relevant changelog entry.The entry about the removal of staking from the vesting contract is clear and includes a reference to the relevant pull request, which is good practice for traceability.
52-52: Well-documented fix in the changelog.The entry documenting the fix for the precision issue with the vestings claim batch method is concise and includes a reference to the pull request. It might be beneficial to add a brief description of the impact of this fix in future updates for better clarity.
Motivation
Given the recent addition of the
validator-stakingADO and feedback from the audit for both this contract and the vesting contract we decided to remove the ability to stake vested tokens using our vesting contract temporarily. In a future iteration it is likely we will add this functionality back but using thevalidator-stakingADO instead.This should resolve the following findings:
Implementation
Removed all references to delegate/undelegate/vote in the vesting contract.
Testing
Removed related tests
Version Changes
andromeda-vesting:2.0.0->3.0.0Future work
Readd this functionality using the
validator-stakingADOSummary by CodeRabbit
Summary by CodeRabbit
New Features
andromeda-vestingpackage, indicating significant changes.Bug Fixes
Refactor
Tests