epic(contracts-lockup): test for all execute calls and lock lifecycle clarity #154
Conversation
Unique-Divine
changed the title
WalkthroughThe pull request introduces several changes to the smart contract codebase, primarily focusing on enhancing the functionality and maintainability of the lockup contract. Key modifications include the addition of a new dependency in the Changes
Possibly related PRs
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 (
|
Unique-Divine
added
type: enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
contracts/lockup/Cargo.toml (1)
24-24: Approved: Addition ofanyhowdependencyThe addition of the
anyhowcrate as a workspace dependency is a good choice for improving error handling capabilities in the project. This change is consistent with the existing dependency management style in theCargo.tomlfile.Consider adding a brief comment in the
Cargo.tomlfile or in the relevant code files explaining the reason for introducing theanyhowcrate. This will help other developers understand the motivation behind this addition and how it relates to the project's goals, especially in the context of enhancing testing and documentation for the lockup functionality.contracts/lockup/src/msgs.rs (1)
Line range hint
15-36: LGTM! Consider refactoring to reduce duplication inQueryMsg.The
QueryMsgenum effectively covers various query scenarios for the lockup contract. The use ofcw_serdeis appropriate and consistent.To reduce code duplication, consider introducing a common struct for shared fields:
#[cw_serde] struct LockQueryParams { denom: String, unlocking_after: u64, } #[cw_serde] enum QueryMsg { LocksByDenomUnlockingAfter(LockQueryParams), LocksByDenomAndAddressUnlockingAfter { params: LockQueryParams, address: String, }, LocksByDenomBetween { params: LockQueryParams, locked_before: u64, }, LocksByDenomAndAddressBetween { params: LockQueryParams, address: String, locked_before: u64, }, }This refactoring would make the code more maintainable and reduce the risk of inconsistencies when updating query parameters.
contracts/lockup/src/events.rs (1)
5-5: Consider updating the constant name for consistency.While the function names have been updated for consistency, the constant
LOCK_FUNDS_WITHDRAWNdoesn't follow the same naming pattern as the other event name constants (COINS_LOCKED_EVENT_NAMEandUNLOCK_INITIATION_EVENT_NAME).Consider renaming
LOCK_FUNDS_WITHDRAWNtoFUNDS_WITHDRAWN_EVENT_NAMEfor better consistency:-pub const LOCK_FUNDS_WITHDRAWN: &str = "funds_withdrawn"; +pub const FUNDS_WITHDRAWN_EVENT_NAME: &str = "funds_withdrawn";Don't forget to update the constant usage in the
event_funds_withdrawnfunction:- Event::new(LOCK_FUNDS_WITHDRAWN) + Event::new(FUNDS_WITHDRAWN_EVENT_NAME)contracts/lockup/src/contract_test.rs (3)
33-65: LGTM: Comprehensive test coverage for the lock functionality.The
test_execute_lockfunction covers successful lock creation, querying the lock, and error cases effectively. It includes both positive and negative test cases, which is crucial for robust testing.Consider adding a test case for the maximum allowed lock duration to ensure the contract handles edge cases correctly.
101-148: LGTM: Comprehensive test coverage for the withdraw funds functionality.The
test_execute_withdraw_fundsfunction effectively covers the entire lifecycle of a lock, from creation to withdrawal. It includes tests for premature withdrawal attempts, successful withdrawal, and repeated withdrawal attempts. The use of environment manipulation to simulate time passage is a good technique for testing time-dependent behavior.Consider adding a test case for partial withdrawals if the contract supports such functionality. If not, it might be worth documenting that all withdrawals are full withdrawals to prevent any confusion.
1-222: Excellent test coverage with room for minor enhancement.This test file provides comprehensive coverage for all major functionalities of the lockup contract, including lock creation, unlock initiation, fund withdrawal, and lock state transitions. The tests are well-structured, covering both happy paths and error cases, with explicit attention to important edge cases like withdrawal at exact maturity.
The use of helper functions and constants enhances the readability and maintainability of the tests. This aligns well with the PR objectives of enhancing documentation and testing related to the lockup functionality.
To further improve the test suite, consider adding the following:
- A test for the maximum lock duration to ensure the contract handles this limit correctly.
- If applicable, tests for any query functions that might exist in the contract to ensure they return correct data in various lock states.
- If the contract supports it, tests for scenarios involving multiple locks for the same user to ensure proper handling of multiple locks.
contracts/lockup/src/state.rs (1)
7-7: Typo in documentation commentThere's a typographical error in the comment on line 7. The word "ifi" should be "if".
Apply this diff to correct the typo:
-/// height and allows the contract to check ifi a Lock started unlocking or not. +/// height and allows the contract to check if a Lock started unlocking or not.
📜 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 (7)
- contracts/lockup/Cargo.toml (1 hunks)
- contracts/lockup/src/contract.rs (4 hunks)
- contracts/lockup/src/contract_test.rs (1 hunks)
- contracts/lockup/src/events.rs (1 hunks)
- contracts/lockup/src/lib.rs (1 hunks)
- contracts/lockup/src/msgs.rs (2 hunks)
- contracts/lockup/src/state.rs (1 hunks)
🔇 Additional comments not posted (16)
contracts/lockup/src/lib.rs (1)
7-8: LGTM! The addition of the test module aligns with the PR objectives.The introduction of the
contract_testmodule, conditionally compiled for tests, is a good practice. This change supports the PR's goal of enhancing testing for the lockup functionality. Making the module public allows for potential use in integration tests, which can be beneficial for comprehensive testing of the contract's behavior.contracts/lockup/src/msgs.rs (2)
Line range hint
6-13: LGTM! Clear and conciseExecuteMsgdefinition.The
ExecuteMsgenum effectively captures the main actions of a lockup contract. The use ofcw_serdeis consistent and appropriate.
Line range hint
1-36: Overall, the changes improve code quality and consistency.The adoption of the
cw_serdemacro across all message structures simplifies the code and reduces the potential for serialization-related errors. The message definitions are clear and well-structured, effectively capturing the contract's functionality.Consider the suggested refactoring for the
QueryMsgenum to further improve code maintainability. Otherwise, the changes look good and are ready for integration.contracts/lockup/src/events.rs (4)
7-11: Improved naming convention for event creation function.The renaming of
new_coins_locked_eventtoevent_coins_lockedenhances code consistency and readability. This change aligns well with best practices for function naming in event-driven systems.
13-18: Consistent naming convention applied to unlock initiation event.The renaming of
new_unlock_initiation_eventtoevent_unlock_initiatedmaintains the improved naming convention established earlier. This change contributes to overall code consistency and readability.
Line range hint
20-24: Consistent naming convention applied to funds withdrawn event.The renaming of
new_funds_withdrawn_eventtoevent_funds_withdrawncompletes the application of the improved naming convention across all event creation functions in this file. This change enhances overall code consistency and readability.
Line range hint
1-24: Overall improvement in event handling function naming.The changes in this file demonstrate a consistent effort to improve the naming convention of event creation functions. This refactoring enhances code readability and maintainability without altering the core functionality of the events system. The new naming pattern (
event_*instead ofnew_*_event) is more intuitive and aligns well with best practices in event-driven systems.To further improve consistency, consider updating the
LOCK_FUNDS_WITHDRAWNconstant name as suggested in the previous comment.contracts/lockup/src/contract_test.rs (4)
1-31: LGTM: Imports and setup function are well-structured.The imports cover all necessary modules and types for testing. The
setup_contractfunction provides a clean way to initialize the contract for each test, which is a good practice for maintaining test isolation.
67-99: LGTM: Thorough test coverage for the initiate unlock functionality.The
test_execute_initiate_unlockfunction effectively tests the unlock initiation process, including successful execution, lock state updates, and error handling for invalid scenarios. The inclusion of both positive and negative test cases ensures robust validation of the functionality.
150-183: LGTM: Excellent coverage of all lock states.The
test_lock_statefunction effectively tests all possible states of a lock: FundedPreUnlock, Unlocking, Matured, and Withdrawn. It creates different lock scenarios and verifies the resulting state, ensuring that the Lock struct behaves correctly in all situations. The approach of using a single lock instance and modifying it to test different states is efficient and clear.
185-222: LGTM: Crucial edge case coverage for withdrawal at exact maturity.The
test_withdraw_at_exact_maturityfunction effectively addresses a critical edge case by testing the withdrawal of funds at the precise moment of lock maturity. This test ensures that the contract correctly handles withdrawals not just after maturity, but at the exact point of maturity as well.This is an important scenario to test as it validates that there's no off-by-one error in the maturity calculation, which could potentially lock user funds for an extra block. The test is well-structured, covering the entire process from lock creation to withdrawal, and verifies both the successful fund transfer and the updated lock state.
contracts/lockup/src/state.rs (1)
38-49: Definition ofLockStateenum looks goodThe
LockStateenum is well-defined and covers all necessary lock lifecycle states.contracts/lockup/src/contract.rs (4)
61-78: Improved lock state handling inexecute_withdraw_fundsThe updated
matchstatement enhances clarity by explicitly handling each possible lock state, making the function more maintainable.
100-110: Enhanced logic inexecute_initiate_unlockThe
matchstatement inexecute_initiate_unlockprovides clear logic for state transitions, ensuring correct behavior based on the current lock state.
167-167: Event emission inexecute_lockThe code correctly emits an event after locking coins, which is important for transparency and auditing.
6-8:⚠️ Potential issueFix syntax error in
usestatementThe
usestatement on line 6 is missing the opening curly brace{, which will cause a syntax error.Apply this diff to fix the syntax error:
-use crate::state:{ +use crate::state::{Likely invalid or redundant comment.
| use cosmwasm_schema::cw_serde; | ||
|
|
||
| #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] | ||
| #[cw_serde] | ||
| pub struct InstantiateMsg {} |
There was a problem hiding this comment.
💡 Codebase verification
InstantiateMsg Should Not Be Empty
The InstantiateMsg struct is being instantiated with parameters in multiple locations across the codebase. This indicates that it should include the necessary fields rather than remaining empty.
- Affected Files:
contracts/nusd-valuator/src/testing.rscontracts/core-token-vesting-v2/src/testing.rscontracts/core-cw3-fixed-msig/src/integration_tests.rscontracts/broker-bank/src/tutil.rscontracts/core-cw3-flex-msig/src/contract.rs
Please update the InstantiateMsg struct to include all required fields to ensure proper contract initialization.
🔗 Analysis chain
LGTM! Verify if InstantiateMsg should remain empty.
The use of cw_serde macro is a good practice in CosmWasm contracts. It simplifies serialization and reduces potential errors.
However, please confirm if InstantiateMsg should remain empty. If instantiation parameters are needed, they should be added here.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if InstantiateMsg is used with parameters elsewhere in the codebase
# Test: Search for InstantiateMsg usage
rg --type rust "InstantiateMsg\s*\{[^}]+\}" -g '!msgs.rs'
Length of output: 59
Script:
#!/bin/bash
# Description: Use ast-grep to find instances where InstantiateMsg is instantiated with parameters
ast-grep --lang rust --pattern 'InstantiateMsg { $$$ }'
Length of output: 12954
| pub owner: String, | ||
| /// The duration of the lock in number of blocks. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing redundant duration_blocks field
Since both start_block and end_block are stored, the duration can be calculated as end_block - start_block. Storing duration_blocks may be redundant unless there is a specific reason to keep it.
| impl Lock { | ||
| /// Computes the lifecycle state of the Lock | ||
| pub fn state(&self, current_block: u64) -> LockState { | ||
| if self.funds_withdrawn { | ||
| return LockState::Withdrawn; | ||
| } | ||
|
|
||
| match self.end_block { | ||
| NOT_UNLOCKING_BLOCK_IDENTIFIER => LockState::FundedPreUnlock, | ||
| end_block if current_block >= end_block => LockState::Matured, | ||
| _ => LockState::Unlocking, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using Option<u64> for end_block instead of a sentinel value
Using Option<u64> for end_block can make the code clearer and more idiomatic by explicitly representing the absence of a value without relying on a sentinel constant.
Apply this diff to update the Lock struct and the state method:
pub struct Lock {
/// Unique identifier for the lock.
pub id: u64,
/// The amount and denomination of the locked funds.
pub coin: Coin,
/// The address of the lock owner.
pub owner: String,
/// The duration of the lock in number of blocks.
pub duration_blocks: u64,
/// The block height at which the lock was created.
pub start_block: u64,
- /// The block height at which the lock ends.
- ///
- /// This is set to `NOT_UNLOCKING_BLOCK_IDENTIFIER` when the lock is created,
- /// and updated to an actual block height when unlocking is initiated.
- pub end_block: u64,
+ /// The optional block height at which the lock ends.
+ ///
+ /// This is set to `None` when the lock is created and updated to `Some(end_block)`
+ /// when unlocking is initiated.
+ pub end_block: Option<u64>,
/// Indicates whether the funds have been withdrawn after the lock period.
pub funds_withdrawn: bool,
}
impl Lock {
/// Computes the lifecycle state of the Lock
pub fn state(&self, current_block: u64) -> LockState {
if self.funds_withdrawn {
return LockState::Withdrawn;
}
match self.end_block {
- NOT_UNLOCKING_BLOCK_IDENTIFIER => LockState::FundedPreUnlock,
+ None => LockState::FundedPreUnlock,
- end_block if current_block >= end_block => LockState::Matured,
+ Some(end_block) if current_block >= end_block => LockState::Matured,
_ => LockState::Unlocking,
}
}
}Committable suggestion was skipped due to low confidence.
| @@ -1,13 +1,14 @@ | |||
| use crate::error::ContractError; | |||
| use crate::events::{ | |||
| new_coins_locked_event, new_funds_withdrawn_event, | |||
| new_unlock_initiation_event, | |||
| event_coins_locked, event_funds_withdrawn, event_unlock_initiated, | |||
There was a problem hiding this comment.
💡 Codebase verification
Old event function names still present in the codebase
- Functions
new_coins_locked_event,new_funds_withdrawn_event, andnew_unlock_initiation_eventare still defined incontracts/nibi-stargate/src/events.rs.
🔗 Analysis chain
Ensure all event function usages are updated
Since event function names have been changed to improve clarity, please verify that all usages of the old event function names have been updated throughout the codebase.
Run the following script to check for any remaining usages of the old event function names:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find any usages of the old event function names in the codebase.
# Expected result: No matches found.
rg --type rust 'new_coins_locked_event|new_funds_withdrawn_event|new_unlock_initiation_event'
Length of output: 374
This increased test coverage a lot, both in terms of LOC and paths encountered in the logic.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor