Feat/validator staking #565
Conversation
…-unstake Feat/validator staking unstake
…ate-default-validator
…-update-default-validator Feat/validator staking update default validator
…-claim-reward fix: fixed claim rewards submessage handler
WalkthroughThe changes introduce enhancements to the validator staking contract and the associated testing framework. Key modifications include updates to the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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: 2
Outside diff range and nitpick comments (12)
ibc-tests/README.md (1)
1-12: LGTM!The README file provides clear and concise documentation for the
ibc-testspackage. The content is well-structured, outlining the prerequisites and workflow for running the e2e tests.A few minor suggestions for improvement:
- Line 5: Consider adding the article "the" before "e2e test" for better readability.
- Line 5: Consider adding a comma after the conjunctive adverb "Currently" to improve clarity.
- Line 5: Consider adding the article "the" before "
developbranch" for consistency.- Line 11: Consider adding the preposition "to" before "setup aOS" for grammatical correctness.
These suggestions are based on the static analysis hints and are not critical issues.
Tools
LanguageTool
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: .... You need to run the local network for e2e test. Currently this package is using `...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~5-~5: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ... to run the local network for e2e test. Currently this package is usingdevelopbranch ...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: ...e test. Currently this package is usingdevelopbranch of [localrelayer](https:...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~11-~11: Possible missing preposition found.
Context: ...2e tests, follow the following steps 1. Run the main function to setup aOS and nece...(AI_HYDRA_LEO_MISSING_TO)
packages/andromeda-testing-e2e/src/economics.rs (1)
17-25: Consider error handling in production code.The implementation of the
EconomicsContractand theinitmethod looks good for the testing environment. TheInstantiateMsgis constructed correctly with the providedkernel_addressand an optionalownerfield.However, the use of
unwrap()assumes that the instantiation will always succeed without errors. While this is acceptable for testing purposes, it's important to consider proper error handling in production code to gracefully handle potential failures and provide meaningful error messages.packages/andromeda-testing-e2e/src/faucet.rs (1)
6-18: LGTM!The
fundfunction provides a useful utility for automated funding of accounts in blockchain testing and development environments. The implementation is secure and efficient, utilizing a predefined mnemonic for the faucet daemon and asynchronously sending the specified amount of coins to the target address.The error handling using
unwrap()is acceptable for a testing environment. However, consider more robust error handling, such as returning aResulttype, for production code to gracefully handle potential errors.packages/andromeda-testing-e2e/src/interface_macro.rs (1)
19-23: Consider improving error handling.The
unwrapmethod is used to extract theWasmPathfrom theOptionreturned byfind_wasm_path. However, this will panic if the WASM file is not found.Consider providing a more descriptive error message or using a different error handling mechanism, such as returning a
Resultor usingexpectwith a custom error message.packages/andromeda-testing-e2e/src/mock.rs (3)
9-18: Consider making the funding amount configurable.The function initializes a
DaemonBaseinstance and funds it with a predefined amount of tokens using thefundfunction. However, the funding amount of 10000000000000 tokens is hardcoded, which may not be suitable for all testing scenarios.Consider adding an optional parameter with a default value to make the funding amount configurable. This would provide more flexibility and allow the function to be used in different testing contexts.
-pub fn mock_app(chain: ChainInfo, mnemonic: &str) -> DaemonBase<Wallet> { +pub fn mock_app(chain: ChainInfo, mnemonic: &str, funding_amount: u128 = 10000000000000) -> DaemonBase<Wallet> { let daemon = Daemon::builder(chain.clone()) // set the network to use .mnemonic(mnemonic) .build() .unwrap(); - fund(&daemon, &chain, 10000000000000); + fund(&daemon, &chain, funding_amount); daemon }
28-91: Use constants for contract names and versions.The function uses hardcoded strings for the contract names and versions when registering the code IDs in the ADO database. If the contract names or versions change, these hardcoded values would need to be updated manually, which can be error-prone and reduce maintainability.
Consider defining constants for the contract names and versions to improve maintainability and reduce the risk of inconsistencies.
+const ADODB_CONTRACT_NAME: &str = "adodb"; +const VFS_CONTRACT_NAME: &str = "vfs"; +const KERNEL_CONTRACT_NAME: &str = "kernel"; +const CONTRACT_VERSION: &str = "0.1.0"; + // register code ids in ado db adodb_contract.clone().execute_publish( adodb_contract.code_id().unwrap(), - "adodb".to_string(), - "0.1.0".to_string(), + ADODB_CONTRACT_NAME.to_string(), + CONTRACT_VERSION.to_string(), ); adodb_contract.clone().execute_publish( vfs_contract.code_id().unwrap(), - "vfs".to_string(), - "0.1.0".to_string(), + VFS_CONTRACT_NAME.to_string(), + CONTRACT_VERSION.to_string(), ); adodb_contract.clone().execute_publish( kernel_contract.code_id().unwrap(), - "kernel".to_string(), - "0.1.0".to_string(), + KERNEL_CONTRACT_NAME.to_string(), + CONTRACT_VERSION.to_string(), );
28-91: Add error handling for contract uploading and initialization.The function assumes that the contract code is available and can be uploaded successfully. It also assumes that the initialization of the contracts will succeed without any issues. However, there could be scenarios where the contract uploading or initialization fails.
Consider adding error handling to gracefully handle any failures during the contract uploading and initialization process. This would improve the robustness of the code and provide better visibility into any issues that may occur.
// Upload and instantiate os ADOs let kernel_contract = KernelContract::new(daemon.clone()); -kernel_contract.upload().unwrap(); +kernel_contract.upload().map_err(|e| { + println!("Failed to upload kernel contract: {}", e); + e +})?; kernel_contract.clone().init(chain_name); let adodb_contract = AdodbContract::new(daemon.clone()); -adodb_contract.upload().unwrap(); +adodb_contract.upload().map_err(|e| { + println!("Failed to upload adodb contract: {}", e); + e +})?; adodb_contract .clone() .init(kernel_contract.addr_str().unwrap()); let vfs_contract = VfsContract::new(daemon.clone()); -vfs_contract.upload().unwrap(); +vfs_contract.upload().map_err(|e| { + println!("Failed to upload vfs contract: {}", e); + e +})?; vfs_contract .clone() .init(kernel_contract.addr_str().unwrap()); let economics_contract = EconomicsContract::new(daemon.clone()); -economics_contract.upload().unwrap(); +economics_contract.upload().map_err(|e| { + println!("Failed to upload economics contract: {}", e); + e +})?; economics_contract .clone() .init(kernel_contract.addr_str().unwrap());ibc-tests/src/main.rs (5)
47-87: LGTM! Consider refactoring the function into smaller functions for better readability and maintainability.The
mainfunction correctly sets up the testing environment for the Andromeda OS and its components. However, the function is quite large and could be refactored into smaller functions for better readability and maintainability.Consider refactoring the function into smaller functions, each responsible for a specific task, such as:
- Setting up the mock application environment
- Creating instances of the kernel and adodb contracts
- Uploading and publishing the AppContract
- Preparing the Validator Staking component
This will make the code easier to understand, maintain, and debug in the future.
48-85: Nitpick: Use a consistent format for logging statements and consider using a logging library.The logging statements provide good visibility into the setup process. However, they use different formats, which could make it harder to parse the logs.
Consider using a consistent format for the logging statements, such as:
println!("[INFO] Setting up the mock application environment");Also, consider using a logging library, such as
logorenv_logger, for better control over the log levels and output. This will make it easier to enable or disable certain log levels in different environments (e.g., development, testing, production).
97-204: LGTM! Consider refactoring the function into smaller functions for better readability and maintainability.The
prepare_validator_stakingfunction correctly prepares the Validator Staking component and integrates it into the main application. However, the function is quite large and could be refactored into smaller functions for better readability and maintainability.Consider refactoring the function into smaller functions, each responsible for a specific task, such as:
- Uploading and publishing the Validator Staking contract
- Initializing the Validator Staking contract with a default validator
- Constructing the application component that integrates the Validator Staking contract into the main application
- Processing staking for each validator
This will make the code easier to understand, maintain, and debug in the future.
133-136: Good safeguard! Consider adding a constant for the minimum number of validators required for the test.The check for at least five validators is a good safeguard to ensure that the test has enough validators to work with.
Consider adding a constant for the minimum number of validators required for the test, such as:
const MIN_VALIDATORS_REQUIRED: usize = 5;Then, you can use this constant in the check:
if validators.len() < MIN_VALIDATORS_REQUIRED { println!("At least {} validators are required for this test", MIN_VALIDATORS_REQUIRED); return; }This will make the code more readable and maintainable, as the minimum number of validators required is clearly defined in a single place.
185-203: Consider parameterizing the amount of stake to delegate to each validator for more flexibility and reusability of the test.The function uses hardcoded values for the amount of stake to delegate to each validator, which could limit the flexibility of the test.
Consider parameterizing the amount of stake to delegate to each validator, such as:
fn prepare_validator_staking( daemon: &DaemonBase<Wallet>, mock_andromeda: &MockAndromeda, app_contract: &AppContract<DaemonBase<Wallet>>, stake_amount: Uint128, ) { // ... validators.into_iter().for_each(|validator| { // ... let amount_to_send = cmp::min(balance[0].amount, stake_amount); // ... }); // ... }Then, you can call the function with different stake amounts depending on your test scenario:
prepare_validator_staking(&daemon, &mock_andromeda, &app_contract, Uint128::new(10000000000));This will make the test more flexible and reusable, as you can easily adjust the amount of stake delegated to each validator without modifying the function itself.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockibc-tests/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (38)
- Cargo.toml (2 hunks)
- contracts/finance/andromeda-validator-staking/src/contract.rs (9 hunks)
- contracts/finance/andromeda-validator-staking/src/lib.rs (1 hunks)
- contracts/finance/andromeda-validator-staking/src/mock.rs (2 hunks)
- contracts/finance/andromeda-validator-staking/src/state.rs (1 hunks)
- contracts/finance/andromeda-validator-staking/src/testing/tests.rs (3 hunks)
- contracts/finance/andromeda-validator-staking/src/util.rs (1 hunks)
- ibc-tests/.eslintrc.json (0 hunks)
- ibc-tests/.gitignore (0 hunks)
- ibc-tests/.prettierignore (0 hunks)
- ibc-tests/Cargo.toml (1 hunks)
- ibc-tests/README.md (1 hunks)
- ibc-tests/package.json (0 hunks)
- ibc-tests/src/interface_macro.rs (1 hunks)
- ibc-tests/src/lib.rs (1 hunks)
- ibc-tests/src/main.rs (1 hunks)
- ibc-tests/test/configs.ts (0 hunks)
- ibc-tests/test/contract.ts (0 hunks)
- ibc-tests/test/ibc.spec.ts (0 hunks)
- ibc-tests/test/logger.ts (0 hunks)
- ibc-tests/test/os.ts (0 hunks)
- ibc-tests/test/relayer.ts (0 hunks)
- ibc-tests/test/utils.ts (0 hunks)
- ibc-tests/tests/validator_staking.rs (1 hunks)
- ibc-tests/tsconfig.json (0 hunks)
- packages/andromeda-finance/src/validator_staking.rs (1 hunks)
- packages/andromeda-testing-e2e/Cargo.toml (1 hunks)
- packages/andromeda-testing-e2e/src/adodb.rs (1 hunks)
- packages/andromeda-testing-e2e/src/economics.rs (1 hunks)
- packages/andromeda-testing-e2e/src/faucet.rs (1 hunks)
- packages/andromeda-testing-e2e/src/interface_macro.rs (1 hunks)
- packages/andromeda-testing-e2e/src/kernel.rs (1 hunks)
- packages/andromeda-testing-e2e/src/lib.rs (1 hunks)
- packages/andromeda-testing-e2e/src/mock.rs (1 hunks)
- packages/andromeda-testing-e2e/src/vfs.rs (1 hunks)
- packages/andromeda-testing/src/faucet.rs (1 hunks)
- packages/andromeda-testing/src/mock.rs (0 hunks)
- tests-integration/tests/validator_staking.rs (7 hunks)
Files not reviewed due to no reviewable changes (13)
- ibc-tests/.eslintrc.json
- ibc-tests/.gitignore
- ibc-tests/.prettierignore
- ibc-tests/package.json
- ibc-tests/test/configs.ts
- ibc-tests/test/contract.ts
- ibc-tests/test/ibc.spec.ts
- ibc-tests/test/logger.ts
- ibc-tests/test/os.ts
- ibc-tests/test/relayer.ts
- ibc-tests/test/utils.ts
- ibc-tests/tsconfig.json
- packages/andromeda-testing/src/mock.rs
Additional context used
LanguageTool
ibc-tests/README.md
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: .... You need to run the local network for e2e test. Currently this package is using `...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~5-~5: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ... to run the local network for e2e test. Currently this package is usingdevelopbranch ...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: ...e test. Currently this package is usingdevelopbranch of [localrelayer](https:...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~11-~11: Possible missing preposition found.
Context: ...2e tests, follow the following steps 1. Run the main function to setup aOS and nece...(AI_HYDRA_LEO_MISSING_TO)
Additional comments not posted (63)
ibc-tests/src/lib.rs (1)
1-1: Module declaration looks good, but more information is needed.The declaration of the
interface_macromodule is syntactically correct. However, to provide a more thorough review, please provide more information about:
- The purpose and intended use of this module.
- The internal implementation of the module, including any macros, functions, or types it defines.
- How this module interacts with or affects other parts of the codebase.
Without more context about the module's implementation and its role within the larger system, it's difficult to assess its potential impact and suggest improvements.
contracts/finance/andromeda-validator-staking/src/lib.rs (1)
5-5: Approve the addition of theutilmodule.The addition of the
utilmodule is a good step towards improving the modularity and reusability of the codebase. It can potentially provide common utility functions or types that can be used across the other modules.However, it would be helpful to have more information about the purpose and contents of the
utilmodule. Could you please provide a brief overview of the utility functions or types that are included in this module and how they are intended to be used?contracts/finance/andromeda-validator-staking/src/state.rs (2)
2-2: LGTM!The import of the
Itemtype from thecw_storage_pluscrate is necessary for the change made to theUNSTAKING_QUEUEconstant.
8-8: Verify the impact of the change on the codebase.The change in data structure from a
Deque<UnstakingTokens>to anItem<Vec<UnstakingTokens>>could affect the order and efficiency of operations that were previously optimized for a deque structure. It may simplify how unstaking tokens are managed, possibly allowing for easier access and manipulation of the list of unstaking tokens. However, the change in type could lead to different performance characteristics and usage patterns in the codebase.Run the following script to verify the usage of the
UNSTAKING_QUEUEconstant:Add a comment to explain the reason for the change.
Consider adding a comment above the
UNSTAKING_QUEUEconstant to explain the reason for the change in data structure. This will help future maintainers understand the motivation behind the change and its potential implications.Verification successful
Verified: Change from Deque to Vec doesn't significantly impact functionality
The change from
Deque<UnstakingTokens>toItem<Vec<UnstakingTokens>>forUNSTAKING_QUEUEappears to be safe and doesn't introduce any significant functional changes:
- Basic operations (load, save, push) remain the same.
- The FIFO nature of the queue is preserved.
- The
retainoperation is still valid forVec.The change might slightly improve memory efficiency and simplifies the data structure. However, consider adding a comment above the
UNSTAKING_QUEUEconstant to explain the rationale behind this change, as it might not be immediately obvious to future maintainers.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `UNSTAKING_QUEUE` constant. # Test 1: Search for read access to the constant. rg --type rust -A 5 $'UNSTAKING_QUEUE\.\w+' # Test 2: Search for write access to the constant. rg --type rust -A 5 $'UNSTAKING_QUEUE\.\w+\s*='Length of output: 3726
packages/andromeda-testing-e2e/src/lib.rs (7)
1-2: LGTM!The conditional compilation of the
faucetmodule based on the target architecture is a good practice. It ensures that the module is only included in non-WebAssembly environments where its functionality is required.
4-5: LGTM!The conditional compilation of the
interface_macromodule based on the target architecture is a good practice. It ensures that the module is only included in non-WebAssembly environments where its functionality is required.
7-8: LGTM!The conditional compilation of the
mockmodule based on the target architecture is a good practice. It ensures that the module is only included in non-WebAssembly environments where its functionality is required.
10-11: LGTM!The conditional compilation of the
kernelmodule based on the target architecture is a good practice. It ensures that the module is only included in non-WebAssembly environments where its functionality is required.
13-14: LGTM!The conditional compilation of the
adodbmodule based on the target architecture is a good practice. It ensures that the module is only included in non-WebAssembly environments where its functionality is required.
16-17: LGTM!The conditional compilation of the
vfsmodule based on the target architecture is a good practice. It ensures that the module is only included in non-WebAssembly environments where its functionality is required.
19-20: LGTM!The conditional compilation of the
economicsmodule based on the target architecture is a good practice. It ensures that the module is only included in non-WebAssembly environments where its functionality is required.packages/andromeda-testing-e2e/src/vfs.rs (1)
12-18: Verify the impact of setting theownerfield toNone.The function logic is correct and the implementation looks good. However, setting the
ownerfield toNonecould potentially lead to security issues if not handled properly.Run the following script to verify the impact of setting the
ownerfield toNone:Verification successful
Setting
ownertoNoneis a common pattern and doesn't pose immediate security risks.The codebase analysis reveals that setting the
ownerfield toNoneduring contract instantiation is a common pattern across various contracts in the project. This approach aligns with the implementation in the reviewed code and doesn't appear to introduce any immediate security vulnerabilities.
- The
initfunction in the VFS contract seems to be the primary method for instantiation, as no directVfsContract::instantiatecalls were found.- Multiple contracts in the project use a similar pattern of allowing
Nonefor theownerfield in theirInstantiateMsg.While this implementation doesn't pose immediate security risks, it's important to ensure that proper ownership and access control mechanisms are in place throughout the contract's lifecycle.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of setting the `owner` field to `None` in the `InstantiateMsg`. # Test 1: Search for the usage of the `owner` field in the codebase. # Expect: The `owner` field should be handled properly in all the places where it is used. rg --type rust -A 5 $'owner:' # Test 2: Search for the instantiation of the VFS contract in the codebase. # Expect: The instantiation should be done only through the `init` function. ast-grep --lang rust --pattern $'VfsContract<$_>::instantiate($$$)'Length of output: 169266
packages/andromeda-testing-e2e/src/economics.rs (2)
1-9: LGTM!The imports are relevant and necessary for the contract implementation. The code segment looks good.
10-16: LGTM!The
contract_interface!macro is used appropriately to define theEconomicsContractinterface. The contract name and associated functionality are specified correctly.ibc-tests/Cargo.toml (3)
1-6: LGTM!The package metadata looks good. Using the latest Rust edition and specifying the minimum Rust version are good practices.
8-9: LGTM!Having a dedicated test target for validator staking is a good practice. It allows running the tests separately and ensures that the validator staking functionality is thoroughly tested.
11-33: Dependencies look good!The listed dependencies are commonly used in Rust projects and are appropriate for this package. Linking the
andromeda-app-contractandandromeda-validator-stakingdependencies to local paths suggests that they are custom contracts being developed in tandem with the tests, which is a good approach for ensuring the reliability of the validator staking logic.One observation: Make sure to keep the local path dependencies up to date with the latest changes in their respective directories to avoid any inconsistencies during testing.
packages/andromeda-testing/src/faucet.rs (4)
5-11: LGTM!The
AirdropRequeststruct is well-defined with appropriate fields and derives necessary traits for serialization, deserialization, and debugging.
12-25: LGTM!The
airdropfunction correctly constructs the URL for the faucet service and sends a POST request with the airdrop request data in JSON format. The use ofasyncis appropriate for handling the asynchronous operation.
27-30: LGTM!The
fundfunction provides a convenient synchronous interface to initiate the airdrop process. Creating a new Tokio runtime and blocking on theairdropfunction is an effective way to bridge the gap between asynchronous and synchronous code.
1-30: Great job on the overall design and architecture!The
faucet.rsfile demonstrates a well-structured and modular approach to handling airdrop requests. The separation of concerns between theAirdropRequeststruct and theairdropfunction promotes code reusability and maintainability. Thefundfunction provides a user-friendly synchronous interface, abstracting away the asynchronous details.The use of Tokio runtime for handling asynchronous operations is a good choice, as it allows for efficient and scalable execution.
Overall, the design and architecture of the
faucet.rsfile are solid and follow best practices.packages/andromeda-testing-e2e/src/adodb.rs (4)
1-9: LGTM!The imports are relevant and necessary for the functionality of the
AdodbContract. The code segment looks good.
10-16: LGTM!The
contract_interface!macro is used correctly to establish theAdodbContractinterface. The bindings and naming conventions are appropriate. The code segment looks good.
18-24: LGTM!The
initmethod is implemented correctly. The method signature,InstantiateMsgconstruction, and theinstantiatemethod call are all appropriate. The code segment looks good.
26-38: LGTM!The
execute_publishmethod is implemented correctly. The method signature,ExecuteMsg::Publishconstruction, and theexecutemethod call are all appropriate. The code segment looks good.ibc-tests/src/interface_macro.rs (1)
2-26: LGTM! Thecontract_interface!macro is a great addition.The macro rule streamlines the process of defining contract interfaces by generating the necessary structs and trait implementations. It ensures consistency and reduces boilerplate code.
Key benefits:
- The
interfaceattribute ensures that the correct message types are specified for the contract.- The
Uploadabletrait implementation provides a consistent way to construct mock contracts and retrieve WASM paths.- The macro parameters allow for easy customization of the generated contract interface.
Overall, this macro rule is a valuable addition to the codebase, making it easier to define and work with contract interfaces.
packages/andromeda-testing-e2e/src/interface_macro.rs (3)
1-3: LGTM!The macro definition and parameters are well-defined and follow the Rust conventions.
4-5: LGTM!The contract struct is correctly generated with the
#[interface(...)]attribute specifying the message types and contract ID.
7-17: LGTM!The
Uploadabletrait implementation for the contract struct is well-defined and correctly constructs aMockContractinstance with the required functions.packages/andromeda-finance/src/validator_staking.rs (3)
25-25: LGTM!The addition of the optional
validatorfield to theClaimvariant enhances the functionality by allowing claims to be associated with specific validators. This change aligns with the PR objective and provides more control over the claiming process.
26-29: LGTM!The addition of the optional
denomandrecipientfields to theWithdrawFundsvariant enhances the functionality by allowing more flexible fund withdrawal specifications. This change provides more control over the fund withdrawal process and is backward compatible.
30-32: Ensure validation of thevalidatorfield.The addition of the
UpdateDefaultValidatorvariant to theExecuteMsgenum introduces a new functionality to update the default validator within the system. This change allows for more flexibility in managing the default validator.However, it's important to ensure that the
validatorfield is validated to be a valid validator address before updating the default validator. Consider adding validation logic to prevent setting an invalid address as the default validator.Run the following script to verify the validation of the
validatorfield:Verification successful
Validation of
validatorfield confirmedThe
UpdateDefaultValidatorvariant'svalidatorfield is properly validated before updating the default validator. Theis_validatorfunction inpackages/andromeda-finance/src/validator_staking.rschecks if the provided validator address is valid by querying the validator. This function is called in the contract's execution logic before saving the new default validator, ensuring that only valid validator addresses can be set as the default.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `validator` field in `UpdateDefaultValidator` variant is validated. # Test: Search for the validation logic. Expect: Validation logic for the `validator` field. rg --type rust -A 5 $'is_validator'Length of output: 3626
packages/andromeda-testing-e2e/Cargo.toml (5)
1-7: LGTM!The
[package]section correctly defines the package metadata with appropriate attributes. The package name, version, authors, description, and license are clearly specified, and the Rust edition is set to the stable 2018 edition.
9-10: LGTM!The
[features]section correctly defines thebacktracesfeature, which enables backtraces in thecosmwasm-stddependency. This feature can be beneficial for debugging and error tracing purposes.
13-14: LGTM!The
[lib]section correctly specifies thecrate-typeas["cdylib", "rlib"]. This configuration allows the library to be used as both a dynamic library (cdylib) and a static library (rlib), providing flexibility for integration with other Rust projects and potentially with non-Rust environments.
16-42: LGTM!The
[dependencies]section includes a comprehensive set of dependencies required for building and testing Andromeda Digital Object Contracts. The dependencies cover essential CosmWasm libraries, standard token interfaces, error handling, and Andromeda-specific modules.The use of workspace dependencies and specific paths indicates a modular architecture and local development setup, which promotes code reuse and maintainability. The dependencies are well-organized and clearly specified, making it easy to understand the library's requirements and dependencies.
44-50: LGTM!The
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]section correctly specifies dependencies that are conditionally included when the target architecture is not WASM32. These dependencies, such ascw-multi-test,reqwest,tokio,cw-orch-daemon, andcw-orch, are likely used for testing and interacting with the contracts in a non-WASM environment.The conditional inclusion of these dependencies demonstrates a well-structured and optimized configuration that adapts to different target architectures. This approach ensures that the necessary dependencies are available for testing and development purposes while keeping the library lean for WASM targets.
Cargo.toml (2)
15-15: LGTM!The addition of
"ibc-tests"to themembersarray is a positive change that aligns with the PR objectives. It demonstrates a focus on thorough testing and validation of the implemented features, particularly those related to Inter-Blockchain Communication (IBC).
41-41: Great addition for comprehensive testing!The inclusion of the
andromeda-testing-e2edependency is a valuable addition to the project's testing capabilities. End-to-end testing is crucial for validating the functionality and integration of new features, especially in the context of the Andromeda protocol.By pointing to a local package, it's evident that this is a custom testing package developed specifically for the Andromeda project, which demonstrates a commitment to thorough and tailored testing.
contracts/finance/andromeda-validator-staking/src/util.rs (4)
3-19: LGTM!The
decode_leb128function correctly decodes a sequence of bytes encoded in LEB128 format into a 64-bit unsigned integer. The logic is sound and handles the continuation bits properly.
21-41: LGTM!The
decode_unstaking_response_datafunction correctly extracts the seconds and nanoseconds values from the raw data of an unstaking submessage reply. It uses thedecode_leb128function to decode the values from the relevant byte ranges and ignores any additional data, ensuring compatibility with different versions of the Cosmos SDK.
43-64: LGTM!The
test_decode_leb128function includes comprehensive test cases to validate the correctness of thedecode_leb128function. The test cases cover different input scenarios and compare the output with the expected values using theassert_eq!macro. This ensures that thedecode_leb128function behaves as intended.
65-82: LGTM!The
test_decode_unstaking_response_datafunction includes test cases to validate the correctness of thedecode_unstaking_response_datafunction. The test cases provide different input data and compare the output tuple with the expected values using theassert_eq!macro. This ensures that thedecode_unstaking_response_datafunction correctly extracts the seconds and nanoseconds values from the raw data.packages/andromeda-testing-e2e/src/mock.rs (1)
28-91: Comprehensive setup for testing the Andromeda application.The
newfunction provides a comprehensive setup for testing the Andromeda application by initializing and configuring the necessary contract instances. It uploads and instantiates the contracts, registers their code IDs in the ADO database, and updates the kernel contract with the addresses of the other contracts.This setup allows for thorough testing of the Andromeda application by simulating a blockchain environment with interconnected contracts.
contracts/finance/andromeda-validator-staking/src/mock.rs (6)
46-46: LGTM!The removal of the
recipientparameter simplifies the function signature and usage. It's a reasonable assumption that the reward will always be claimed by the sender. The change is consistent with the corresponding mock function update.
51-53: LGTM!The addition of the
denomandrecipientparameters enhances the flexibility of the withdrawal operation. It allows specifying the denomination of funds to withdraw and a different recipient address for the withdrawn funds. The changes are consistent with the corresponding mock function update.
55-60: LGTM!The addition of the
execute_update_default_validatorfunction provides a convenient way to update the default validator address. It encapsulates the logic for sending an update message for the default validator. The correspondingmock_execute_update_default_validatorfunction has been added to support this functionality.
111-113: LGTM!The removal of the
recipientparameter from themock_execute_claim_rewardfunction simplifies the function signature and usage. It reflects the assumption that the reward will always be claimed by the sender. This change is consistent with the corresponding update in theexecute_claim_rewardfunction.
115-119: LGTM!The addition of the
denomandrecipientparameters to themock_execute_withdraw_fundfunction enhances the flexibility of the withdrawal operation. It allows specifying the denomination of funds to withdraw and a different recipient address for the withdrawn funds. This change is consistent with the corresponding update in theexecute_withdraw_fundfunction.
122-123: LGTM!The addition of the
mock_execute_update_default_validatorfunction provides a convenient way to construct anExecuteMsgfor updating the default validator. It supports the newly addedexecute_update_default_validatorfunction.contracts/finance/andromeda-validator-staking/src/testing/tests.rs (2)
6-6: LGTM!Removing the unused import
AndrAddris a good practice to keep the code clean.
16-16: Great work on enhancing the test coverage!The introduction of the
ANYONEconstant and its usage in thetest_unauthorized_claimfunction effectively broadens the scope of the test to ensure that any user, not just the owner, is correctly denied access when attempting to claim rewards. This change aligns with the expected behavior of theClaimmessage and helps improve the robustness of the test suite.Also applies to: 172-172
ibc-tests/tests/validator_staking.rs (2)
54-125: LGTM!The test function
test_validator_stakingprovides good coverage for the validator staking functionality. It sets up the test environment correctly, waits for a bonded validator to be found, and covers the essential staking operations. The assertions are valid and ensure that the contract balance is zero after the operations.
128-210: LGTM!The test function
test_kicked_validatorprovides good coverage for handling validators that have been kicked from the network. It sets up the test environment correctly, waits for a kicked validator to be found, and covers the essential operations for a kicked validator. The assertions are valid and ensure that the contract balance is zero after the operations.contracts/finance/andromeda-validator-staking/src/contract.rs (6)
Line range hint
215-256: LGTM!The changes to
execute_claimsimplify the claim process by removing the recipient parameter and treating the message sender as the recipient by default. The authorization checks ensure that only the contract owner can execute claims.
Line range hint
259-308: LGTM!The changes to
execute_withdraw_fundimprove the flexibility of fund withdrawals by allowing the contract owner to specify a specific denomination or recipient. The removal of expired unstaking requests helps keep the queue clean and reduces unnecessary storage. The authorization checks ensure that only the contract owner can withdraw funds.
310-330: LGTM!The new function
execute_update_default_validatorprovides flexibility to the contract owner to update the default validator. The authorization and validation checks ensure that only the contract owner can update the default validator and that the provided address is a valid validator.
366-393: LGTM!The changes to
on_validator_unstakeimprove the robustness of the function by accommodating both data and event-based payout time extraction. The function now handles scenarios where the payout time is provided in the response data or as an event attribute.
189-202: LGTM!The changes to
execute_unstakeimprove the unstaking process by pushing the unstaking tokens into the queue with a specified payout time. The payout time is now derived from the response data of the undelegation message, which will be handled in theon_validator_unstakefunction. The authorization checks ensure that only the contract owner can unstake tokens.
32-32: Please provide more information about the purpose and usage of theSetWithdrawAddressvariant.The
SetWithdrawAddressvariant has been added to theReplyIdenum, but it is not used anywhere in the provided code changes. Could you please clarify the intended purpose of this variant and how it will be utilized in the future?tests-integration/tests/validator_staking.rs (3)
72-72: LGTM!The changes to the
execute_claim_rewardfunction calls and the test case for claiming rewards when there are none look good. Asserting the contract balance instead of the owner's balance is a better approach to ensure the contract's state reflects the expected outcomes.Also applies to: 87-87, 92-96
218-218: LGTM!The changes to the
execute_claim_rewardfunction calls and the test case for claiming rewards look good. Asserting the contract balance instead of the owner's balance is a better approach to ensure the contract's state reflects the expected outcomes. The new test cases for unstaking an amount larger than staked and unstaking a zero amount improve the test coverage.Also applies to: 233-233, 238-242, 297-297
319-385: LGTM!The new test function
test_update_default_validatorlooks good. It correctly checks for invalid validator updates and successful updates. It also ensures that the staking operations correctly reflect the updated default validator.
| pub fn init(self, chain_name: String) { | ||
| let msg = kernel::InstantiateMsg { | ||
| chain_name, | ||
| owner: None, | ||
| }; | ||
| self.instantiate(&msg, None, None).unwrap(); | ||
| } |
There was a problem hiding this comment.
Handle potential errors during contract instantiation.
The init function assumes that the contract instantiation will always succeed by unwrapping the result. However, if the instantiation fails for any reason, it will cause the program to panic and terminate.
Consider replacing unwrap with proper error handling using match or ? operator to gracefully handle any potential errors during instantiation.
For example, you can use the ? operator to propagate the error:
self.instantiate(&msg, None, None)?;Or use match to handle the error explicitly:
match self.instantiate(&msg, None, None) {
Ok(_) => {},
Err(e) => {
// Handle the error, e.g., log it or return an error response
eprintln!("Failed to instantiate the contract: {}", e);
}
}| pub fn execute_store_key_address(self, key: String, value: String) { | ||
| self.execute(&kernel::ExecuteMsg::UpsertKeyAddress { key, value }, None) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
Handle potential errors during message execution.
The execute_store_key_address function assumes that the message execution will always succeed by unwrapping the result. However, if the execution fails for any reason, it will cause the program to panic and terminate.
Consider replacing unwrap with proper error handling using match or ? operator to gracefully handle any potential errors during execution.
For example, you can use the ? operator to propagate the error:
self.execute(&kernel::ExecuteMsg::UpsertKeyAddress { key, value }, None)?;Or use match to handle the error explicitly:
match self.execute(&kernel::ExecuteMsg::UpsertKeyAddress { key, value }, None) {
Ok(_) => {},
Err(e) => {
// Handle the error, e.g., log it or return an error response
eprintln!("Failed to execute the message: {}", e);
}
}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
45-45: Looks good, but please provide more details about the specific changes made.Updating the Validator Staking functionality based on the shrelock audit findings is a positive change that likely improves security and compliance. However, it would be helpful to provide a brief summary of the key updates made in response to the audit, so reviewers have more context.
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 (2)
- CHANGELOG.md (2 hunks)
- contracts/modules/andromeda-shunting/Cargo.toml (1 hunks)
Files skipped from review due to trivial changes (1)
- contracts/modules/andromeda-shunting/Cargo.toml
Additional comments not posted (1)
CHANGELOG.md (1)
54-54: Verify the compatibility fix has been tested with Rust 1.75.0.Fixing the shunting ado for compatibility with Rust 1.75.0 is important for maintainability and staying up-to-date with the language. Please confirm that the fix has been thoroughly tested with Rust 1.75.0 to ensure there are no regressions.
Motivation
Resolves following issues
UnbondingTimeare not accounted for sherlock-audit/2024-05-andromeda-ado-judging#54Implementation
Testing
E2E test added to test changes on local testnet
Version Changes
Updated
validator-stakingfrom0.2.1to0.2.2.Updated
shunting-adofrom0.2.1to0.2.2(Previous version was not compatible with rust 1.75.0 and above).Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
ibc-testspackage detailing testing prerequisites and workflows.Chores