feat: updated withdraw fund logic to withdraw the whole balance #509
Conversation
WalkthroughThe updates to the Andromeda validator-staking module enhance fund withdrawal capabilities by introducing two new parameters, Changes
Assessment against linked issues
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- contracts/finance/andromeda-validator-staking/src/contract.rs (6 hunks)
- contracts/finance/andromeda-validator-staking/src/state.rs (1 hunks)
- packages/andromeda-finance/src/validator_staking.rs (1 hunks)
Additional context used
Learnings (1)
contracts/finance/andromeda-validator-staking/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 (4)
contracts/finance/andromeda-validator-staking/src/state.rs (1)
8-8: Update theUNSTAKING_QUEUEtype toItem<Vec<UnstakingTokens>>.The change from
Deque<UnstakingTokens>toItem<Vec<UnstakingTokens>>affects how the unstaking queue is stored and managed. Ensure that all references toUNSTAKING_QUEUEin the codebase are updated accordingly.packages/andromeda-finance/src/validator_staking.rs (1)
27-30: Adddenomandrecipientfields toWithdrawFunds.The
ExecuteMsgenum now includesdenomandrecipientfields in theWithdrawFundsvariant. Ensure these fields are correctly utilized in the rest of the codebase.contracts/finance/andromeda-validator-staking/src/contract.rs (2)
87-89: Updatehandle_executeto includeWithdrawFundsvariant.The
handle_executefunction now includes a match arm for theWithdrawFundsvariant, invoking the newexecute_withdraw_fundfunction. Ensure this integration is correct.
Line range hint
257-304:
Implementexecute_withdraw_fundfunction.The
execute_withdraw_fundfunction is added to handle the withdrawal of funds, including the newdenomandrecipientparameters. Ensure that:
- The recipient address is correctly determined.
- The contract's balance is queried and validated.
- Expired unstaking requests are removed.
- Funds are sent to the recipient.
The logic appears sound, but ensure thorough testing to cover edge cases.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- contracts/finance/andromeda-validator-staking/src/contract.rs (6 hunks)
- contracts/finance/andromeda-validator-staking/src/mock.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- contracts/finance/andromeda-validator-staking/src/contract.rs
Additional comments not posted (2)
contracts/finance/andromeda-validator-staking/src/mock.rs (2)
52-53: LGTM!The
execute_withdraw_fundfunction has been correctly updated to callmock_execute_withdraw_fundwithNonevalues fordenomandrecipient.
112-113: LGTM!The
mock_execute_withdraw_fundfunction has been correctly updated to acceptdenomandrecipientparameters and return anExecuteMsgwith these parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- contracts/finance/andromeda-validator-staking/src/mock.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- contracts/finance/andromeda-validator-staking/src/mock.rs
|
We should add tests (where possible) for the issues to demonstrate that they have been resolved. Some of them may be impossible without e2e tests but we should cover what we can for now. |
Tried to add integration tests but it did not work as the reports are related to the validator operation. For example:
It seems like validator related operations (especially sudo operation for slashing), cw-multi-test is not providing necessary simulation environment and I doubt if adding integration test for the reports is worthy now. |
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range, codebase verification and nitpick comments (11)
ibc-tests/configs/starship.yaml (2)
2-2: Remove trailing spaces.Trailing spaces are present on lines 2, 20, and 28. Removing them will improve the file's formatting.
2c2 - + 20c20 - + 28c28 - +Also applies to: 20-20, 28-28
Tools
yamllint
[error] 2-2: trailing spaces
(trailing-spaces)
32-32: Add a newline at the end of the file.Adding a newline at the end of the file ensures compatibility with various tools and editors.
32c32 - rest: 8081 + rest: 8081Tools
yamllint
[error] 32-32: no new line character at the end of file
(new-line-at-end-of-file)
packages/andromeda-testing/src/faucet.rs (1)
5-11: Consider validatingAirdropRequestfields.Ensure that the
addressanddenomfields are validated before use to prevent potential issues with invalid data.packages/andromeda-testing/Cargo.toml (1)
11-11: Consider providing a description for thee2efeature.The
e2efeature is currently an empty array. It might be beneficial to provide a brief description or plan for its intended use to guide future development.Makefile (1)
14-59: Consider reviewing the commented-out sections.Several sections of the
Makefileare commented out. It might be useful to review these sections to determine if they are needed or if they can be removed to clean up the file.ibc-tests/scripts/install.sh (1)
6-6: Fix typo in usage comment.There's a typo in the usage comment for the
--configoption.-## ./scripts/install.sh --coinfig <config_file> +## ./scripts/install.sh --config <config_file>ibc-tests/scripts/port-forward.sh (1)
8-8: Remove unused color variables.The color variables
black,red,yellow,blue,magenta,cyan, andwhiteare defined but not used.- local black=30 red=31 green=32 yellow=33 blue=34 magenta=35 cyan=36 white=37 + local green=32Tools
Shellcheck
[warning] 8-8: black appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: red appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: yellow appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: blue appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: magenta appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: cyan appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: white appears unused. Verify use (or export if used externally).
(SC2034)
ibc-tests/tests/validator_staking.rs (2)
1-24: Consider removing unused imports.Several imports, such as
serde::{Deserialize, Serialize}andtokio::runtime::Runtime, are not used in this file. Consider removing them to improve readability.- use serde::{Deserialize, Serialize}; - use tokio::runtime::Runtime;
25-33: Consider removing commented-out code.The commented-out mnemonic and address lines seem unnecessary for the test and could be removed for clarity.
- // const GENESIS_VALIDATOR_MNEMONIC: &str = "issue have volume expire shoe year finish poem alien urban license undo rural endless food host opera fix forum crack wide example firm learn"; - // razor dog gown public private couple ecology paper flee connect local robot diamond stay rude join sound win ribbon soup kidney glass robot vehicle - // osmo1qjtcxl86z0zua2egcsz4ncff2gzlcndz2jeczk - // OSMO1C3DDFF822002C6BE6A61AB51230A66272DF6BCEFcontracts/finance/andromeda-validator-staking/src/contract.rs (2)
87-89: Enhance error handling inhandle_execute.The
execute_withdraw_fundfunction is called directly without any error handling. Consider wrapping the call in amatchstatement to provide more informative error messages or logging.ExecuteMsg::WithdrawFunds { denom, recipient } => { match execute_withdraw_fund(ctx, denom, recipient) { Ok(response) => Ok(response), Err(err) => Err(err), } }
Line range hint
262-308:
Improve error handling for invalid denominations.The
execute_withdraw_fundfunction currently usesexpect("Invalid denom")which could be replaced with proper error handling to avoid panics and provide more informative error messages.let funds = denom.map_or( deps.querier.query_all_balances(env.contract.address.clone())?, |d| { deps.querier .query_balance(env.contract.address.clone(), d) .map(|fund| vec![fund]) .map_err(|_| ContractError::InvalidDenom { denom: d }) // Define ContractError::InvalidDenom }, )?;
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 (14)
- Makefile (1 hunks)
- contracts/finance/andromeda-validator-staking/src/contract.rs (6 hunks)
- ibc-tests/Cargo.toml (1 hunks)
- ibc-tests/configs/starship.yaml (1 hunks)
- ibc-tests/scripts/dev-setup.sh (1 hunks)
- ibc-tests/scripts/install.sh (1 hunks)
- ibc-tests/scripts/port-forward.sh (1 hunks)
- ibc-tests/src/interface_macro.rs (1 hunks)
- ibc-tests/src/lib.rs (1 hunks)
- ibc-tests/tests/validator_staking.rs (1 hunks)
- packages/andromeda-testing/Cargo.toml (2 hunks)
- packages/andromeda-testing/src/faucet.rs (1 hunks)
- packages/andromeda-testing/src/lib.rs (1 hunks)
- packages/andromeda-testing/src/mock.rs (1 hunks)
Additional context used
Learnings (1)
contracts/finance/andromeda-validator-staking/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.
yamllint
ibc-tests/configs/starship.yaml
[error] 2-2: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 28-28: trailing spaces
(trailing-spaces)
[error] 32-32: no new line character at the end of file
(new-line-at-end-of-file)
Shellcheck
ibc-tests/scripts/dev-setup.sh
[warning] 8-8: black appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: red appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: yellow appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: blue appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: magenta appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: cyan appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: white appears unused. Verify use (or export if used externally).
(SC2034)
ibc-tests/scripts/install.sh
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
[warning] 64-64: chain appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 113-113: This pattern always overrides a later one on line 113.
(SC2221)
[warning] 113-113: This pattern never matches because of a previous pattern on line 113.
(SC2222)
ibc-tests/scripts/port-forward.sh
[warning] 8-8: black appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: red appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: yellow appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: blue appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: magenta appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: cyan appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: white appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 38-38: This pattern always overrides a later one on line 38.
(SC2221)
[warning] 38-38: This pattern never matches because of a previous pattern on line 38.
(SC2222)
Additional comments not posted (11)
ibc-tests/src/lib.rs (1)
1-1: Module declaration looks good!The introduction of the
interface_macromodule is straightforward. Ensure that the module is correctly implemented and integrated within the codebase.packages/andromeda-testing/src/lib.rs (1)
1-2: Conditional module addition looks good!The
faucetmodule is conditionally added under the "e2e" feature flag, which is a good practice for managing feature-specific code. Ensure that the module is implemented correctly and that its usage is well-documented.ibc-tests/src/interface_macro.rs (1)
1-25: Verify path handling and trait implementations.Ensure that the paths used in
find_wasm_pathare correct and that the trait implementations forUploadableare functioning as expected.ibc-tests/Cargo.toml (3)
42-44: Verify test configuration forvalidator_staking.Ensure that the test configuration is correct and aligns with the testing requirements.
Verification successful
Test configuration for
validator_stakingappears correct.The test file
validator_staking.rsis present in the codebase, and there are multiple references to it, indicating it is integrated into the project. However, manual verification is recommended to ensure the tests execute as expected.
- File:
ibc-tests/tests/validator_staking.rs- File:
tests-integration/tests/validator_staking.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test configuration for `validator_staking`. # Test: Check if the test is correctly configured and can be executed. cargo test --test validator_stakingLength of output: 110
Script:
#!/bin/bash # Search for the test file related to `validator_staking` in the codebase. fd 'validator_staking' --extension rs # Check for any references to `validator_staking` in the codebase to ensure it's correctly configured. rg 'validator_staking'Length of output: 7116
1-6: Verify thepublish = falsesetting.Ensure that setting
publish = falsealigns with the project's requirements and that the package is not intended for publishing.
8-52: Verify dependency compatibility and correctness.Ensure that the specified dependencies are compatible with each other and that the versions and features are correct.
packages/andromeda-testing/Cargo.toml (1)
46-49: New dependencies added for asynchronous and HTTP operations.The addition of
reqwest,tokio,cw-orch-daemon, andcw-orchindicates a move towards supporting asynchronous operations and HTTP requests, which aligns with the PR's testing enhancements.Makefile (1)
1-11: Active commands for cluster management.The
Makefileincludes active commands for cleaning, restarting, and starting clusters, which are essential for managing the testing environment.ibc-tests/scripts/install.sh (1)
64-64: Verify usage of thechainvariable.The
chainvariable appears to be unused in the script. Verify if it should be used or removed.Verification successful
Unused Variable Detected:
chainThe
chainvariable is set but not used elsewhere in theinstall.shscript. It can be safely removed unless intended for future use.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `chain` variable is used elsewhere in the codebase. # Test: Search for the usage of the `chain` variable. Expect: No additional occurrences. rg --type shell '\bchain\b'Length of output: 59
Script:
#!/bin/bash # Description: Verify if the `chain` variable is used elsewhere in the install.sh script. # Test: Search for the usage of the `chain` variable in the install.sh script. rg '\bchain\b' ibc-tests/scripts/install.shLength of output: 97
Tools
Shellcheck
[warning] 64-64: chain appears unused. Verify use (or export if used externally).
(SC2034)
ibc-tests/tests/validator_staking.rs (1)
104-313: Ensure comprehensive test coverage.The test currently sets up contracts and performs a staking operation but does not verify the withdrawal logic introduced in the PR. Consider adding tests to cover the new withdrawal functionality, including scenarios with and without the
denomandrecipientparameters.Verification successful
Withdrawal functionality is already tested in
validator_staking.rs.The
validator_staking.rsfile contains tests for the withdrawal functionality, specifically testing theexecute_withdraw_fundfunction. The concerns about testing scenarios with and without thedenomandrecipientparameters appear to be addressed by the existing tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage of the new withdrawal functionality. # Test: Search for test functions related to withdrawal. Expect: At least one test covering the withdrawal logic. rg --type rust 'withdraw' -A 5Length of output: 172865
contracts/finance/andromeda-validator-staking/src/contract.rs (1)
Line range hint
262-308:
Verify the correctness of withdrawal logic.The logic for determining the recipient and the funds to withdraw has changed significantly. Ensure that this logic is thoroughly tested, especially the scenarios where
denomandrecipientareNone.
| @@ -0,0 +1,124 @@ | |||
|
|
|||
| #!/bin/bash | |||
There was a problem hiding this comment.
Move the shebang to the first line.
The shebang must be on the first line to ensure the script is interpreted correctly.
Committable suggestion was skipped due to low confidence.
Tools
Shellcheck
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
| DRY_RUN=1 | ||
| shift # past argument | ||
| ;; | ||
| -*|--*) |
There was a problem hiding this comment.
Correct argument pattern matching logic.
The pattern matching logic for unknown options is incorrect, leading to unreachable code.
- -*|--*)
+ -*)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -*|--*) | |
| -*) |
Tools
Shellcheck
[warning] 113-113: This pattern always overrides a later one on line 113.
(SC2221)
[warning] 113-113: This pattern never matches because of a previous pattern on line 113.
(SC2222)
| #!/bin/bash | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| function color() { | ||
| local color=$1 | ||
| shift | ||
| local black=30 red=31 green=32 yellow=33 blue=34 magenta=35 cyan=36 white=37 | ||
| local color_code=${!color:-$green} | ||
| printf "\033[%sm%s\033[0m\n" "$color_code" "$*" | ||
| } | ||
|
|
||
| # Define a function to install a binary on macOS | ||
| install_macos() { | ||
| case $1 in | ||
| kubectl) brew install kubectl ;; | ||
| helm) brew install helm ;; | ||
| yq) brew install yq ;; | ||
| kind) brew install kind ;; | ||
| esac | ||
| } | ||
|
|
||
| # Define a function to install a binary on Linux | ||
| install_linux() { | ||
| color green "Installing $1 at ~/.local/bin, please add it to PATH" | ||
| mkdir -p ~/.local/bin | ||
| case $1 in | ||
| kubectl) curl -Lks "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" > ~/.local/bin/kubectl && chmod +x ~/.local/bin/kubectl ;; | ||
| helm) curl https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | bash ;; | ||
| yq) curl -Lks "https://github.com/mikefarah/yq/releases/download/v4.33.3/yq_linux_amd64" > ~/.local/bin/yq && chmod +x ~/.local/bin/yq ;; | ||
| kind) curl -Lks https://kind.sigs.k8s.io/dl/v0.18.1/kind-linux-amd64 > ~/.local/bin/kind && chmod +x ~/.local/bin/kind ;; | ||
| esac | ||
| } | ||
|
|
||
| # Define a function to install a binary | ||
| install_binary() { | ||
| if [[ $(uname -s) == "Darwin" ]]; then | ||
| install_macos $1 | ||
| else | ||
| install_linux $1 | ||
| fi | ||
| } | ||
|
|
||
| # Define a function to check for the presence of a binary | ||
| check_binary() { | ||
| if ! command -v $1 &> /dev/null | ||
| then | ||
| echo "$1 is not installed" | ||
| install_binary $1 | ||
| if ! command -v $1 &> /dev/null | ||
| then | ||
| color red "Installation of $1 failed, exiting..." | ||
| color red "Please install $1 manually, then run me again to verify the installation" | ||
| exit 1 | ||
| fi | ||
| fi | ||
| } | ||
|
|
||
| # Check the binaries | ||
| check_binary kubectl | ||
| check_binary helm | ||
| check_binary yq | ||
| check_binary kind | ||
|
|
||
| color green "All binaries are installed" |
There was a problem hiding this comment.
Well-structured setup script with minor improvements needed.
The script efficiently installs and checks for required binaries, providing a good setup for development. However, several color variables (black, red, yellow, blue, magenta, cyan, white) are defined but not used, which can be removed or utilized to improve the script.
- local black=30 red=31 green=32 yellow=33 blue=34 magenta=35 cyan=36 white=37
+ local green=32Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/bin/bash | |
| set -euo pipefail | |
| function color() { | |
| local color=$1 | |
| shift | |
| local black=30 red=31 green=32 yellow=33 blue=34 magenta=35 cyan=36 white=37 | |
| local color_code=${!color:-$green} | |
| printf "\033[%sm%s\033[0m\n" "$color_code" "$*" | |
| } | |
| # Define a function to install a binary on macOS | |
| install_macos() { | |
| case $1 in | |
| kubectl) brew install kubectl ;; | |
| helm) brew install helm ;; | |
| yq) brew install yq ;; | |
| kind) brew install kind ;; | |
| esac | |
| } | |
| # Define a function to install a binary on Linux | |
| install_linux() { | |
| color green "Installing $1 at ~/.local/bin, please add it to PATH" | |
| mkdir -p ~/.local/bin | |
| case $1 in | |
| kubectl) curl -Lks "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" > ~/.local/bin/kubectl && chmod +x ~/.local/bin/kubectl ;; | |
| helm) curl https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | bash ;; | |
| yq) curl -Lks "https://github.com/mikefarah/yq/releases/download/v4.33.3/yq_linux_amd64" > ~/.local/bin/yq && chmod +x ~/.local/bin/yq ;; | |
| kind) curl -Lks https://kind.sigs.k8s.io/dl/v0.18.1/kind-linux-amd64 > ~/.local/bin/kind && chmod +x ~/.local/bin/kind ;; | |
| esac | |
| } | |
| # Define a function to install a binary | |
| install_binary() { | |
| if [[ $(uname -s) == "Darwin" ]]; then | |
| install_macos $1 | |
| else | |
| install_linux $1 | |
| fi | |
| } | |
| # Define a function to check for the presence of a binary | |
| check_binary() { | |
| if ! command -v $1 &> /dev/null | |
| then | |
| echo "$1 is not installed" | |
| install_binary $1 | |
| if ! command -v $1 &> /dev/null | |
| then | |
| color red "Installation of $1 failed, exiting..." | |
| color red "Please install $1 manually, then run me again to verify the installation" | |
| exit 1 | |
| fi | |
| fi | |
| } | |
| # Check the binaries | |
| check_binary kubectl | |
| check_binary helm | |
| check_binary yq | |
| check_binary kind | |
| color green "All binaries are installed" | |
| #!/bin/bash | |
| set -euo pipefail | |
| function color() { | |
| local color=$1 | |
| shift | |
| local green=32 | |
| local color_code=${!color:-$green} | |
| printf "\033[%sm%s\033[0m\n" "$color_code" "$*" | |
| } | |
| # Define a function to install a binary on macOS | |
| install_macos() { | |
| case $1 in | |
| kubectl) brew install kubectl ;; | |
| helm) brew install helm ;; | |
| yq) brew install yq ;; | |
| kind) brew install kind ;; | |
| esac | |
| } | |
| # Define a function to install a binary on Linux | |
| install_linux() { | |
| color green "Installing $1 at ~/.local/bin, please add it to PATH" | |
| mkdir -p ~/.local/bin | |
| case $1 in | |
| kubectl) curl -Lks "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" > ~/.local/bin/kubectl && chmod +x ~/.local/bin/kubectl ;; | |
| helm) curl https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | bash ;; | |
| yq) curl -Lks "https://github.com/mikefarah/yq/releases/download/v4.33.3/yq_linux_amd64" > ~/.local/bin/yq && chmod +x ~/.local/bin/yq ;; | |
| kind) curl -Lks https://kind.sigs.k8s.io/dl/v0.18.1/kind-linux-amd64 > ~/.local/bin/kind && chmod +x ~/.local/bin/kind ;; | |
| esac | |
| } | |
| # Define a function to install a binary | |
| install_binary() { | |
| if [[ $(uname -s) == "Darwin" ]]; then | |
| install_macos $1 | |
| else | |
| install_linux $1 | |
| fi | |
| } | |
| # Define a function to check for the presence of a binary | |
| check_binary() { | |
| if ! command -v $1 &> /dev/null | |
| then | |
| echo "$1 is not installed" | |
| install_binary $1 | |
| if ! command -v $1 &> /dev/null | |
| then | |
| color red "Installation of $1 failed, exiting..." | |
| color red "Please install $1 manually, then run me again to verify the installation" | |
| exit 1 | |
| fi | |
| fi | |
| } | |
| # Check the binaries | |
| check_binary kubectl | |
| check_binary helm | |
| check_binary yq | |
| check_binary kind | |
| color green "All binaries are installed" |
Tools
Shellcheck
[warning] 8-8: black appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: red appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: yellow appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: blue appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: magenta appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: cyan appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 8-8: white appears unused. Verify use (or export if used externally).
(SC2034)
| async fn airdrop(addr: String, chain: &ChainInfo) { | ||
| let client = reqwest::Client::new(); | ||
| let url = chain.fcd_url.unwrap_or("http://localhost:8001"); | ||
| let url = format!("{}/credit", url); | ||
| client | ||
| .post(url) | ||
| .json(&AirdropRequest { | ||
| address: addr.to_string(), | ||
| denom: chain.gas_denom.to_string(), | ||
| }) | ||
| .send() | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
Add error handling for HTTP requests.
The airdrop function uses unwrap on the result of the HTTP request, which can cause a panic if the request fails. Consider handling errors gracefully.
let response = client
.post(url)
.json(&AirdropRequest {
address: addr.to_string(),
denom: chain.gas_denom.to_string(),
})
.send()
.await;
match response {
Ok(res) => {
// Handle successful response
}
Err(e) => {
// Handle error, e.g., log it or return an error
}
}| pub fn fund(addr: String, chain_info: &ChainInfo) { | ||
| let rt_handle = Runtime::new().unwrap(); | ||
| rt_handle.block_on(airdrop(addr, chain_info)); |
There was a problem hiding this comment.
Handle potential errors in runtime creation and consider async context.
Using unwrap for creating a new Tokio runtime can cause a panic if it fails. Additionally, consider whether using block_on is appropriate, as it can block the current thread.
let rt_handle = Runtime::new().expect("Failed to create Tokio runtime");Consider using an async context if possible to avoid blocking.
| CONFIGFILE="${i#*=}" | ||
| shift # past argument=value | ||
| ;; | ||
| -*|--*) |
There was a problem hiding this comment.
Correct argument pattern matching logic.
The pattern matching logic for unknown options is incorrect, leading to unreachable code.
- -*|--*)
+ -*)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -*|--*) | |
| -*) |
Tools
Shellcheck
[warning] 38-38: This pattern always overrides a later one on line 38.
(SC2221)
[warning] 38-38: This pattern never matches because of a previous pattern on line 38.
(SC2222)
| #[cfg(feature = "e2e")] | ||
| pub fn mock_app(chain: ChainInfo, mnemonic: &str) -> DaemonBase<Wallet> { | ||
| let daemon = Daemon::builder(chain.clone()) // set the network to use | ||
| .mnemonic(mnemonic) | ||
| .build() | ||
| .unwrap(); | ||
| fund(daemon.sender_addr().to_string(), &chain); | ||
| daemon | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in mock_app.
The mock_app function uses unwrap() which can cause a panic if an error occurs. Consider handling the error more gracefully.
let daemon = Daemon::builder(chain.clone())
.mnemonic(mnemonic)
.build()
.expect("Failed to build Daemon");| // Remove expired unstaking requests | ||
| let mut unstaking_queue = UNSTAKING_QUEUE.load(deps.storage)?; | ||
| unstaking_queue.retain(|token| token.payout_at >= env.block.time); | ||
| UNSTAKING_QUEUE.save(deps.storage, &unstaking_queue)?; |
There was a problem hiding this comment.
Optimize the unstaking queue cleanup.
The current logic for removing expired unstaking requests is functional but could be optimized by using drain_filter if available in the standard library or a similar method to avoid loading and saving the entire queue.
unstaking_queue.retain(|token| token.payout_at >= env.block.time);
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range, codebase verification and nitpick comments (2)
packages/andromeda-testing-e2e/src/faucet.rs (1)
5-11: Consider adding validation forAirdropRequestfields.The
AirdropRequeststruct currently lacks validation for its fields. Consider adding checks to ensure thataddressanddenomare valid and non-empty strings to prevent potential errors during request execution.packages/andromeda-testing-e2e/src/interface_macro.rs (1)
1-5: Define a clear purpose for the macro.The
contract_interfacemacro is well-structured, but consider adding comments to explain its purpose and usage. This will improve maintainability and readability.#[macro_export] /// Macro to define a contract interface with uploadable capabilities.
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 (11)
- Cargo.toml (2 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/mock.rs (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/andromeda-testing/src/mock.rs
Additional comments not posted (7)
packages/andromeda-testing-e2e/src/lib.rs (1)
1-20: LGTM! Conditional module declarations are appropriate.The use of
#[cfg(not(target_arch = "wasm32"))]ensures that these modules are only included for non-WASM32 architectures, which is a common practice in Rust for platform-specific code.packages/andromeda-testing-e2e/src/kernel.rs (1)
10-16: Verify the correctness of thecontract_interface!macro usage.Ensure that the macro
contract_interface!is correctly generating the expected code for theKernelContract. Verify that the identifiers and parameters match the intended contract interface.Verification successful
Macro Usage Verified:
contract_interface!forKernelContractis CorrectThe
contract_interface!macro is correctly used inpackages/andromeda-testing-e2e/src/kernel.rsfor theKernelContract. The identifiers and parameters match the intended contract interface as defined by the macro. No issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `contract_interface!` macro usage for `KernelContract`. # Test: Search for the macro definition and its usage. Expect: Correctly generated code for `KernelContract`. rg --type rust 'contract_interface!\s*\(\s*KernelContract' -A 5Length of output: 65
Script:
#!/bin/bash # Search for the definition of the `contract_interface!` macro in the codebase. rg --type rust 'macro_rules! contract_interface' -A 10 # Search for any usage of the `contract_interface!` macro in the codebase. rg --type rust 'contract_interface!' -A 5Length of output: 5825
packages/andromeda-testing-e2e/src/adodb.rs (1)
10-16: Verify the correctness of thecontract_interface!macro usage.Ensure that the macro
contract_interface!is correctly generating the expected code for theAdodbContract. Verify that the identifiers and parameters match the intended contract interface.Verification successful
The
contract_interface!macro usage forAdodbContractis correct.The macro is used as intended, with parameters matching its definition and consistent with other usages. The identifiers and parameters align with the expected contract interface.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `contract_interface!` macro usage for `AdodbContract`. # Test: Search for the macro definition and its usage. Expect: Correctly generated code for `AdodbContract`. rg --type rust 'contract_interface!\s*\(\s*AdodbContract' -A 5Length of output: 64
Script:
#!/bin/bash # Search for the definition of the `contract_interface!` macro. rg --type rust 'macro_rules!\s*contract_interface' -A 10 # Search for all usages of the `contract_interface!` macro to see how it is generally used. rg --type rust 'contract_interface!' -A 5Length of output: 5827
packages/andromeda-testing-e2e/Cargo.toml (2)
16-21: Consider updating dependency versions.The
anyhowdependency is specified as version "1.0.79". Ensure that this version is compatible with other dependencies and consider updating to the latest version if necessary.Verification successful
Consider updating
anyhowto the latest version.The
anyhowdependency in yourCargo.tomlis currently set to "1.0.79". The latest available version is "1.0.86". Consider updating to this version to benefit from any bug fixes or improvements, but ensure compatibility with other dependencies.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for updates to the `anyhow` dependency. # Test: List available versions of `anyhow`. cargo search anyhowLength of output: 94
Script:
#!/bin/bash # Description: Fetch the latest version of the `anyhow` crate from crates.io. # Use curl to get the latest version of the `anyhow` crate from crates.io API. curl -s https://crates.io/api/v1/crates/anyhow | jq '.crate.max_version'Length of output: 81
1-7: Verify the package metadata.Ensure that the package metadata, such as the version, authors, and description, accurately reflects the project's current state and intentions.
Cargo.toml (2)
15-15: Ensureibc-testsis correctly integrated.The addition of
ibc-teststo the workspace members suggests a focus on IBC-related testing. Verify that all necessary configurations and dependencies for these tests are in place.Verification successful
ibc-testsis correctly integrated. Theibc-testsdirectory contains a well-configuredCargo.tomlwith appropriate dependencies and features. It is included in the workspace members and referenced in the Makefile for service management, indicating proper integration into the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `ibc-tests`. # Test: Check for the presence of `ibc-tests` directory and its Cargo.toml. fd Cargo.toml ibc-testsLength of output: 46
Script:
#!/bin/bash # Description: Gather information about `ibc-tests` integration. # Check the contents of `ibc-tests/Cargo.toml` for dependencies and configurations. cat ibc-tests/Cargo.toml # Search for references to `ibc-tests` in the main `Cargo.toml` or other configuration files. rg 'ibc-tests' --glob '!ibc-tests/Cargo.toml' -A 5Length of output: 2076
41-41: Review the addition ofandromeda-testing-e2e.The new dependency
andromeda-testing-e2eis added to enhance end-to-end testing. Ensure that this package is correctly configured and integrated with existing tests.
| pub fn mock_app(chain: ChainInfo, mnemonic: &str) -> DaemonBase<Wallet> { | ||
| let daemon = Daemon::builder(chain.clone()) // set the network to use | ||
| .mnemonic(mnemonic) | ||
| .build() | ||
| .unwrap(); | ||
| fund(daemon.sender_addr().to_string(), &chain); | ||
|
|
||
|
|
||
| daemon | ||
| } |
There was a problem hiding this comment.
Consider handling potential errors instead of using unwrap.
The use of unwrap can lead to panics if an error occurs. Consider handling these errors gracefully to improve robustness.
let daemon = Daemon::builder(chain.clone())
.mnemonic(mnemonic)
.build()
.expect("Failed to build daemon");
fund(daemon.sender_addr().to_string(), &chain);| impl MockAndromeda { | ||
| pub fn new(daemon: &DaemonBase<Wallet>) -> MockAndromeda { | ||
| let chain_name: String = daemon.chain_info().network_info.chain_name.to_string(); | ||
|
|
||
| // Upload and instantiate os ADOs | ||
| let kernel_contract = KernelContract::new(daemon.clone()); | ||
| kernel_contract.upload().unwrap(); | ||
| kernel_contract.clone().init(chain_name); | ||
|
|
||
| let adodb_contract = AdodbContract::new(daemon.clone()); | ||
| adodb_contract.upload().unwrap(); | ||
| adodb_contract.clone().init(kernel_contract.addr_str().unwrap()); | ||
|
|
||
| let vfs_contract = VfsContract::new(daemon.clone()); | ||
| vfs_contract.upload().unwrap(); | ||
| vfs_contract.clone().init(kernel_contract.addr_str().unwrap()); | ||
|
|
||
| let economics_contract = EconomicsContract::new(daemon.clone()); | ||
| economics_contract.upload().unwrap(); | ||
| economics_contract.clone().init(kernel_contract.addr_str().unwrap()); | ||
|
|
There was a problem hiding this comment.
Improve error handling for contract operations.
The use of unwrap in contract operations can lead to panics. Consider using proper error handling to ensure robustness.
kernel_contract.upload().expect("Failed to upload kernel contract");
kernel_contract.clone().init(chain_name).expect("Failed to initialize kernel contract");Repeat similar changes for other contract operations.
| pub fn new(daemon: &DaemonBase<Wallet>) -> MockAndromeda { | ||
| let chain_name: String = daemon.chain_info().network_info.chain_name.to_string(); | ||
|
|
||
| // Upload and instantiate os ADOs | ||
| let kernel_contract = KernelContract::new(daemon.clone()); | ||
| kernel_contract.upload().unwrap(); | ||
| kernel_contract.clone().init(chain_name); | ||
|
|
||
| let adodb_contract = AdodbContract::new(daemon.clone()); | ||
| adodb_contract.upload().unwrap(); | ||
| adodb_contract.clone().init(kernel_contract.addr_str().unwrap()); | ||
|
|
||
| let vfs_contract = VfsContract::new(daemon.clone()); | ||
| vfs_contract.upload().unwrap(); | ||
| vfs_contract.clone().init(kernel_contract.addr_str().unwrap()); | ||
|
|
||
| let economics_contract = EconomicsContract::new(daemon.clone()); | ||
| economics_contract.upload().unwrap(); | ||
| economics_contract.clone().init(kernel_contract.addr_str().unwrap()); | ||
|
|
||
| // 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.clone().execute_publish( | ||
| vfs_contract.code_id().unwrap(), | ||
| "vfs".to_string(), | ||
| "0.1.0".to_string() | ||
| ); | ||
|
|
||
| adodb_contract.clone().execute_publish( | ||
| kernel_contract.code_id().unwrap(), | ||
| "kernel".to_string(), | ||
| "0.1.0".to_string() | ||
| ); | ||
|
|
||
|
|
||
| // update kernel | ||
| kernel_contract.clone().execute_store_key_address( | ||
| "adodb".to_string(), | ||
| adodb_contract.addr_str().unwrap() | ||
| ); | ||
| kernel_contract.clone().execute_store_key_address( | ||
| "vfs".to_string(), | ||
| vfs_contract.addr_str().unwrap() | ||
| ); | ||
| kernel_contract.clone().execute_store_key_address( | ||
| "economics".to_string(), | ||
| economics_contract.addr_str().unwrap() | ||
| ); | ||
|
|
||
| MockAndromeda { | ||
| kernel_contract, | ||
| adodb_contract, | ||
| vfs_contract, | ||
| economics_contract, | ||
| } | ||
| } |
There was a problem hiding this comment.
Evaluate the necessity of cloning contracts.
The frequent use of clone might be unnecessary and could impact performance. Consider passing references where possible to avoid unnecessary cloning.
let kernel_contract = KernelContract::new(daemon.clone());
kernel_contract.upload().expect("Failed to upload kernel contract");
kernel_contract.init(chain_name).expect("Failed to initialize kernel contract");Repeat similar changes for other contract operations.
| impl VfsContract<DaemonBase<Wallet>> { | ||
| pub fn init(self, kernel_address: String) { | ||
| let msg = vfs::InstantiateMsg { | ||
| kernel_address, | ||
| owner: None, | ||
| }; | ||
| self.instantiate(&msg, None, None).unwrap(); | ||
| } |
There was a problem hiding this comment.
Handle potential errors from unwrap().
The use of unwrap() in self.instantiate(&msg, None, None).unwrap(); can lead to a panic if the instantiation fails. Consider using proper error handling to manage this scenario gracefully.
self.instantiate(&msg, None, None).expect("Failed to instantiate VFS contract");| impl EconomicsContract<DaemonBase<Wallet>> { | ||
| pub fn init(self, kernel_address: String) { | ||
| let msg = economics::InstantiateMsg { | ||
| kernel_address, | ||
| owner: None, | ||
| }; | ||
| self.instantiate(&msg, None, None).unwrap(); | ||
| } |
There was a problem hiding this comment.
Handle potential errors from unwrap().
The use of unwrap() in self.instantiate(&msg, None, None).unwrap(); can lead to a panic if the instantiation fails. Consider using proper error handling to manage this scenario gracefully.
self.instantiate(&msg, None, None).expect("Failed to instantiate Economics contract");| pub fn fund(addr: String, chain_info: &ChainInfo) { | ||
| let rt_handle = Runtime::new().unwrap(); | ||
| rt_handle.block_on(airdrop(addr, chain_info)); |
There was a problem hiding this comment.
Ensure Runtime is properly managed in fund function.
The fund function creates a new Runtime but does not explicitly shut it down. Consider using a scoped runtime or ensuring that the runtime is properly dropped to avoid potential resource leaks.
pub fn fund(addr: String, chain_info: &ChainInfo) {
let rt_handle = Runtime::new().expect("Failed to create runtime");
rt_handle.block_on(airdrop(addr, chain_info)).expect("Airdrop failed");
rt_handle.shutdown_timeout(std::time::Duration::from_secs(1));
}| impl KernelContract<DaemonBase<Wallet>> { | ||
| 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 in the init method.
The init method uses unwrap() on the result of instantiate, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning a Result and propagating errors using the ? operator.
pub fn init(self, chain_name: String) -> Result<(), Box<dyn std::error::Error>> {
let msg = kernel::InstantiateMsg {
chain_name,
owner: None,
};
self.instantiate(&msg, None, None)?;
Ok(())
}| 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 in the execute_store_key_address method.
The execute_store_key_address method uses unwrap() on the result of execute, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning a Result and propagating errors using the ? operator.
pub fn execute_store_key_address(self, key: String, value: String) -> Result<(), Box<dyn std::error::Error>> {
self.execute(
&kernel::ExecuteMsg::UpsertKeyAddress {
key,
value,
},
None,
)?;
Ok(())
}| impl AdodbContract<DaemonBase<Wallet>> { | ||
| pub fn init(self, kernel_address: String) { | ||
| let msg = adodb::InstantiateMsg { | ||
| kernel_address, | ||
| owner: None, | ||
| }; | ||
| self.instantiate(&msg, None, None).unwrap(); | ||
| } |
There was a problem hiding this comment.
Handle potential errors in the init method.
The init method uses unwrap() on the result of instantiate, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning a Result and propagating errors using the ? operator.
pub fn init(self, kernel_address: String) -> Result<(), Box<dyn std::error::Error>> {
let msg = adodb::InstantiateMsg {
kernel_address,
owner: None,
};
self.instantiate(&msg, None, None)?;
Ok(())
}| pub fn execute_publish(self, code_id: u64, ado_type: String, version: String) { | ||
| self.execute( | ||
| &adodb::ExecuteMsg::Publish { | ||
| code_id: code_id, | ||
| ado_type, | ||
| action_fees: None, | ||
| version, | ||
| publisher: None, | ||
| }, | ||
| None, | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
Handle potential errors in the execute_publish method.
The execute_publish method uses unwrap() on the result of execute, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning a Result and propagating errors using the ? operator.
pub fn execute_publish(self, code_id: u64, ado_type: String, version: String) -> Result<(), Box<dyn std::error::Error>> {
self.execute(
&adodb::ExecuteMsg::Publish {
code_id,
ado_type,
action_fees: None,
version,
publisher: None,
},
None,
)?;
Ok(())
}
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 (4)
- ibc-tests/Cargo.toml (1 hunks)
- ibc-tests/tests/validator_staking.rs (1 hunks)
- packages/andromeda-testing-e2e/Cargo.toml (1 hunks)
- packages/andromeda-testing-e2e/src/mock.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- ibc-tests/Cargo.toml
Files skipped from review as they are similar to previous changes (2)
- ibc-tests/tests/validator_staking.rs
- packages/andromeda-testing-e2e/src/mock.rs
Additional comments not posted (5)
packages/andromeda-testing-e2e/Cargo.toml (5)
1-7: Package Metadata: Looks Good!The package metadata is complete and follows Rust's conventions. The information provided is clear and sufficient for identifying the package.
9-10: Feature Definition: Looks Good!The feature
backtracesis defined to enable backtraces incosmwasm-std. This is useful for debugging and aligns with best practices.
13-14: Library Configuration: Looks Good!The crate types
cdylibandrlibare specified correctly. This configuration supports both dynamic and static linking, which is appropriate for a testing utilities library.
43-49: Target-Specific Dependencies: Looks Good!The dependencies under the
not(target_arch = "wasm32")section are appropriate for non-WASM targets. This includescw-multi-testfor testing andreqwestfor HTTP requests, which are relevant for end-to-end testing scenarios.
16-41: Dependencies: Verify Compatibility and NecessityThe dependencies listed are comprehensive and appear to cover the necessary libraries for end-to-end testing. However, ensure that the versions specified are compatible with each other and with the rest of the project. Additionally, verify that each dependency is necessary for the intended functionality.
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)
- ibc-tests/Cargo.toml (1 hunks)
- ibc-tests/tests/validator_staking.rs (1 hunks)
- packages/andromeda-testing-e2e/Cargo.toml (1 hunks)
- packages/andromeda-testing-e2e/src/faucet.rs (1 hunks)
- packages/andromeda-testing-e2e/src/mock.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- ibc-tests/Cargo.toml
Additional comments not posted (9)
packages/andromeda-testing-e2e/src/faucet.rs (1)
6-13: Handle potential errors in thefundfunction.The
fundfunction usesunwrap()on therebuildandbank_sendresults, which can cause a panic if the request fails. Consider handling errors more gracefully by using proper error handling techniques, such asmatchor?operator, to provide better feedback and resilience.pub fn fund(daemon: &DaemonBase<Wallet>, chain_info: &ChainInfo, amount: u128) -> Result<(), Box<dyn std::error::Error>> { let target_addr = daemon.sender().pub_addr_str(); let faucet_daemon = daemon.rebuild().mnemonic(FAUCET_MNEMONIC).build()?; let rt = faucet_daemon.rt_handle.clone(); let wallet = faucet_daemon.sender(); rt.block_on(wallet.bank_send(&target_addr, coins(amount, chain_info.gas_denom.to_string())))?; Ok(()) }packages/andromeda-testing-e2e/Cargo.toml (1)
1-50: Cargo.toml configuration looks good.The package configuration, dependencies, and features are well-defined.
packages/andromeda-testing-e2e/src/mock.rs (3)
10-10: Consider handling potential errors instead of usingunwrap.The use of
unwrapcan lead to panics if an error occurs. Consider handling these errors gracefully to improve robustness.let daemon = Daemon::builder(chain.clone()) .mnemonic(mnemonic) .build() .expect("Failed to build daemon");
29-44: Improve error handling for contract operations.The use of
unwrapin contract operations can lead to panics. Consider using proper error handling to ensure robustness.kernel_contract.upload().expect("Failed to upload kernel contract"); kernel_contract.clone().init(chain_name).expect("Failed to initialize kernel contract");Repeat similar changes for other contract operations.
25-85: Evaluate the necessity of cloning contracts.The frequent use of
clonemight be unnecessary and could impact performance. Consider passing references where possible to avoid unnecessary cloning.let kernel_contract = KernelContract::new(daemon.clone()); kernel_contract.upload().expect("Failed to upload kernel contract"); kernel_contract.init(chain_name).expect("Failed to initialize kernel contract");Repeat similar changes for other contract operations.
ibc-tests/tests/validator_staking.rs (4)
1-22: Imports look good.The imported modules and libraries appear necessary for the test setup and execution.
23-60: Constants are well-defined.The constants
TESTNET_MNEMONICandLOCAL_TERRAare correctly defined and necessary for the test setup.
27-43: Contract interfaces are correctly defined.The contract interfaces
AppContractandValidatorStakingContractare well-defined and necessary for the test setup.
62-176: Comprehensive test setup and execution.The
test_validator_stakingfunction is well-structured and covers the necessary scenarios for staking and claiming rewards. The assertions ensure the correctness of contract balance operations.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
ibc-tests/README.md (1)
5-5: Improve grammatical clarity.Consider adding the article "the" and a comma for better readability.
Apply this diff to improve the sentence:
1. You need to run the local network for e2e test. Currently this package is using `develop` branch of [localrelayer](https://github.com/andromedaprotocol/localrelayer/). + You need to run the local network for the e2e test. Currently, this package is using the `develop` branch of [localrelayer](https://github.com/andromedaprotocol/localrelayer/).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)
ibc-tests/tests/validator_staking.rs (2)
55-169: Test function covers staking and withdrawal but consider uncommenting assertions.The
test_validator_stakingfunction covers the staking and withdrawal process. Uncommenting assertions can improve test coverage and validation.Consider uncommenting the assertions to ensure the test validates the expected outcomes.
171-295: Test function for kicked validator is well-implemented but consider uncommenting assertions.The
test_kicked_validatorfunction covers the scenario of a kicked validator. Uncommenting assertions can improve test coverage and validation.Consider uncommenting the assertions to ensure the test validates the expected outcomes.
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 (11)
- ibc-tests/Cargo.toml (1 hunks)
- ibc-tests/README.md (1 hunks)
- ibc-tests/src/main.rs (1 hunks)
- ibc-tests/tests/validator_staking.rs (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/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)
Files skipped from review as they are similar to previous changes (3)
- ibc-tests/Cargo.toml
- packages/andromeda-testing-e2e/src/faucet.rs
- packages/andromeda-testing-e2e/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)
Additional comments not posted (13)
packages/andromeda-testing-e2e/src/lib.rs (1)
1-20: LGTM!The module declarations are conditionally compiled for non-WASM32 architectures, which is appropriate for excluding modules not needed in a WASM environment.
packages/andromeda-testing-e2e/src/vfs.rs (1)
1-18: Approved with existing error handling comment.The contract interface and initialization logic are well-structured. The previous comment regarding the use of
unwrap()is still valid.packages/andromeda-testing-e2e/src/economics.rs (1)
17-24: Handle potential errors fromunwrap().The use of
unwrap()in theinitmethod can lead to a panic if the instantiation fails. Consider using proper error handling to manage this scenario gracefully.pub fn init(self, kernel_address: String) -> Result<(), Box<dyn std::error::Error>> { let msg = economics::InstantiateMsg { kernel_address, owner: None, }; self.instantiate(&msg, None, None)?; Ok(()) }packages/andromeda-testing-e2e/src/kernel.rs (2)
18-25: Handle potential errors in theinitmethod.The
initmethod usesunwrap()on the result ofinstantiate, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning aResultand propagating errors using the?operator.pub fn init(self, chain_name: String) -> Result<(), Box<dyn std::error::Error>> { let msg = kernel::InstantiateMsg { chain_name, owner: None, }; self.instantiate(&msg, None, None)?; Ok(()) }
27-29: Handle potential errors in theexecute_store_key_addressmethod.The
execute_store_key_addressmethod usesunwrap()on the result ofexecute, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning aResultand propagating errors using the?operator.pub fn execute_store_key_address(self, key: String, value: String) -> Result<(), Box<dyn std::error::Error>> { self.execute( &kernel::ExecuteMsg::UpsertKeyAddress { key, value, }, None, )?; Ok(()) }packages/andromeda-testing-e2e/src/adodb.rs (2)
17-24: Handle potential errors in theinitmethod.The
initmethod usesunwrap()on the result ofinstantiate, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning aResultand propagating errors using the?operator.pub fn init(self, kernel_address: String) -> Result<(), Box<dyn std::error::Error>> { let msg = adodb::InstantiateMsg { kernel_address, owner: None, }; self.instantiate(&msg, None, None)?; Ok(()) }
26-37: Handle potential errors in theexecute_publishmethod.The
execute_publishmethod usesunwrap()on the result ofexecute, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning aResultand propagating errors using the?operator.pub fn execute_publish(self, code_id: u64, ado_type: String, version: String) -> Result<(), Box<dyn std::error::Error>> { self.execute( &adodb::ExecuteMsg::Publish { code_id, ado_type, action_fees: None, version, publisher: None, }, None, )?; Ok(()) }ibc-tests/src/main.rs (6)
1-37: Imports and constants look good.The imports and constants are appropriate for setting up the test environment.
39-87: Contract interface and main function setup are correct.The
contract_interface!macro is used correctly, and themainfunction sets up the test environment as expected.
97-204: Validator staking preparation is well-implemented.The
prepare_validator_stakingfunction correctly sets up the validator staking contract and initializes it with validators.
89-95: ValidatorStakingContract interface is correctly defined.The
contract_interface!macro is used correctly for theValidatorStakingContract.
126-203: App initialization with validator staking is correctly implemented.The logic for initializing the app with validator staking and handling validators is correct.
185-203: Staking process for testing is well-implemented.The staking process iterates over validators and performs staking operations correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
contracts/finance/andromeda-validator-staking/src/contract.rs (1)
Line range hint
269-299: Optimize the unstaking queue cleanup.The current logic for removing expired unstaking requests is functional but could be optimized by using
drain_filterif available in the standard library or a similar method to avoid loading and saving the entire queue.unstaking_queue.retain(|token| token.payout_at >= env.block.time);
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 (3)
- contracts/finance/andromeda-validator-staking/Cargo.toml (1 hunks)
- contracts/finance/andromeda-validator-staking/src/contract.rs (7 hunks)
- ibc-tests/tests/validator_staking.rs (1 hunks)
Additional comments not posted (6)
contracts/finance/andromeda-validator-staking/Cargo.toml (1)
28-28: LGTM!The addition of the
cosmrscrate with version "0.19.0" is approved.ibc-tests/tests/validator_staking.rs (2)
55-171: Uncomment and validate the commented-out sections.The test
test_validator_stakingis comprehensive, but there are several commented-out sections that should be uncommented and validated to ensure full coverage.Uncomment the following sections and validate their functionality:
- // let user_balance = daemon - // .balance(daemon.sender_addr(), Some(denom.to_string())) - // .unwrap()[0].amount; + let user_balance = daemon + .balance(daemon.sender_addr(), Some(denom.to_string())) + .unwrap()[0].amount; - // let contract_balance = daemon - // .balance(validator_staking_contract.addr_str().unwrap(), Some(denom.to_string())) - // .unwrap()[0].amount; - // assert_eq!(contract_balance, Uint128::zero()); + let contract_balance = daemon + .balance(validator_staking_contract.addr_str().unwrap(), Some(denom.to_string())) + .unwrap()[0].amount; + assert_eq!(contract_balance, Uint128::zero()); - // let updated_user_balance = daemon - // .balance(daemon.sender_addr(), Some(denom.to_string())) - // .unwrap()[0].amount; + let updated_user_balance = daemon + .balance(daemon.sender_addr(), Some(denom.to_string())) + .unwrap()[0].amount; - // assert_eq!(user_balance + rewards_response.unwrap().accumulated_rewards[0].amount, updated_user_balance); + assert_eq!(user_balance + rewards_response.unwrap().accumulated_rewards[0].amount, updated_user_balance);
173-297: Uncomment and validate the commented-out sections.The test
test_kicked_validatoris comprehensive, but there are several commented-out sections that should be uncommented and validated to ensure full coverage.Uncomment the following sections and validate their functionality:
- // let user_balance = daemon - // .balance(daemon.sender_addr(), Some(denom.to_string())) - // .unwrap()[0].amount; + let user_balance = daemon + .balance(daemon.sender_addr(), Some(denom.to_string())) + .unwrap()[0].amount; - // let contract_balance = daemon - // .balance(validator_staking_contract.addr_str().unwrap(), Some(denom.to_string())) - // .unwrap()[0].amount; - // assert_eq!(contract_balance, Uint128::zero()); + let contract_balance = daemon + .balance(validator_staking_contract.addr_str().unwrap(), Some(denom.to_string())) + .unwrap()[0].amount; + assert_eq!(contract_balance, Uint128::zero()); - // let updated_user_balance = daemon - // .balance(daemon.sender_addr(), Some(denom.to_string())) - // .unwrap()[0].amount; + let updated_user_balance = daemon + .balance(daemon.sender_addr(), Some(denom.to_string())) + .unwrap()[0].amount; - // assert_eq!(user_balance + rewards_response.unwrap().accumulated_rewards[0].amount, updated_user_balance); + assert_eq!(user_balance + rewards_response.unwrap().accumulated_rewards[0].amount, updated_user_balance);contracts/finance/andromeda-validator-staking/src/contract.rs (3)
358-384: LGTM!The changes to the function
on_validator_unstakeenhance the robustness of the function and are approved.
Line range hint
218-225: LGTM!The changes to the function
execute_claimenhance the flexibility of the function and are approved.
86-88: LGTM!The function
execute_stakeis correctly implemented and no changes are needed.
| let fund = coin(unstake_amount.u128(), res.amount.denom); | ||
| let undelegate_msg = CosmosMsg::Staking(StakingMsg::Undelegate { | ||
| validator: validator.to_string(), | ||
| amount: coin(unstake_amount.u128(), res.amount.denom), | ||
| amount: fund.clone(), | ||
| }); | ||
|
|
||
| let mut unstaking_queue = UNSTAKING_QUEUE.load(deps.storage).unwrap_or_default(); | ||
| unstaking_queue.push(UnstakingTokens { | ||
| fund, | ||
| payout_at: Timestamp::default(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
Optimize the unstaking queue cleanup.
The current logic for removing expired unstaking requests is functional but could be optimized by using drain_filter if available in the standard library or a similar method to avoid loading and saving the entire queue.
unstaking_queue.retain(|token| token.payout_at >= env.block.time);
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 (6)
- contracts/finance/andromeda-validator-staking/src/contract.rs (7 hunks)
- contracts/finance/andromeda-validator-staking/src/lib.rs (1 hunks)
- contracts/finance/andromeda-validator-staking/src/util.rs (1 hunks)
- ibc-tests/src/main.rs (1 hunks)
- ibc-tests/tests/validator_staking.rs (1 hunks)
- packages/andromeda-testing-e2e/src/adodb.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- ibc-tests/src/main.rs
- ibc-tests/tests/validator_staking.rs
- packages/andromeda-testing-e2e/src/adodb.rs
Additional comments not posted (8)
contracts/finance/andromeda-validator-staking/src/lib.rs (1)
5-5: Approved: Addition ofutilmodule.The introduction of the
utilmodule enhances modularity and provides a dedicated space for utility functions or types, which is a positive change for the codebase.contracts/finance/andromeda-validator-staking/src/util.rs (3)
3-19: Approved: Implementation ofdecode_leb128.The function
decode_leb128correctly implements the LEB128 decoding algorithm, which is essential for handling certain data formats within the contract.
21-41: Approved: Implementation ofdecode_unstaking_response_data.The function
decode_unstaking_response_datais well-implemented, providing a clear and efficient method for extracting necessary data from unstaking response messages.
43-64: Approved: Comprehensive tests for utility functions.The tests for
decode_leb128anddecode_unstaking_response_dataare comprehensive and ensure that the functions behave as expected under various scenarios.Also applies to: 66-82
contracts/finance/andromeda-validator-staking/src/contract.rs (4)
88-90: Approved: UpdatedExecuteMsg::WithdrawFundsto handle new parameters.The update to
ExecuteMsg::WithdrawFundsto acceptdenomandrecipientparameters allows for more flexible fund withdrawal options, aligning with the PR objectives to improve fund management.
189-202: Approved: Enhanced handling ofunstaking_queue.The modifications to the
unstaking_queuehandling ensure that expired unstaking requests are efficiently removed, improving the management of unstaked tokens.
Line range hint
273-319: Approved: Revisedexecute_withdraw_fundfunction.The revised
execute_withdraw_fundfunction now correctly handles the withdrawal of the entire contract balance whendenomis not specified, and properly sets the recipient. This implementation meets the PR objectives and addresses the issues outlined in the linked issue.
300-303: Duplicate Comment: Optimize the unstaking queue cleanup.The optimization suggestion for the unstaking queue cleanup remains valid and should be considered to improve performance.
Motivation
Resolves #508
Implementation
denomandrecipientto draw fund messageUNSTAKING_QUEUEto useItem<Vec>instead ofDequeexecute_withdraw_fundslogic to withdraw contract's balance instead of manually calculated balanceTesting
Updated mock for withdraw message.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
ibc-testsandandromeda-testing-e2epackages, clarifying their setup and usage.Refactor