Feat/e2e refactor #575
Feat/e2e refactor #575
Conversation
WalkthroughThe pull request introduces significant changes to the workflow and codebase for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 18
🧹 Outside diff range and nitpick comments (19)
ibc-tests/src/lib.rs (1)
1-3: Summary: Structural changes support E2E test refactoringThe additions of the
constantsandinterfacesmodules, along with the existinginterface_macromodule, provide a solid foundation for the E2E test refactoring described in the PR objectives. These changes will enhance reusability and facilitate better integration of E2E tests into the CI/CD workflow.Consider documenting the purpose and contents of each module in comments above their declarations to improve code readability and maintainability.
ibc-tests/src/interfaces/mod.rs (1)
1-5: LGTM! Well-structured module declarations.The new module declarations are well-organized and align perfectly with the PR objectives of refactoring E2E tests and introducing interfaces for Application Domain Objects (ADOs). This modular approach should enhance reusability and maintainability of the testing framework.
Consider adding a brief comment at the top of the file to explain its purpose, for example:
// This file declares the public interfaces used in E2E testing for various // Application Domain Objects (ADOs) and related functionalities.ibc-tests/Cargo.toml (1)
8-12: Approve test configuration changes and suggest cleanup.The change from "validator_staking" to "crowdfund" aligns with the PR objectives. However, consider removing the commented-out test configuration if it's no longer needed to keep the file clean.
If the "validator_staking" test is obsolete, apply this diff:
-# [[test]] -# name = "validator_staking" [[test]] name = "crowdfund"ibc-tests/src/constants.rs (2)
14-46: LGTM: Network constants are well-defined for local testing.The
NetworkInfoandChainInfoconstants are appropriately configured for local test networks. This setup ensures consistent testing environments.Suggestions for improvement:
- Consider adding comments to explain the purpose of each network/chain constant, especially distinguishing between TERRA and WASM networks.
- The gas price is set to 0.15 for both networks. Consider making this a separate constant for easier maintenance if it needs to change in the future.
Here's a suggested improvement for the gas price:
+const LOCAL_GAS_PRICE: f64 = 0.15; + pub const LOCAL_TERRA: ChainInfo = ChainInfo { kind: ChainKind::Local, chain_id: "localterraa-1", gas_denom: "uluna", - gas_price: 0.15, + gas_price: LOCAL_GAS_PRICE, // ... other fields }; pub const LOCAL_WASM: ChainInfo = ChainInfo { kind: ChainKind::Local, chain_id: "localwasma-1", gas_denom: "ubindo", - gas_price: 0.15, + gas_price: LOCAL_GAS_PRICE, // ... other fields };
1-46: LGTM: Well-structured constants file with a suggestion for documentation.The file is well-organized, with a logical separation between imports, mnemonic constants, and network-related constants. This structure makes it easy to locate and maintain these constants.
Suggestion for improvement:
Consider adding a file-level documentation comment (//!) at the beginning of the file to explain its purpose and the nature of the constants defined within. This would provide valuable context for developers working with this file.Example:
//! Constants for IBC tests //! //! This file contains constants used in IBC (Inter-Blockchain Communication) tests, //! including test account mnemonics and network configurations for local test environments. //! Note: The mnemonics in this file are for testing purposes only and should never be used //! in a production environment. // Rest of the file...build.sh (4)
33-35: LGTM! Consider adding comments for clarity.The addition of
OUT_FILE_IBC_TESTandOUT_FILE_PACKAGEvariables aligns well with the PR objectives to refactor E2E tests and integrate them into the CI/CD workflow. The naming and path structure are consistent and correct.Consider adding brief comments explaining the purpose of these new output paths for better maintainability:
+ # Output path for IBC test artifacts local OUT_FILE_IBC_TEST="./ibc-tests/artifacts/$BUILD_TARGET.wasm" + # Output path for Andromeda testing end-to-end artifacts local OUT_FILE_PACKAGE="./packages/andromeda-testing-e2e/artifacts/$BUILD_TARGET.wasm"
37-38: LGTM! Consider adding error handling.The addition of these copy commands ensures that the built contracts are available in the new locations for IBC tests and end-to-end testing, which aligns with the PR objectives.
Consider adding error handling to ensure the script fails if the copy operations are unsuccessful:
- cp $IN_FILE $OUT_FILE_IBC_TEST - cp $IN_FILE $OUT_FILE_PACKAGE + cp $IN_FILE $OUT_FILE_IBC_TEST || { echo "Failed to copy to IBC test artifacts"; exit 1; } + cp $IN_FILE $OUT_FILE_PACKAGE || { echo "Failed to copy to Andromeda testing artifacts"; exit 1; }
92-93: LGTM! Consider adding a safety check.The addition of these cleanup commands ensures a clean build environment by removing any existing artifacts, which is good practice.
Consider adding a safety check to prevent accidental deletion of important directories:
-rm -rf ./packages/andromeda-testing-e2e/artifacts -rm -rf ./ibc-tests/artifacts +[ -d "./packages/andromeda-testing-e2e/artifacts" ] && rm -rf ./packages/andromeda-testing-e2e/artifacts +[ -d "./ibc-tests/artifacts" ] && rm -rf ./ibc-tests/artifactsThis change ensures that the
rm -rfcommand is only executed if the directory exists, providing an additional layer of safety.
95-96: LGTM! Consider adding error handling.The addition of these
mkdircommands ensures that the necessary directories exist for storing the built artifacts, which is essential for the script's functionality.Consider adding error handling to ensure the script fails if the directory creation is unsuccessful:
-mkdir packages/andromeda-testing-e2e/artifacts -mkdir ibc-tests/artifacts +mkdir packages/andromeda-testing-e2e/artifacts || { echo "Failed to create Andromeda testing artifacts directory"; exit 1; } +mkdir ibc-tests/artifacts || { echo "Failed to create IBC test artifacts directory"; exit 1; }This change will make the script more robust by explicitly handling potential errors during directory creation.
.github/workflows/rust.yml (2)
96-105: Consider adding error handling to setup steps.The Docker and localrelayer setup steps look good, but they lack error handling. Consider adding checks to ensure each step completes successfully before moving to the next one. This will help prevent silent failures and make debugging easier if issues occur.
Here's an example of how you could improve the localrelayer setup:
- name: Setup localrelayer run: | cd ./localrelayer - make start-chains - make start-validators + make start-chains || exit 1 + make start-validators || exit 1 + echo "Localrelayer setup completed successfully"
106-119: LGTM: Toolchain setup and artifact download steps are well-configured.The Rust toolchain setup and artifact download steps are correctly configured. The use of a consistent Rust version (1.75.0) across jobs is good practice. Downloading artifacts to multiple locations seems intentional and likely necessary for different test scenarios.
A minor suggestion for improvement:
Consider adding a step to verify the downloaded artifacts, ensuring they are not corrupted during the download process. This could be a simple check of file existence or integrity.
Example:
- name: Verify downloaded artifacts run: | [ -d "./ibc-tests/artifacts" ] && [ "$(ls -A ./ibc-tests/artifacts)" ] || exit 1 [ -d "./packages/andromeda-testing-e2e/artifacts" ] && [ "$(ls -A ./packages/andromeda-testing-e2e/artifacts)" ] || exit 1 echo "Artifacts verified successfully"🧰 Tools
🪛 actionlint
106-106: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
ibc-tests/tests/validator_staking.rs (1)
Line range hint
1-191: Clarify the new testing strategy and update file documentation.The changes in this file have a significant impact on the testing of the validator staking functionality:
- Both tests in this file (
test_validator_stakingandtest_kicked_validator) have been marked as ignored, effectively disabling all tests in this module.- The
appmodule import andAppContractinterface have been removed, suggesting a shift in the testing strategy or architecture.These changes raise some concerns:
- What is the new strategy for testing validator staking functionality?
- Are these tests being replaced by tests in another file or module?
- How is the functionality previously covered by these tests now being verified?
Please provide clarification on the following:
- The overall testing strategy for validator staking functionality.
- The reason for removing the
appmodule andAppContractinterface.- Any new or alternative tests that cover the functionality previously tested by these ignored tests.
Additionally, consider updating the file documentation to reflect these changes and the new testing approach. This will help other developers understand the current state and purpose of this file.
ibc-tests/src/main.rs (1)
16-16: Approve changes and suggest removing unused importThe reorganization of the
AppContractinterface improves code modularity. However, thecontract_interfaceimport appears to be unused in the visible code.If
contract_interfaceis not used elsewhere in this file, consider removing it from the import statement:-use ibc_tests::{contract_interface, interfaces::app_interface::AppContract}; +use ibc_tests::interfaces::app_interface::AppContract;ibc-tests/src/interfaces/splitter_interface.rs (2)
23-23: Consider usingexpect()instead ofunwrap()for clearer error messages.Using
.unwrap()can cause a panic without providing context if an error occurs. Replacing.unwrap()with.expect("custom error message")provides more informative panic messages, aiding in debugging:-splitter_contract.upload().unwrap(); +splitter_contract.upload().expect("Failed to upload splitter contract"); - splitter_contract.code_id().unwrap(), + splitter_contract.code_id().expect("Failed to retrieve code ID"),Also applies to: 28-28
18-33: Add documentation comments to thepreparefunction for better clarity.Since
prepareis a public function, consider adding Rustdoc comments (///) to explain its purpose, parameters, and return value. This enhances code readability and helps other developers understand how to use it.ibc-tests/src/interfaces/app_interface.rs (2)
1-1: Remove unnecessary commented-out codeThe import statement on line 1 is commented out. If it is no longer needed, consider removing it to keep the code clean and maintain readability.
Apply this diff to remove the commented-out code:
-// use ibc_tests::contract_interface;
21-42: Add documentation comments to public methodsAdding Rust documentation comments (
///) to public methods improves code readability and helps other developers understand the functionality and usage of the methods.Apply this diff to include documentation comments:
+/// Initializes the `AppContract` with the given parameters. pub fn init( &self, andr_os: &MockAndromeda, name: impl Into<String>, app_components: Vec<AppComponent>, owner: Option<String>, ) { +/// Queries the address associated with a specific component name. pub fn query_address_by_component_name(&self, name: impl Into<String>) -> String {ibc-tests/tests/crowdfund.rs (2)
279-280: Ensure correct time calculations for end_timeThe end time is calculated using
daemon.block_info().unwrap().time.plus_days(1).nanos(), which may not account for potential time zone differences or daylight saving changes. Verify that the time calculations are robust and consider using a fixed timestamp or configurable duration.
153-158: Handle potential errors when cloningcw20_componentIn the match statement, when
use_native_tokenis false,cw20_componentis unwrapped without checking forNone. While the code logic ensures it's safe here, consider adding error handling or comments to clarify this behavior for future maintainability.Apply this diff to add a comment:
+ // Safe to unwrap here because `cw20_component` is guaranteed to be `Some` when `use_native_token` is false let denom = match use_native_token { true => Asset::NativeToken(chain_info.gas_denom.to_string()), false => Asset::Cw20Token(AndrAddr::from_string(format!( "./{}", cw20_component.clone().unwrap().name ))), };
📜 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 (16)
- .github/workflows/rust.yml (1 hunks)
- build.sh (2 hunks)
- contracts/non-fungible-tokens/andromeda-crowdfund/src/state.rs (2 hunks)
- ibc-tests/Cargo.toml (1 hunks)
- ibc-tests/src/constants.rs (1 hunks)
- ibc-tests/src/interface_macro.rs (1 hunks)
- ibc-tests/src/interfaces/app_interface.rs (1 hunks)
- ibc-tests/src/interfaces/crowdfund_interface.rs (1 hunks)
- ibc-tests/src/interfaces/cw20_interface.rs (1 hunks)
- ibc-tests/src/interfaces/cw721_interface.rs (1 hunks)
- ibc-tests/src/interfaces/mod.rs (1 hunks)
- ibc-tests/src/interfaces/splitter_interface.rs (1 hunks)
- ibc-tests/src/lib.rs (1 hunks)
- ibc-tests/src/main.rs (1 hunks)
- ibc-tests/tests/crowdfund.rs (1 hunks)
- ibc-tests/tests/validator_staking.rs (2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/rust.yml
106-106: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (28)
ibc-tests/src/lib.rs (2)
1-1: LGTM: Addition ofconstantsmoduleThe public declaration of the
constantsmodule aligns with the PR objectives. This will allow for centralized management of network configuration, promoting consistency across E2E tests.
3-3: LGTM: Addition ofinterfacesmoduleThe public declaration of the
interfacesmodule is in line with the PR objectives. This will facilitate the introduction of interfaces for Application Domain Objects (ADOs), enhancing the reusability of E2E tests.ibc-tests/src/interface_macro.rs (1)
21-21: Verify the new WASM file naming convention across the project.The change in how the WASM path is constructed (
format!("andromeda_{}@", $wasm_path)) appears to introduce a new naming convention for Andromeda-specific WASM files. This modification aligns with the PR's objective to refactor E2E tests, but it may have far-reaching implications:
- It could potentially break existing tests or configurations that rely on the old naming convention.
- Other parts of the project might need to be updated to use this new convention consistently.
Please run the following script to check for consistency and potential issues:
Consider updating any relevant documentation to reflect this new naming convention for WASM files.
ibc-tests/Cargo.toml (6)
19-19: Approve addition of rstest dependency.The addition of rstest (version 0.19.0) is a good choice. It will enhance the testing capabilities by allowing for parameterized tests and fixtures, which can lead to more robust and maintainable tests.
27-29: Approve addition of andromeda-cw20 dependency.The addition of the andromeda-cw20 dependency with the "testing" feature is appropriate. It aligns with the PR objectives of integrating CW20 token interactions and ensures that the latest version of the contract is used during testing.
31-33: Approve addition of andromeda-cw721 dependency.The addition of the andromeda-cw721 dependency with the "testing" feature is appropriate. It aligns with the PR objectives of integrating CW721 token interactions and ensures that the latest version of the contract is used during testing.
35-41: Approve addition of andromeda-crowdfund and andromeda-splitter dependencies.The addition of the andromeda-crowdfund and andromeda-splitter dependencies with the "testing" feature is appropriate. These additions align with the PR objectives, particularly the mention of adding an E2E test for the Crowdfund ADO. Using local paths ensures that the latest versions of these contracts are used during testing.
50-51: Approve addition of workspace dependencies.The addition of andromeda-fungible-tokens and andromeda-non-fungible-tokens as workspace dependencies is a good structural change. This move towards a more modular structure aligns with the PR objectives of enhancing reusability and should improve maintainability of the project.
55-56: Approve updates to cw20 and cw721 dependencies.Updating the cw20 and cw721 dependencies to use workspace versions is a good practice. This change ensures consistent versions are used across the workspace and aligns with the modular approach seen in other dependency changes. It should help maintain consistency and ease future updates.
ibc-tests/src/constants.rs (2)
1-4: LGTM: Imports are appropriate for the defined constants.The imports from the
cw_orchcrate are relevant to the network and chain information constants defined in this file.
6-12:⚠️ Potential issueSecurity concern: Hardcoded mnemonics in source code.
The file contains four hardcoded BIP39 mnemonics. Storing sensitive information like mnemonics in source code poses a significant security risk, especially if these are used for real accounts or on public networks.
Please clarify:
- Are these mnemonics intended solely for testing purposes?
- Will this file be included in the production build?
Consider the following recommendations:
- If these are for testing, clearly label them as such in the comments.
- Use environment variables or secure secret management solutions instead of hardcoding.
- Ensure this file is not included in production builds.
- If these are dummy mnemonics for testing, consider using a deterministic generation method instead of hardcoding.
To verify if these mnemonics are used elsewhere in the codebase, run:
✅ Verification successful
[/run_scripts]
[/run_scripts]
Mnemonics are confined to test code and secure from production exposure.
The hardcoded mnemonic constants in
ibc-tests/src/constants.rsare exclusively used within theibc-tests/tests/crowdfund.rsfile. Searches confirmed thatibc-testsis only included under test dependencies inCargo.tomland is not referenced elsewhere in the codebase.**Category: **
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of mnemonic constants rg "USER_MNEMONIC|RECIPIENT_MNEMONIC_1|RECIPIENT_MNEMONIC_2|PURCHASER_MNEMONIC_1" --type rustLength of output: 1723
Script:
#!/bin/bash # Check if constants.rs is imported in non-test Rust files rg 'constants\.rs' --type rust -g '!ibc-tests/**'Length of output: 51
Script:
#!/bin/bash # Search Cargo.toml for any dependencies on ibc-tests outside of test/dev sections rg '\bibc-tests\b' Cargo.toml --context 5 # Search for references to constants.rs outside the ibc-tests directory rg 'constants\.rs' --type rust -g '!ibc-tests/**'Length of output: 279
build.sh (1)
Line range hint
1-113: Overall, the changes tobuild.share well-implemented and align with the PR objectives.The modifications to the build script effectively support the refactoring of E2E tests and their integration into the CI/CD workflow. The new output paths for IBC test artifacts and Andromeda testing end-to-end artifacts are correctly managed, ensuring that the built contracts are available where needed.
The script maintains good structure and consistency with existing code. The suggested improvements for clarity, safety, and error handling will further enhance its robustness and maintainability.
Great job on these changes!
.github/workflows/rust.yml (2)
84-95: LGTM: Job setup and initial steps are well-structured.The new
ibc-testsjob is correctly set up to run E2E tests after the build job. The checkout steps for both the main repository and the localrelayer are properly configured. The use of a secret token for accessing the localrelayer repository is a good security practice.
84-122: Overall assessment: Well-structured integration of E2E tests into the CI/CD workflow.The addition of the
ibc-testsjob successfully integrates E2E testing into the CI/CD pipeline, aligning with the PR objectives. The job is well-structured and includes necessary steps for setting up the testing environment, including the local relayer and artifact management.Key strengths:
- Proper job dependencies ensuring artifacts are available.
- Consistent use of Rust toolchain version across jobs.
- Secure handling of repository access using secret tokens.
Areas for potential improvement:
- Enhanced error handling in setup steps.
- Verification of downloaded artifacts.
- Optimization of test execution, potentially allowing for parallel testing if feasible.
- Addition of timeouts to prevent long-running tests from blocking the workflow.
- Updating the actions-rs/toolchain action to the latest version.
These improvements will further enhance the reliability and efficiency of the E2E testing process within the CI/CD pipeline.
🧰 Tools
🪛 actionlint
106-106: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
ibc-tests/tests/validator_staking.rs (2)
43-43: Explain the reason for ignoring the test and create a tracking issue.The
#[ignore]attribute has been added to thetest_validator_stakingfunction, which prevents it from running automatically with the test suite. While this may be necessary in some cases, it's important to understand and document the reasoning behind this decision.
- Please provide an explanation for why this test is being ignored.
- Consider adding a comment above the
#[ignore]attribute explaining the reason for ignoring the test. For example:// TODO: This test is currently ignored due to [reason]. See issue #[number] for details. #[ignore]- Create an issue in the project's issue tracker to document this ignored test and track its resolution. Include details about why it's ignored and any steps needed to make it runnable again.
To ensure proper tracking, let's check if an issue has been created for this ignored test:
118-118: Clarify the reason for ignoring the test and improve documentation.The
#[ignore]attribute has been added to thetest_kicked_validatorfunction. While there's a comment suggesting to pause a validator before running this test, it's not clear if this is the reason for ignoring it.
- Please provide a clear explanation for why this test is being ignored. Is it related to the need to pause a validator?
- Enhance the existing comment to explain both the prerequisite of pausing a validator and the reason for ignoring the test. For example:
// TODO: This test requires a validator to be paused before execution. // It is currently ignored due to [specific reason]. See issue #[number] for details. #[ignore]- Create an issue in the project's issue tracker to document this ignored test and track its resolution. Include details about the prerequisite, why it's ignored, and any steps needed to make it runnable again.
Let's check if an issue has been created for this ignored test:
ibc-tests/src/main.rs (2)
Line range hint
46-52: Approve changes in main functionThe main function has been updated to use the imported
AppContract, which is consistent with the earlier import changes. The rest of the function's logic remains unchanged, maintaining existing functionality.
Line range hint
1-214: Summary of changes and their impactThe changes in this file primarily focus on improving code organization and modularity:
- The
AppContractinterface has been moved to a separate module and is now imported.- The local contract interface declaration for
AppContracthas been removed.- The main function has been updated to use the imported
AppContract.These changes enhance code maintainability without affecting the core functionality of the file. The main logic, including the
prepare_validator_stakingfunction, remains intact.ibc-tests/src/interfaces/cw721_interface.rs (5)
1-10: LGTM!The import statements are appropriate and include all necessary modules for functionality.
11-17: Correct use of thecontract_interface!macroThe
contract_interface!macro is correctly utilized to define theCw721Contract, ensuring proper integration with the Andromeda CW721 contract.
18-18: Type alias forChainis properly definedDefining
type Chain = DaemonBase<Wallet>;enhances code readability and simplifies type annotations.
20-28: Implementation ofquery_owner_ofis accurateThe
query_owner_ofmethod correctly constructs the query message and retrieves the owner information for a given token ID.
30-45:preparefunction effectively initializes the contractThe
preparefunction properly initializes, uploads, and publishes the CW721 contract within the testing environment.ibc-tests/src/interfaces/crowdfund_interface.rs (2)
28-87: Implementation of contract methods is correct and consistent.The methods within the
CrowdfundContractimplementation are well-structured and follow consistent patterns. The use ofself.executeandself.queryaligns with the best practices for contract interfaces.
68-68:⚠️ Potential issueMissing import of
Coinfromcosmwasm_std.In the
execute_purchasefunction,Coinis used but not imported. This will lead to a compilation error.Add the following import at the top of the file to fix this issue:
+use cosmwasm_std::Coin;Likely invalid or redundant comment.
ibc-tests/tests/crowdfund.rs (3)
82-90: Validate percentage allocations sum to 100%The total percentage allocated to recipients is 0.2 (20%) and 0.8 (80%), summing to 100%, which is correct. However, ensure that any future changes or additions to recipients maintain this total to prevent misallocation of funds.
Optionally, you can add a validation check in the code to enforce that the total percentage equals 100%.
393-400: Check for successful execution of CW20 token transferAfter executing
cw20_contract.execute_send, it's important to verify that the transaction was successful and the correct amount was sent. Ensure that error handling is in place to catch any issues during the token transfer.You can enhance the test by asserting the transaction result.
414-419: Verify fund distribution for CW20 tokensThe comment indicates that the splitter is not distributing CW20 tokens. Ensure that after ending the campaign, the CW20 tokens are correctly distributed to the recipients.
Run the following script to verify if the recipients have received the CW20 tokens:
This script outlines the steps to verify CW20 token balances for each recipient, ensuring that the distribution logic functions as intended.
| - name: Run IBC Tests | ||
| run: | | ||
| cargo test -p ibc-tests -- --test-threads 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing test execution and adding a timeout.
The IBC tests are correctly targeted using the -p ibc-tests flag. The use of --test-threads 1 ensures sequential test execution, which can be beneficial for E2E tests to avoid potential race conditions. However, this approach might significantly increase the total execution time.
Consider the following improvements:
- If possible, structure the tests to allow for parallel execution to reduce overall execution time.
- Add a timeout to the step to prevent excessively long-running tests from blocking the workflow.
Example with timeout:
- name: Run IBC Tests
timeout-minutes: 30 # Adjust as needed
run: |
cargo test -p ibc-tests -- --test-threads 1If parallel execution is feasible:
- name: Run IBC Tests
timeout-minutes: 30 # Adjust as needed
run: |
cargo test -p ibc-tests| - uses: actions-rs/toolchain@v1 | ||
| with: | ||
| toolchain: 1.75.0 | ||
| profile: minimal |
There was a problem hiding this comment.
Update the actions-rs/toolchain action to the latest version.
The static analysis tool has flagged that the runner for the "actions-rs/toolchain@v1" action is too old for use with GitHub Actions. To ensure compatibility and access to the latest features and bug fixes, it's recommended to update this action to the latest version.
Update the action as follows:
- uses: actions-rs/toolchain@v1
+ uses: actions-rs/[email protected]Alternatively, you could consider switching to the official GitHub Actions setup-rust action:
- uses: actions/setup-rust@v1
with:
rust-version: 1.75.0This official action is well-maintained and integrated with GitHub's systems.
🧰 Tools
🪛 actionlint
106-106: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| let splitter_contract = SplitterContract::new(daemon.clone()); | ||
| splitter_contract.upload().unwrap(); | ||
|
|
||
| let MockAndromeda { adodb_contract, .. } = &andr_os; |
There was a problem hiding this comment.
Correct the destructuring of andr_os to avoid unnecessary reference.
The destructuring pattern let MockAndromeda { adodb_contract, .. } = &andr_os; is unnecessarily taking a reference of andr_os, which is already a reference (&MockAndromeda). You can destructure andr_os directly without adding an extra &:
-let MockAndromeda { adodb_contract, .. } = &andr_os;
+let MockAndromeda { adodb_contract, .. } = andr_os;📝 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.
| let MockAndromeda { adodb_contract, .. } = &andr_os; | |
| let MockAndromeda { adodb_contract, .. } = andr_os; |
| let app_contract = AppContract::new(daemon.clone()); | ||
| app_contract.upload().unwrap(); |
There was a problem hiding this comment.
Handle potential errors instead of using unwrap() during contract upload
The use of unwrap() on app_contract.upload() can cause a panic if the upload fails. It's advisable to handle the possible error to ensure the program can respond appropriately.
Apply this diff to handle errors explicitly:
- app_contract.upload().unwrap();
+ if let Err(e) = app_contract.upload() {
+ // Handle the error, e.g., log it or propagate it
+ eprintln!("Contract upload failed: {}", e);
+ // You may choose to return an error or handle it as needed
+ }📝 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.
| let app_contract = AppContract::new(daemon.clone()); | |
| app_contract.upload().unwrap(); | |
| let app_contract = AppContract::new(daemon.clone()); | |
| if let Err(e) = app_contract.upload() { | |
| // Handle the error, e.g., log it or propagate it | |
| eprintln!("Contract upload failed: {}", e); | |
| // You may choose to return an error or handle it as needed | |
| } |
| adodb_contract.clone().execute_publish( | ||
| app_contract.code_id().unwrap(), | ||
| "app-contract".to_string(), | ||
| "0.1.0".to_string(), | ||
| ); |
There was a problem hiding this comment.
Check the result of execute_publish for errors
The execute_publish method may return a Result indicating whether the publish was successful. To ensure robustness, consider handling any potential errors.
Apply this diff to handle errors explicitly:
adodb_contract.clone().execute_publish(
app_contract.code_id().unwrap(),
"app-contract".to_string(),
"0.1.0".to_string(),
- );
+ ).unwrap_or_else(|e| {
+ // Handle the error, e.g., log it or propagate it
+ eprintln!("Publish failed: {}", e);
+ // You may choose to return an error or handle it as needed
+ });Committable suggestion was skipped due to low confidence.
| update_tier(storage, &tier)?; | ||
| set_tier_sales(storage, new_order.level.into(), sold_amount)?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unnecessary Update of the Tier
The call to update_tier(storage, &tier)?; may be unnecessary if the tier object has not been modified within this scope. Updating an unmodified object can lead to redundant writes and decreased performance.
Consider removing the update_tier call if there are no changes to the tier that need to be saved.
|
|
||
| sold_amount = sold_amount.checked_add(new_order.amount)?; |
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Arithmetic Operations Without Proper Error Handling
./contracts/fungible-tokens/andromeda-cw20-staking/src/allocated_rewards.rs:73: let new_cycle = last_distribution_cycle.checked_add(1);./contracts/fungible-tokens/andromeda-cw20-staking/src/contract.rs:217: let new_number = config.number_of_reward_tokens.checked_add(1);./contracts/finance/andromeda-vesting/src/contract.rs:343: .checked_add(claims_release_unit);
🔗 Analysis chain
Verify Arithmetic Operations for Potential Overflows
The line sold_amount = sold_amount.checked_add(new_order.amount)?; updates sold_amount by adding new_order.amount. Ensure that checked_add is used to handle any potential overflows, and confirm that the ? operator appropriately handles the None case by propagating the error.
To automate the verification, run the following script to check all instances of arithmetic operations using checked_add without proper error handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all arithmetic operations use proper error handling.
# Test: Search for arithmetic operations and ensure they handle the Option type correctly.
# Expect: All uses of `checked_add`, `checked_sub`, etc., should be followed by `?`.
ast-grep --lang rust --pattern '$_ = $_.checked_add($_)' \
--output 'Found arithmetic operation without error handling at $FILE:$LINE'
Length of output: 334
Script:
#!/bin/bash
# Description: Verify that all arithmetic operations use proper error handling.
# Test: Search for arithmetic operations using `checked_add`, `checked_sub`, etc., without the `?` operator.
# Expect: All uses should be followed by `?` to handle errors appropriately.
ast-grep --lang rust --pattern '$_ = $_.checked_add($_)' --match-arg=false
Length of output: 285
Script:
#!/bin/bash
# Description: Verify that all arithmetic operations use proper error handling.
# Test: Search for arithmetic operations using `checked_add`, `checked_sub`, etc., without the `?` operator.
# Expect: All uses should be followed by `?` to handle errors appropriately.
ast-grep --lang rust --pattern '$_ = $_.checked_add($_)' --print-results
Length of output: 295
Script:
#!/bin/bash
# Description: Verify that all arithmetic operations use proper error handling.
# Test: Search for `checked_add` and `checked_sub` operations that are not followed by the `?` operator.
# Expect: Lines where `checked_add` or `checked_sub` are called without `?` for error handling.
rg --ignore-case --with-filename --line-number \
'checked_(add|sub)\s*\([^)]*\)\s*;' \
.
Length of output: 477
| let cw20_component = AppComponent::new( | ||
| "cw20".to_string(), | ||
| "cw20".to_string(), | ||
| to_json_binary(&cw20_init_msg).unwrap(), | ||
| ); | ||
| app_components.push(cw20_component.clone()); | ||
| Some(cw20_component) | ||
| } | ||
| }; | ||
|
|
||
| let denom = match use_native_token { | ||
| true => Asset::NativeToken(chain_info.gas_denom.to_string()), | ||
| false => Asset::Cw20Token(AndrAddr::from_string(format!( | ||
| "./{}", | ||
| cw20_component.clone().unwrap().name | ||
| ))), | ||
| }; | ||
|
|
||
| let withdrawal_recipient = Recipient::new( | ||
| format!("./{}", splitter_component.name), | ||
| Some(to_json_binary(&splitter::ExecuteMsg::Send {}).unwrap()), | ||
| ); | ||
|
|
||
| let campaign_config = CampaignConfig { | ||
| title: Some("Demo Crowdfund".to_string()), | ||
| description: Some("Crowdfund For Internal Testing".to_string()), | ||
| banner: Some("http://<campaign_banner>".to_string()), | ||
| url: Some("http://<campaign_url>".to_string()), | ||
| token_address: AndrAddr::from_string(format!("./{}", cw721_component.name)), | ||
| denom, | ||
| withdrawal_recipient, | ||
| soft_cap: Some(Uint128::new(100000)), | ||
| hard_cap: None, | ||
| }; | ||
|
|
||
| let crowdfund_init_msg = crowdfund::InstantiateMsg { | ||
| campaign_config, | ||
| kernel_address: kernel_address.clone(), | ||
| owner: None, | ||
| tiers: vec![], | ||
| }; | ||
|
|
||
| let crowdfund_component = AppComponent::new( | ||
| "crowdfund".to_string(), | ||
| "crowdfund".to_string(), | ||
| to_json_binary(&crowdfund_init_msg).unwrap(), | ||
| ); | ||
| app_components.push(crowdfund_component.clone()); | ||
|
|
||
| app_contract.init(&mock_andromeda, "Crowdfund App", app_components, None); | ||
| let crowdfund_addr = app_contract.query_address_by_component_name(crowdfund_component.name); | ||
| crowdfund_contract.set_address(&Addr::unchecked(crowdfund_addr)); | ||
|
|
||
| let cw721_addr = app_contract.query_address_by_component_name(cw721_component.name); | ||
| cw721_contract.set_address(&Addr::unchecked(cw721_addr)); | ||
|
|
||
| let splitter_addr = app_contract.query_address_by_component_name(splitter_component.name); | ||
| splitter_contract.set_address(&Addr::unchecked(splitter_addr)); | ||
|
|
||
| if !use_native_token { | ||
| let cw20_addr = app_contract.query_address_by_component_name(cw20_component.unwrap().name); | ||
| cw20_contract.set_address(&Addr::unchecked(cw20_addr)); | ||
| } | ||
|
|
||
| let meta_data = TierMetaData { | ||
| token_uri: None, | ||
| extension: TokenExtension { | ||
| ..Default::default() | ||
| }, | ||
| }; | ||
| crowdfund_contract.execute_add_tier( | ||
| "Tier 1".to_string(), | ||
| Uint64::one(), | ||
| Uint128::new(10000), | ||
| None, | ||
| meta_data.clone(), | ||
| ); | ||
| crowdfund_contract.execute_add_tier( | ||
| "Tier 2".to_string(), | ||
| Uint64::new(2u64), | ||
| Uint128::new(20000), | ||
| Some(Uint128::new(100)), | ||
| meta_data, | ||
| ); | ||
|
|
||
| let presale = vec![PresaleTierOrder { | ||
| level: Uint64::one(), | ||
| amount: Uint128::new(10u128), | ||
| orderer: Addr::unchecked(purchaser_1_daemon.sender().address()), | ||
| }]; | ||
|
|
||
| TestCase { | ||
| daemon, | ||
| crowdfund_contract, | ||
| cw20_contract, | ||
| cw721_contract, | ||
| splitter_contract, | ||
| presale, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the setup function for improved maintainability
The setup function spans nearly 200 lines, which can make it difficult to read and maintain. Consider breaking it down into smaller helper functions that initialize specific components or configurations. This will enhance readability and make future modifications easier.
| fn test_successful_crowdfund_app_native(#[with(true, LOCAL_WASM)] setup: TestCase) { | ||
| let TestCase { | ||
| daemon, | ||
| mut crowdfund_contract, | ||
| presale, | ||
| cw721_contract, | ||
| .. | ||
| } = setup; | ||
| let recipient_1_daemon = daemon | ||
| .rebuild() | ||
| .mnemonic(RECIPIENT_MNEMONIC_1) | ||
| .build() | ||
| .unwrap(); | ||
| let recipient_1_balance = daemon | ||
| .balance( | ||
| recipient_1_daemon.sender_addr(), | ||
| Some(LOCAL_WASM.gas_denom.to_string()), | ||
| ) | ||
| .unwrap()[0] | ||
| .amount; | ||
|
|
||
| let recipient_2_daemon = daemon | ||
| .rebuild() | ||
| .mnemonic(RECIPIENT_MNEMONIC_2) | ||
| .build() | ||
| .unwrap(); | ||
| let recipient_2_balance = daemon | ||
| .balance( | ||
| recipient_2_daemon.sender_addr(), | ||
| Some(LOCAL_WASM.gas_denom.to_string()), | ||
| ) | ||
| .unwrap()[0] | ||
| .amount; | ||
|
|
||
| let start_time = None; | ||
| let end_time = Milliseconds::from_nanos(daemon.block_info().unwrap().time.plus_days(1).nanos()); | ||
|
|
||
| crowdfund_contract.execute_start_campaign(start_time, end_time, Some(presale)); | ||
|
|
||
| let summary = crowdfund_contract.query_campaign_summary(); | ||
| assert_eq!(summary.current_capital, 0); | ||
| assert_eq!(summary.current_stage, CampaignStage::ONGOING.to_string()); | ||
|
|
||
| let orders = vec![ | ||
| SimpleTierOrder { | ||
| level: Uint64::one(), | ||
| amount: Uint128::new(10), | ||
| }, | ||
| SimpleTierOrder { | ||
| level: Uint64::new(2), | ||
| amount: Uint128::new(10), | ||
| }, | ||
| ]; | ||
|
|
||
| let purchaser_1_daemon = daemon | ||
| .rebuild() | ||
| .mnemonic(PURCHASER_MNEMONIC_1) | ||
| .build() | ||
| .unwrap(); | ||
| crowdfund_contract.set_sender(purchaser_1_daemon.sender()); | ||
| let funds = vec![coin(500000, LOCAL_WASM.gas_denom)]; | ||
| crowdfund_contract.execute_purchase(orders, Some(&funds)); | ||
|
|
||
| crowdfund_contract.set_sender(daemon.sender()); | ||
| crowdfund_contract.execute_end_campaign(); | ||
|
|
||
| let summary = crowdfund_contract.query_campaign_summary(); | ||
| assert_eq!(summary.current_capital, 10 * 10000 + 20000 * 10); | ||
| assert_eq!(summary.current_stage, CampaignStage::SUCCESS.to_string()); | ||
|
|
||
| let recipient_1_change = daemon | ||
| .balance( | ||
| recipient_1_daemon.sender_addr(), | ||
| Some(LOCAL_WASM.gas_denom.to_string()), | ||
| ) | ||
| .unwrap()[0] | ||
| .amount | ||
| - recipient_1_balance; | ||
|
|
||
| let recipient_2_change = daemon | ||
| .balance( | ||
| recipient_2_daemon.sender_addr(), | ||
| Some(LOCAL_WASM.gas_denom.to_string()), | ||
| ) | ||
| .unwrap()[0] | ||
| .amount | ||
| - recipient_2_balance; | ||
|
|
||
| assert_eq!(recipient_1_change.u128(), summary.current_capital / 5); | ||
| assert_eq!(recipient_2_change.u128(), summary.current_capital * 4 / 5); | ||
|
|
||
| crowdfund_contract.set_sender(purchaser_1_daemon.sender()); | ||
| crowdfund_contract.execute_claim(); | ||
|
|
||
| let owner_resp = cw721_contract.query_owner_of("0".to_string()).owner; | ||
| assert_eq!(owner_resp, purchaser_1_daemon.sender_addr().into_string()); | ||
|
|
||
| let owner_resp = cw721_contract.query_owner_of("29".to_string()).owner; | ||
| assert_eq!(owner_resp, purchaser_1_daemon.sender_addr().into_string()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate common logic in test functions
Both test_successful_crowdfund_app_native and test_successful_crowdfund_app_cw20 share similar logic for setting up orders, purchasing, ending the campaign, and claiming tokens. Consider abstracting the common code into helper functions to reduce duplication and enhance code clarity.
| cw20_contract.set_sender(purchaser_1_daemon.sender()); | ||
| cw20_contract.execute_send( | ||
| crowdfund_contract.addr_str().unwrap(), | ||
| Uint128::new(500000), | ||
| &hook_msg, | ||
| ); |
There was a problem hiding this comment.
Ensure the splitter contract handles CW20 tokens
In the test test_successful_crowdfund_app_cw20, the splitter contract does not distribute CW20 tokens to the recipients. This is highlighted by the comment that the splitter only works for native tokens. To fully test the crowdfunding functionality with CW20 tokens, consider updating the splitter contract to handle CW20 token distribution.
Apply this diff to update the splitter contract implementation:
- // Current implementation only handles native tokens
+ // Update the splitter contract to handle CW20 tokens
+ // Adjust the splitter's `ExecuteMsg` and logic to support CW20 transfersThis change will ensure that the funds collected in CW20 tokens are properly distributed to the designated recipients, maintaining consistency with the native token handling.
Committable suggestion was skipped due to low confidence.
| @@ -51,6 +40,7 @@ pub const LOCAL_TERRA: ChainInfo = ChainInfo { | |||
| }; | |||
|
|
|||
| #[test] | |||
| #[ignore] | |||
There was a problem hiding this comment.
Why is this test being ignored?
There was a problem hiding this comment.
Validator staking tests require some preparation steps. For example staking and letting the validator leave the validator set etc. Thus in normal github workflow, the validator staking results in infinite loop. Validator staking test cases will be the only exception in e2e test cases.
| @@ -125,6 +115,7 @@ fn test_validator_staking() { | |||
| } | |||
|
|
|||
| #[test] | |||
| #[ignore] | |||
Motivation
Refactor the E2E test in a more reusable way.
Integrate e2e test into CI/CD workflow
Implementation
E2E test is refactored similar to the integration test.
ibc-tests/src/interfacesfolderibc-tests/src/constants.rsibc-tests/src/main.rsis only used for validatr staking test casesTesting
E2E test for Crowdfund ADO is added.
Summary by CodeRabbit
New Features
ibc-testsfor running end-to-end tests in the CI workflow.Bug Fixes
Tests