feat(00-hello-world-counter): implement the classic, counter contract for testing #155
Conversation
WalkthroughThe changes encompass the introduction of a new smart contract named "hello-world-counter," along with updates to various WebAssembly (WASM) files and their corresponding checksums. A new configuration file for building the project is added, and existing files are modified to reflect updates in checksums and project structure. The contract includes functionalities for counting, resetting, and querying a state, with necessary message types and state management implemented. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
participant State
User->>Contract: Instantiate(count)
Contract->>State: Store initial count
User->>Contract: Execute(Increment)
Contract->>State: Increment count
User->>Contract: Query(Count)
Contract->>State: Retrieve current count
State-->>User: Return current count
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: 5
🧹 Outside diff range and nitpick comments (10)
contracts/00-hello-world-counter/src/lib.rs (1)
1-5: Consider adding documentation for each module.While the structure is good, it would be beneficial to add brief documentation for each module. This can help other developers quickly understand the purpose of each part of the contract.
Consider adding doc comments like this:
/// Main contract logic pub mod contract; /// Message types for contract interaction pub mod msg; /// Contract state management pub mod state; /// Test utilities #[cfg(test)] pub mod tutil;contracts/00-hello-world-counter/README.md (1)
1-4: LGTM! Consider enhancing the README structure.The README content accurately describes the purpose of the "00-hello-world-counter" smart contract, aligning well with the PR objectives. It provides a clear, concise introduction to the contract's role as a foundational example.
To further improve the README, consider the following suggestions:
- Add a title using a top-level heading.
- Include more details about the contract's functionality.
- Add examples of how to interact with the contract.
Here's a suggested structure:
# Hello World Counter Contract The classic "counter" smart contract. It serves as a hello-world example on how to implement execute messages and queries in Wasm. ## Functionality This contract implements a simple counter that can be incremented, decremented, and reset. It demonstrates: - Basic state management - Handling of execute messages - Implementing query functions ## Usage Examples (Add examples of how to interact with the contract, such as incrementing the counter or querying its value)These additions would provide more context and guidance for developers working with this contract.
contracts/00-hello-world-counter/src/state.rs (2)
7-11: LGTM: State struct is well-defined, with a minor suggestion.The
Statestruct is correctly defined with appropriate fields for a counter contract. The use of#[cw_serde]ensures proper serialization and deserialization.Consider using
u64instead ofi64for thecountfield if negative values are not needed. This would more accurately represent a counter that only increases and potentially save some storage space.#[cw_serde] pub struct State { - pub count: i64, + pub count: u64, pub owner: Addr, }
1-11: Overall, excellent implementation of state management for a counter contract.This file provides a solid foundation for the counter contract's state management. It follows CosmWasm best practices, uses appropriate data types, and leverages the framework's features effectively. The code is concise, clear, and well-structured, making it easy to understand and maintain.
As the contract grows in complexity, consider separating different aspects of the state into multiple
Items or usingMapfor more complex data structures. This can improve performance and make the contract more modular.contracts/00-hello-world-counter/.cargo/config.toml (1)
6-9: Consider removing commented-out older aliasesWhile keeping the old aliases as comments can be helpful for immediate reference, it might clutter the configuration file. Since this project is version-controlled, the history of these changes is already preserved in the version control system. Consider removing these comments to maintain a cleaner and more concise configuration file.
contracts/00-hello-world-counter/Cargo.toml (2)
12-16: LGTM: Well-definedlibraryfeature with clear comments.The
libraryfeature is correctly implemented, allowing the crate to be used as a dependency in other smart contracts without export conflicts. The comments clearly explain its purpose.Consider adding a brief example of when to use this feature in the comments, to further clarify its usage for developers. For example:
# features.library: Use the library feature to disable all # instantiate/execute/query exports. This is necessary use this as a dependency # for another smart contract crate. +# Example: When importing this crate in another contract, add `features = ["library"]` +# to the dependency specification. library = []
1-31: Overall, the Cargo.toml file is well-structured and appropriate for a CosmWasm smart contract.This Cargo.toml file sets up a solid foundation for the "hello-world-counter" smart contract. It correctly defines the package metadata, library configuration, features, and dependencies. The use of workspace-level definitions ensures consistency across the project. The
libraryfeature is a particularly good practice for smart contract development.As the project grows, consider the following best practices:
- Regularly update dependencies to ensure security and performance improvements.
- If specific version constraints become necessary for certain dependencies, document the reasons in comments.
- Consider adding a
[package.metadata]section with additional information about the contract, such as its purpose or any specific usage instructions.contracts/00-hello-world-counter/src/tutil.rs (3)
13-13: LGTM: Good use of constant for test owner address.Using a constant for the test owner address is a good practice for consistency across tests.
Consider adding a brief comment explaining the purpose of this constant and the
easy_addr::addr!macro for better documentation.
15-29: LGTM: Well-structured setup function for contract testing.The
setup_contractfunction provides a convenient way to initialize the contract for testing. The use ofanyhow::Resultfor error handling and the assertion for instantiation behavior are good practices.Consider adding a brief comment explaining the purpose of the assertion on line 27. This would help other developers understand why we expect no messages upon instantiation.
39-47: LGTM: Useful utility functions for creating mock objects.The
mock_info_for_senderandmock_env_heightfunctions provide convenient ways to create mock objects for testing. They encapsulate common testing setup tasks, which can help reduce code duplication in tests.Consider adding brief comments above each function explaining their purpose and usage. This would improve the documentation and make it easier for other developers to understand and use these utilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (15)
Cargo.lockis excluded by!**/*.lockartifacts/broker_bank.wasmis excluded by!**/*.wasmartifacts/broker_staking.wasmis excluded by!**/*.wasmartifacts/core_token_vesting_v2.wasmis excluded by!**/*.wasmartifacts/cw3_fixed_multisig.wasmis excluded by!**/*.wasmartifacts/cw3_flex_multisig.wasmis excluded by!**/*.wasmartifacts/hello_world_counter.wasmis excluded by!**/*.wasmartifacts/incentives.wasmis excluded by!**/*.wasmartifacts/lockup.wasmis excluded by!**/*.wasmartifacts/nibi_stargate.wasmis excluded by!**/*.wasmartifacts/nibi_stargate_perp.wasmis excluded by!**/*.wasmartifacts/nusd_valuator.wasmis excluded by!**/*.wasmartifacts/pricefeed.wasmis excluded by!**/*.wasmartifacts/shifter.wasmis excluded by!**/*.wasmartifacts/token_vesting.wasmis excluded by!**/*.wasm
📒 Files selected for processing (10)
- artifacts/checksums.txt (1 hunks)
- contracts/00-hello-world-counter/.cargo/config.toml (1 hunks)
- contracts/00-hello-world-counter/Cargo.toml (1 hunks)
- contracts/00-hello-world-counter/README.md (1 hunks)
- contracts/00-hello-world-counter/src/contract.rs (1 hunks)
- contracts/00-hello-world-counter/src/lib.rs (1 hunks)
- contracts/00-hello-world-counter/src/msg.rs (1 hunks)
- contracts/00-hello-world-counter/src/state.rs (1 hunks)
- contracts/00-hello-world-counter/src/tutil.rs (1 hunks)
- contracts/broker-bank/Cargo.toml (1 hunks)
🔇 Additional comments (22)
contracts/00-hello-world-counter/src/lib.rs (2)
1-5: LGTM! Well-structured module organization.The overall structure of the contract is well-organized, following good practices for Rust smart contract development. The separation of concerns into
contract,msg, andstatemodules is commendable, and the inclusion of a test-onlytutilmodule is a good approach for managing test utilities.
1-5: Verify implementation of counter logic.The PR objectives mention implementing a classic counter contract, but the current file only defines the module structure. Let's verify that the counter logic is correctly implemented in the
contractmodule.contracts/00-hello-world-counter/src/state.rs (2)
1-3: LGTM: Imports are appropriate and necessary.The imports from the CosmWasm framework are well-chosen for the contract's state management needs.
cw_serdefor serialization,Addrfor address handling, andItemfor state storage are all relevant to the contract's functionality.
5-5: LGTM: STATE constant is well-defined.The
STATEconstant is appropriately defined as anItem<State>with a clear storage key. This approach aligns well with CosmWasm best practices for state management in smart contracts.contracts/00-hello-world-counter/.cargo/config.toml (3)
2-2: Improved WASM build commandThe addition of the
--libflag to thewasmalias is a good improvement. It explicitly specifies that we're building a library, which is typically the case for WASM contracts. This change makes the build command more precise and less prone to potential misinterpretation.
3-3: Consistent improvement in WASM debug build commandThe
wasm-debugalias has been updated consistently with thewasmalias, adding the--libflag. This change maintains consistency across build configurations and provides the same benefits of explicitness and precision for debug builds.
4-4: Schema generation now uses a binary targetThe
schemaalias has been updated to use--bininstead of--example. This suggests that the schema generation code is now a separate binary target in the project, which can lead to better code organization.Please ensure that the project structure and Cargo.toml file have been updated accordingly:
contracts/00-hello-world-counter/src/msg.rs (5)
1-2: LGTM: Imports are appropriate and follow CosmWasm conventions.The import statements are correct and follow the standard practices for CosmWasm contract development. The local
statemodule is imported, and thecw_serdemacro fromcosmwasm_schemais used, which is a common pattern in CosmWasm contracts for serialization and deserialization.
4-8: LGTM: ExecuteMsg enum is well-defined and follows CosmWasm best practices.The
ExecuteMsgenum is correctly defined with the#[cw_serde]attribute, which is essential for serialization in CosmWasm contracts. The two variants,IncrementandReset, provide a clear and concise interface for the counter operations. The use ofi64for the count in theResetvariant is appropriate for a wide range of counter values.The inline comments for each variant are helpful and provide clear explanations of their purposes.
10-16: LGTM: QueryMsg enum is well-defined and follows CosmWasm query conventions.The
QueryMsgenum is correctly defined with both the#[cw_serde]and#[derive(cosmwasm_schema::QueryResponses)]attributes, which are essential for query messages in CosmWasm contracts. The singleCountvariant is appropriate for this simple counter contract.The use of the
#[returns(state::State)]attribute is correct and clearly specifies the return type for the query. The comment explaining that the count returns the JSON-encoded state is helpful for understanding the query's behavior.
18-21: LGTM: InstantiateMsg struct is well-defined and appropriate for the counter contract.The
InstantiateMsgstruct is correctly defined with the#[cw_serde]attribute, which is essential for serialization in CosmWasm contracts. The singlecountfield of typei64is appropriate for initializing the counter state.The simplicity of this struct aligns well with the purpose of a basic counter contract, providing just the necessary information for instantiation.
1-21: Excellent implementation of message types for the counter contract.This file provides a well-structured and complete definition of all necessary message types for a basic counter contract in the CosmWasm framework. The use of appropriate attributes (
#[cw_serde],#[derive(cosmwasm_schema::QueryResponses)],#[returns]) demonstrates adherence to CosmWasm best practices.The code is clean, concise, and includes helpful comments that enhance readability. The message structures (
ExecuteMsg,QueryMsg, andInstantiateMsg) are well-designed and provide a clear interface for interacting with the counter contract.Overall, this implementation serves as an excellent example of how to define message types for a CosmWasm smart contract.
contracts/broker-bank/Cargo.toml (1)
6-6: LGTM: Workspace configuration for repository field.The change to use
repository = { workspace = true }is a good practice. It aligns therepositoryfield with the workspace configuration, which is consistent with thehomepagefield above it. This approach helps maintain consistency across multiple packages in a Rust workspace and simplifies management of common metadata.contracts/00-hello-world-counter/Cargo.toml (3)
1-7: LGTM: Package metadata is well-defined.The package metadata is correctly set up with appropriate name, version, and edition. Using workspace values for homepage and repository ensures consistency across the project.
9-10: LGTM: Appropriate library configuration for a smart contract.The crate-type configuration allows the package to be compiled as both a dynamic library (
cdylib) and a Rust library (rlib). This is ideal for smart contracts, enabling them to be loaded by a blockchain runtime and used as a dependency in other Rust projects.
30-31: LGTM: Dev dependency is appropriately defined.The dev dependency
easy-addris correctly specified withworkspace = true, ensuring version consistency with the rest of the project. This dependency is likely useful for testing purposes in the smart contract development process.artifacts/checksums.txt (3)
7-7: New file:hello_world_counter.wasmThe addition of
hello_world_counter.wasmaligns with the PR objective of implementing a classic counter contract for testing. This is a positive step towards addressing issue #2048 in the NibiruChain repository.Could you please provide more details about the functionality of this counter contract and how it can be used for testing purposes?
1-15: Acknowledge checksum updates and request additional information.The checksums for multiple WASM files have been updated, indicating changes in these files. While this is expected during development, it's important to ensure that these changes are intentional and documented.
Could you please provide a brief summary of the changes made to each modified WASM file? This will help in understanding the scope and impact of the changes.
#!/bin/bash # List all .wasm files in the repository wasm_files=$(fd -e wasm) # For each .wasm file, find the corresponding entry in the PR description or commit messages for file in $wasm_files; do echo "Searching for changes related to $file" gh pr view --json body | jq -r .body | grep -i "$file" git log -n 5 --grep="$file" done
1-15: Verify integrity of WASM filesTo ensure the integrity of the WASM files and maintain security, it's crucial to verify that the checksums in this file match the actual files in the repository.
Please run the following script to verify the integrity of the WASM files:
This script will help ensure that the checksums in the file match the actual WASM files in the repository.
contracts/00-hello-world-counter/src/tutil.rs (3)
1-11: LGTM: Appropriate configuration and imports for test utilities.The file header, configuration, and imports are well-structured for a test utility file. The use of
#![cfg(not(target_arch = "wasm32"))]correctly ensures these utilities are only used in non-WASM environments, which is appropriate for testing purposes.
31-37: LGTM: Good use of default setup function.The
setup_contract_defaultsfunction provides a convenient way to initialize the contract with default values. It's a good use of the DRY principle by reusing thesetup_contractfunction.
1-47: Overall, excellent test utility implementation.This file provides a comprehensive set of test utilities for the smart contract. The code is well-structured, follows good practices, and should greatly facilitate testing of the contract. The use of mock objects and setup functions will help ensure consistent and reliable tests.
A few minor suggestions for improvement:
- Add brief comments explaining the purpose of constants and functions.
- Consider adding examples of how to use these utilities in actual tests.
Great job on implementing these test utilities!
| [dependencies] | ||
| cosmwasm-std = { workspace = true } | ||
| cosmwasm-schema = { workspace = true } | ||
| cw-storage-plus = { workspace = true } | ||
| schemars = { workspace = true } | ||
| serde = { workspace = true } | ||
| thiserror = { workspace = true } | ||
| nibiru-std = { workspace = true } | ||
| cw2 = { workspace = true } | ||
| serde_json = { workspace = true } | ||
| anyhow = { workspace = true } |
There was a problem hiding this comment.
💡 Codebase verification
Dependency Definition Issue: Missing in Workspace Root
The dependencies homepage and repository are not defined in the workspace root Cargo.toml. Please ensure these dependencies are correctly added to maintain consistency across the project.
homepagerepository
🔗 Analysis chain
LGTM: Dependencies are well-structured and appropriate.
The dependencies list includes all necessary libraries for CosmWasm smart contract development in the Nibiru ecosystem. Using workspace = true for all dependencies ensures version consistency across the project.
To ensure all dependencies are correctly defined in the workspace, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all dependencies are defined in the workspace root Cargo.toml
# Test: Check if all dependencies are defined in the workspace
grep -E '^\s*[a-zA-Z0-9_-]+ = \{ workspace = true \}' contracts/00-hello-world-counter/Cargo.toml | cut -d'=' -f1 | while read -r dep; do
if ! grep -q "$dep" Cargo.toml; then
echo "Warning: $dep is not defined in the workspace root Cargo.toml"
fi
done
Length of output: 715
Script:
#!/bin/bash
# Description: Verify that all dependencies in contracts/00-hello-world-counter/Cargo.toml are defined in the workspace root Cargo.toml
# Extract dependency names from the project Cargo.toml
DEPENDENCIES=$(grep -E '^\s*[a-zA-Z0-9_-]+ = \{ workspace = true \}' contracts/00-hello-world-counter/Cargo.toml | cut -d'=' -f1 | tr -d ' ')
# Iterate through each dependency and check its presence in the workspace root Cargo.toml
for dep in $DEPENDENCIES; do
if ! grep -q "^$dep\s*=" Cargo.toml; then
echo "Warning: Dependency '$dep' is not defined in the workspace root Cargo.toml"
fi
done
Length of output: 1476
| state::{State, STATE}, | ||
| }; | ||
|
|
||
| type ContractError = anyhow::Error; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defining a custom ContractError enum for better error handling.
Currently, ContractError is defined as anyhow::Error. Using a custom ContractError enum derived from thiserror::Error can provide more precise error handling and clearer error messages. This approach aligns with CosmWasm best practices and improves maintainability.
| let err = res.expect_err("err should be defined"); | ||
| let is_contained = err.to_string().contains(want_err); | ||
| assert!(is_contained, "got error {}", err); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in tests by matching error types instead of messages.
In the tests, errors are being matched by comparing error messages as strings, which can be fragile and may break if error messages change. Defining a custom ContractError enum with specific variants allows for more robust error handling and testing.
Consider updating your error handling approach:
- Define a custom
ContractErrorenum with variants for different error cases. - Implement
thiserror::Errorfor theContractErrorenum. - In your contract logic, return specific
ContractErrorvariants. - In tests, match against these variants instead of comparing error message strings.
| let state = STATE.load(deps.storage)?; | ||
| Ok(to_json_binary(&state)?) | ||
| } |
There was a problem hiding this comment.
Limit the data exposed in query responses.
The QueryMsg::Count {} handler returns the entire State, which includes the owner's address. To enhance security and privacy, consider returning only the count value, unless there's a specific need to expose the owner's address.
Refactor the query handler to return only the count:
- let state = STATE.load(deps.storage)?;
- Ok(to_json_binary(&state)?)
+ let count = STATE.load(deps.storage)?.count;
+ Ok(to_json_binary(&count)?)Also, update the corresponding response type in your msg module to reflect the new return structure.
📝 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 state = STATE.load(deps.storage)?; | |
| Ok(to_json_binary(&state)?) | |
| } | |
| let count = STATE.load(deps.storage)?.count; | |
| Ok(to_json_binary(&count)?) | |
| } |
| return Err(anyhow::anyhow!( | ||
| "Unauthorized: only the owner ({owner}) can use reset", | ||
| )); |
There was a problem hiding this comment.
Avoid exposing the owner's address in error messages.
Including the owner's address in the error message could lead to unwanted disclosure of sensitive information. It's recommended to provide a generic error message without revealing the owner's identity.
Apply this diff to modify the error message:
return Err(anyhow::anyhow!(
- "Unauthorized: only the owner ({owner}) can use reset",
+ "Unauthorized: only the owner can use reset",
));📝 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.
| return Err(anyhow::anyhow!( | |
| "Unauthorized: only the owner ({owner}) can use reset", | |
| )); | |
| return Err(anyhow::anyhow!( | |
| "Unauthorized: only the owner can use reset", | |
| )); |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores