F/dummy contract auction #246
Conversation
…d not marshalling as coin correctly
WalkthroughThe pull request includes several modifications primarily centered around updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (12)
contracts/dummy/Cargo.toml (1)
34-34: LGTM! Consider adding a version constraint.The addition of the
injective-stddependency aligns with the PR objectives and enhances the functionality of the dummy contract. However, to ensure better dependency management and avoid potential compatibility issues in the future, consider adding a version constraint.You could modify the dependency line as follows:
-injective-std = { path = "../../packages/injective-std" } +injective-std = { path = "../../packages/injective-std", version = "0.1.0" }Replace "0.1.0" with the appropriate version number of the injective-std package.
contracts/injective-cosmwasm-stargate-example/src/testing/authz.rs (3)
49-49: Improved iterator chaining in grant combination.The simplification of the grant combination logic enhances both readability and potentially performance. By removing the unnecessary
into_iter()call onresponse_user2.grants, the code becomes more concise while maintaining its functionality.Consider further simplifying this line by using the
extendmethod:let mut combined_grants = response_user0.grants; combined_grants.extend(response_user2.grants);This approach may be more idiomatic and slightly more efficient, as it avoids creating an intermediate iterator.
131-131: Enhanced error handling in test_query_grants.The introduction of the
panic!macro improves the test's error handling by explicitly failing with a detailed error message when an unexpected success occurs. This change enhances the test's robustness and aids in debugging by including the actual response in the panic message.For consistency with Rust's testing conventions, consider using
assert!instead ofpanic!:assert!(false, "Expected an error, but got a success: {:?}", contract_response);This achieves the same result but aligns more closely with standard test assertions in Rust.
Line range hint
1-155: Overall improvements in test implementation and error handling.The changes in this file enhance the quality of the tests and improve error handling. The simplification of iterator chaining in
test_query_grantee_grantsand the introduction of explicit error handling intest_query_grantscontribute to more robust and maintainable test code. These modifications align well with Rust best practices and should make debugging easier in the future.Consider applying similar improvements to other test functions in this file for consistency. Additionally, it might be beneficial to add more detailed comments explaining the purpose and expected outcomes of each test, especially for complex scenarios involving multiple users and authorizations.
packages/injective-std/src/types/injective/auction/v1beta1.rs (4)
24-25: Remove commented-out code to maintain code cleanlinessThe old
amountfield definition is commented out. It is advisable to remove unused code to keep the codebase clean, as version control systems retain history.
38-39: Remove commented-out code to maintain code cleanlinessConsider removing the commented-out
amountfield to keep the codebase clean and maintainable.
59-60: Remove commented-out code to maintain code cleanlinessPlease remove the commented-out
amountfield to keep the code clean, as version control retains the history.
80-81: Remove commented-out code to maintain code cleanlinessConsider removing the commented-out code for the
amountfield to keep the codebase clean.contracts/dummy/src/contract.rs (4)
Line range hint
46-50: Return structured JSON object inExecuteMsg::PingresponseIn the
executefunction, when handlingExecuteMsg::Ping, consider returning a structured JSON object for better extensibility and clarity.Proposed change:
ExecuteMsg::Ping { .. } => { let mut response = Response::new(); - response.data = Some(to_json_binary("pong")?); + response.data = Some(to_json_binary(&serde_json::json!({"message": "pong"}))?); Ok(response) }
Line range hint
66-71: Directly serialize data types in query responsesIn the
queryfunction, forQueryMsg::RunsandQueryMsg::Active, avoid converting values to strings before serialization. Directly serializing the data types improves efficiency.Apply this change:
QueryMsg::Runs {} => { let runs_count = COUNTER.load(deps.storage)?; - to_json_binary(&format!("{runs_count}")) + to_json_binary(&runs_count) } QueryMsg::Active {} => { let is_active = ACTIVE.load(deps.storage)?; - to_json_binary(&format!("{is_active}")) + to_json_binary(&is_active) }
Line range hint
54-56: Clarify the intentional error inExecuteMsg::ErrorThe
ExecuteMsg::Errorvariant returns an error intentionally. Ensure this is documented clearly to prevent confusion for users interacting with the contract.
Line range hint
62-64: Handle potential overflow when incrementingCOUNTERWhen incrementing
COUNTERin thesudofunction forSudoMsg::BeginBlocker, there's a potential for overflow ifCOUNTERreachesu32::MAX. Consider adding overflow handling.Proposed change:
SudoMsg::BeginBlocker {} => { let runs = COUNTER.load(deps.storage)?; + let (new_runs, overflowed) = runs.overflowing_add(1); + if overflowed { + return Err(ContractError::Std(StdError::generic_err("COUNTER has reached its maximum value"))); + } - let runs = COUNTER.load(deps.storage)? + 1; - COUNTER.save(deps.storage, &runs)?; + COUNTER.save(deps.storage, &new_runs)?; ACTIVE.save(deps.storage, &true)?; Ok(Response::new()) }
📜 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 (8)
- Cargo.toml (1 hunks)
- contracts/dummy/Cargo.toml (1 hunks)
- contracts/dummy/src/contract.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/authz.rs (2 hunks)
- packages/injective-cosmwasm/Cargo.toml (1 hunks)
- packages/injective-std/Cargo.toml (1 hunks)
- packages/injective-std/src/types/injective/auction/v1beta1.rs (4 hunks)
- packages/injective-testing/Cargo.toml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/injective-std/Cargo.toml
🧰 Additional context used
🔇 Additional comments (3)
packages/injective-testing/Cargo.toml (1)
15-17: Consider the implications of removing version constraints for local dependencies.The changes to the dependency specifications for
injective-cosmwasm,injective-math, andinjective-stdremove the version constraints, leaving only the path specifications. While this allows for more flexibility in development by always using the latest local versions, it may lead to potential compatibility issues if these local packages are updated independently.To ensure this change is intentional and consistent with other packages, let's check the dependency specifications in other Cargo.toml files:
Please review the results of this check and consider:
- Is this change intentional and part of a broader strategy for managing local dependencies?
- Are there any potential risks in removing version constraints for these packages?
- Should we add a comment in the Cargo.toml file explaining this decision to help future maintainers?
Consider implementing a workspace-level version management strategy for local packages to ensure consistency across the project while maintaining flexibility for development.
✅ Verification successful
Dependency specifications are consistently managed across the codebase.
The removal of version constraints for
injective-cosmwasm,injective-math, andinjective-stdis consistent across all relevant Cargo.toml files. This approach aligns with the project's strategy to use the latest local versions, ensuring flexibility during development.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check dependency specifications in other Cargo.toml files # Expected result: Consistent use of path-only specifications for local dependencies # Search for injective-cosmwasm, injective-math, and injective-std in all Cargo.toml files rg --type toml 'injective-(cosmwasm|math|std).*path\s*=\s*"\.\./' -g 'Cargo.toml'Length of output: 1753
packages/injective-cosmwasm/Cargo.toml (1)
17-17: Clarify the rationale for removing the workspace attribute for injective-mathThe dependency specification for
injective-mathhas been changed from a workspace dependency to a direct path dependency. While this gives more precise control over the dependency version, it may lead to potential inconsistencies if other packages in the workspace use a different version ofinjective-math.Could you please clarify the reasoning behind this change? Additionally, it would be beneficial to verify that this modification doesn't introduce any version inconsistencies across the project.
To ensure consistency, let's check the usage of
injective-mathacross the project:This will help us understand if the change is consistent across the project and if
injective-mathis still defined in the workspace.✅ Verification successful
Verified removal of the workspace attribute for
injective-mathThe
workspace = trueattribute forinjective-mathhas been successfully removed frompackages/injective-cosmwasm/Cargo.toml. All references toinjective-mathacross the project are consistently managed via direct path dependencies, ensuring no version inconsistencies or build issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of injective-math across the project # Search for injective-math in all Cargo.toml files echo "Searching for injective-math in Cargo.toml files:" rg --type toml 'injective-math' -A 3 -B 1 # Check if injective-math is still defined in the workspace echo "\nChecking if injective-math is defined in the workspace root Cargo.toml:" rg --type toml '^\[workspace\]' -A 20 ./Cargo.toml | rg 'injective-math'Length of output: 5665
contracts/dummy/src/contract.rs (1)
30-32: Initialization ofCOUNTERandACTIVEis correctThe initialization of
COUNTERandACTIVEin theinstantiatefunction is properly implemented and follows best practices.
| @@ -22,7 +22,7 @@ ethereum-types = { version = "0.5.2" } | |||
| hex = { version = "0.4.3", features = [ "serde" ] } | |||
| injective-cosmwasm = { version = "0.3.0", path = "./packages/injective-cosmwasm" } | |||
| injective-math = { version = "0.3.0", path = "./packages/injective-math" } | |||
| injective-std = { version = "1.13.0", path = "./packages/injective-std" } | |||
| injective-std = { version = "1.13.2-auction", path = "./packages/injective-std" } | |||
There was a problem hiding this comment.
💡 Codebase verification
Issues Found:
-
Malformed Version in Cargo.toml: The
injective-stddependency inCargo.tomlhas an incorrect version specification:injective-std = { version = "1.13.2-auction 1.13.0", path = "./packages/injective-std" }This should be corrected to:
injective-std = { version = "1.13.2-auction", path = "./packages/injective-std" } -
Missing
injective-stdDependencies: The followingCargo.tomlfiles are missing theinjective-stddependency and should be updated accordingly:contracts/atomic-order-example/Cargo.tomlcontracts/dummy/Cargo.tomlcontracts/injective-cosmwasm-mock/Cargo.tomlcontracts/injective-cosmwasm-stargate-example/Cargo.tomlpackages/injective-cosmwasm/Cargo.tomlpackages/injective-math/Cargo.tomlpackages/injective-std/Cargo.tomlpackages/injective-std-derive/Cargo.tomlpackages/injective-testing/Cargo.toml
Please address these issues to ensure consistency and prevent potential build or runtime errors.
🔗 Analysis chain
LGTM: Version update for injective-std looks good.
The update of injective-std to version "1.13.2-auction" aligns with the PR objectives, suggesting improvements to auction-related functionality. This minor version bump should maintain compatibility while introducing new features or fixes.
To ensure consistency across the project, please run the following script:
This script will help identify any inconsistencies in the injective-std version across the project and highlight direct imports that might need attention due to the version change.
This script will iterate through all Cargo.toml files in the project, extract the version of injective-std, and report any instances where the version does not match 1.13.2-auction. This will help ensure that the dependency update is consistent across the entire codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the version of injective-std across the project
# Check for any occurrences of injective-std with different versions
rg --type toml 'injective-std.*version\s*=\s*"(?!1\.13\.2-auction)' -g 'Cargo.toml'
# If the above command returns no results, it means all occurrences are consistent
# with the new version. Otherwise, it will show the inconsistent entries.
echo "Checking for any direct imports of injective-std to ensure they are compatible with the new version:"
rg --type rust '^use injective_std' -g '!packages/injective-std/**'
Length of output: 1373
Script:
#!/bin/bash
# Description: Verify the version of injective-std across all Cargo.toml files
# Find all Cargo.toml files
fd -g 'Cargo.toml' | while read -r file; do
# Extract the version of injective-std in each Cargo.toml
version=$(grep 'injective-std' "$file" | grep -oP 'version\s*=\s*"\K[^"]+')
if [[ -z "$version" ]]; then
echo "injective-std not found in $file"
elif [[ "$version" != "1.13.2-auction" ]]; then
echo "Version mismatch in $file: found $version, expected 1.13.2-auction"
fi
done
Length of output: 2958
| // #[prost(string, tag = "2")] | ||
| // pub amount: ::prost::alloc::string::String, | ||
| // NOTE: THIS IS A FIX FOR RUST BINDINGS, PLEASE COPY IN FUTURE RELEASES | ||
| #[prost(message, optional, tag = "2")] | ||
| pub amount: ::core::option::Option<super::super::super::cosmos::base::v1beta1::Coin>, |
There was a problem hiding this comment.
Changing field type may cause backward compatibility issues
Modifying the amount field type from String to Option<Coin> in the Bid struct can lead to compatibility issues with existing data or services. Please ensure that this change is safe and compatible with all clients and services that use this struct.
| // #[prost(string, tag = "2")] | ||
| // pub amount: ::prost::alloc::string::String, | ||
| // NOTE: THIS IS A FIX FOR RUST BINDINGS, PLEASE COPY IN FUTURE RELEASES | ||
| #[prost(message, optional, tag = "2")] | ||
| pub amount: ::core::option::Option<super::super::super::cosmos::base::v1beta1::Coin>, |
There was a problem hiding this comment.
Changing field type may cause backward compatibility issues
The amount field in the LastAuctionResult struct has been changed from String to Option<Coin>, which might affect compatibility with existing data and services. Please verify that this change does not introduce any issues.
| // #[prost(string, tag = "2")] | ||
| // pub amount: ::prost::alloc::string::String, | ||
| // NOTE: THIS IS A FIX FOR RUST BINDINGS, PLEASE COPY IN FUTURE RELEASES | ||
| #[prost(message, optional, tag = "2")] | ||
| pub amount: ::core::option::Option<super::super::super::cosmos::base::v1beta1::Coin>, |
There was a problem hiding this comment.
Changing field type may cause backward compatibility issues
The amount field in the EventBid struct has been updated from String to Option<Coin>. Ensure that this change is compatible with all clients and services using this struct.
| // #[prost(string, tag = "2")] | ||
| // pub amount: ::prost::alloc::string::String, | ||
| // NOTE: THIS IS A FIX FOR RUST BINDINGS, PLEASE COPY IN FUTURE RELEASES | ||
| #[prost(message, optional, tag = "2")] | ||
| pub amount: ::core::option::Option<super::super::super::cosmos::base::v1beta1::Coin>, |
There was a problem hiding this comment.
Changing field type may cause backward compatibility issues
The amount field in the EventAuctionResult struct has been modified from String to Option<Coin>. Verify that this change does not introduce compatibility issues with existing data or services.
| @@ -5,7 +5,7 @@ license = "MIT OR Apache-2.0" | |||
| name = "injective-std" | |||
| readme = "README.md" | |||
| repository = "https://github.com/InjectiveLabs/cw-injective/tree/dev/packages/injective-std" | |||
| version = "1.13.0" | |||
| version = "1.13.2-auction" | |||
There was a problem hiding this comment.
because it's got some hacks for the auction module
Makes some fixes to the auction protos in injective-std
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Cargo.tomlfiles to streamline project configuration.