-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_QUEUE
type 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_QUEUE
in the codebase are updated accordingly.packages/andromeda-finance/src/validator_staking.rs (1)
27-30
: Adddenom
andrecipient
fields toWithdrawFunds
.The
ExecuteMsg
enum now includesdenom
andrecipient
fields in theWithdrawFunds
variant. Ensure these fields are correctly utilized in the rest of the codebase.contracts/finance/andromeda-validator-staking/src/contract.rs (2)
87-89
: Updatehandle_execute
to includeWithdrawFunds
variant.The
handle_execute
function now includes a match arm for theWithdrawFunds
variant, invoking the newexecute_withdraw_fund
function. Ensure this integration is correct.
Line range hint
257-304
:
Implementexecute_withdraw_fund
function.The
execute_withdraw_fund
function is added to handle the withdrawal of funds, including the newdenom
andrecipient
parameters. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_fund
function has been correctly updated to callmock_execute_withdraw_fund
withNone
values fordenom
andrecipient
.
112-113
: LGTM!The
mock_execute_withdraw_fund
function has been correctly updated to acceptdenom
andrecipient
parameters and return anExecuteMsg
with these parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 validatingAirdropRequest
fields.Ensure that the
address
anddenom
fields are validated before use to prevent potential issues with invalid data.packages/andromeda-testing/Cargo.toml (1)
11-11
: Consider providing a description for thee2e
feature.The
e2e
feature 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
Makefile
are 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
--config
option.-## ./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
, andwhite
are 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_fund
function is called directly without any error handling. Consider wrapping the call in amatch
statement 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_fund
function 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.lock
is excluded by!**/*.lock
ibc-tests/package-lock.json
is 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_macro
module 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
faucet
module 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_path
are correct and that the trait implementations forUploadable
are 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_staking
appears correct.The test file
validator_staking.rs
is 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.rs
Scripts 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 = false
setting.Ensure that setting
publish = false
aligns 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-orch
indicates 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
Makefile
includes 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 thechain
variable.The
chain
variable appears to be unused in the script. Verify if it should be used or removed.Verification successful
Unused Variable Detected:
chain
The
chain
variable is set but not used elsewhere in theinstall.sh
script. 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
denom
andrecipient
parameters.Verification successful
Withdrawal functionality is already tested in
validator_staking.rs
.The
validator_staking.rs
file contains tests for the withdrawal functionality, specifically testing theexecute_withdraw_fund
function. The concerns about testing scenarios with and without thedenom
andrecipient
parameters 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
denom
andrecipient
areNone
.
@@ -0,0 +1,124 @@ | |||
|
|||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=32
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.
#!/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forAirdropRequest
fields.The
AirdropRequest
struct currently lacks validation for its fields. Consider adding checks to ensure thataddress
anddenom
are 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_interface
macro 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.lock
is 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!
forKernelContract
is CorrectThe
contract_interface!
macro is correctly used inpackages/andromeda-testing-e2e/src/kernel.rs
for 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 forAdodbContract
is 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
anyhow
dependency 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
anyhow
to the latest version.The
anyhow
dependency in yourCargo.toml
is 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-tests
is correctly integrated.The addition of
ibc-tests
to 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-tests
is correctly integrated. Theibc-tests
directory contains a well-configuredCargo.toml
with 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-e2e
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is 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
backtraces
is 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
cdylib
andrlib
are 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-test
for testing andreqwest
for 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is 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 thefund
function.The
fund
function usesunwrap()
on therebuild
andbank_send
results, which can cause a panic if the request fails. Consider handling errors more gracefully by using proper error handling techniques, such asmatch
or?
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
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");
29-44
: 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.
25-85
: 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.
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_MNEMONIC
andLOCAL_TERRA
are correctly defined and necessary for the test setup.
27-43
: Contract interfaces are correctly defined.The contract interfaces
AppContract
andValidatorStakingContract
are well-defined and necessary for the test setup.
62-176
: Comprehensive test setup and execution.The
test_validator_staking
function 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usingdevelop
branch ...(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_staking
function 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_validator
function 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.lock
is 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 usingdevelop
branch ...(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 theinit
method 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 theinit
method.The
init
method usesunwrap()
on the result ofinstantiate
, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning aResult
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(()) }
27-29
: Handle potential errors in theexecute_store_key_address
method.The
execute_store_key_address
method usesunwrap()
on the result ofexecute
, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning aResult
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(()) }packages/andromeda-testing-e2e/src/adodb.rs (2)
17-24
: Handle potential errors in theinit
method.The
init
method usesunwrap()
on the result ofinstantiate
, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning aResult
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(()) }
26-37
: Handle potential errors in theexecute_publish
method.The
execute_publish
method usesunwrap()
on the result ofexecute
, which can cause a panic if the operation fails. Consider handling errors more gracefully by returning aResult
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(()) }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 themain
function sets up the test environment as expected.
97-204
: Validator staking preparation is well-implemented.The
prepare_validator_staking
function 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_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);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is 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
cosmrs
crate 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_staking
is 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_validator
is 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_unstake
enhance the robustness of the function and are approved.
Line range hint
218-225
: LGTM!The changes to the function
execute_claim
enhance the flexibility of the function and are approved.
86-88
: LGTM!The function
execute_stake
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is 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 ofutil
module.The introduction of the
util
module 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_leb128
correctly 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_data
is 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_leb128
anddecode_unstaking_response_data
are 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::WithdrawFunds
to handle new parameters.The update to
ExecuteMsg::WithdrawFunds
to acceptdenom
andrecipient
parameters 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_queue
handling ensure that expired unstaking requests are efficiently removed, improving the management of unstaked tokens.
Line range hint
273-319
: Approved: Revisedexecute_withdraw_fund
function.The revised
execute_withdraw_fund
function now correctly handles the withdrawal of the entire contract balance whendenom
is 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
denom
andrecipient
to draw fund messageUNSTAKING_QUEUE
to useItem<Vec>
instead ofDeque
execute_withdraw_funds
logic to withdraw contract's balance instead of manually calculated balanceTesting
Updated mock for withdraw message.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
ibc-tests
andandromeda-testing-e2e
packages, clarifying their setup and usage.Refactor