F/bump prost #50
F/bump prost #50
Conversation
WalkthroughThe pull request introduces several 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: 2
🧹 Outside diff range and nitpick comments (3)
packages/injective-test-tube/Cargo.toml (1)
20-20: Document the addition of prost-derive featureThe prost dependency has been updated from version 0.12.3 to 0.13.3, which aligns with the PR objectives. Additionally, the prost-derive feature has been added.
Could you please add a comment explaining why the prost-derive feature was added? This will help future maintainers understand the reasoning behind this change.
packages/injective-test-tube/src/module/exchange.rs (2)
178-180: LGTM! Consider using constants for decimal places.The addition of
base_decimalsandquote_decimalsfields is a good improvement, aligning with the PR objective. The values (18 for INJ and 6 for USDT) are correct for these currencies.Consider defining these values as constants at the module level for better maintainability and reusability. For example:
const INJ_DECIMALS: u32 = 18; const USDT_DECIMALS: u32 = 6;Then use these constants in the test:
base_decimals: INJ_DECIMALS, quote_decimals: USDT_DECIMALS,
216-219: LGTM! Consider using constants for consistency.The addition of
base_decimalsandquote_decimalsfields in the expectedSpotMarketresponse is correct and consistent with the changes made to the struct.For consistency with the earlier suggestion, consider using the same constants here:
base_denom: "inj".to_owned(), base_decimals: INJ_DECIMALS, quote_denom: "usdt".to_owned(), quote_decimals: USDT_DECIMALS,This would ensure that if the decimal places need to be changed in the future, they only need to be updated in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/injective-test-tube/CHANGELOG.md (1 hunks)
- packages/injective-test-tube/Cargo.toml (1 hunks)
- packages/injective-test-tube/src/module/auction.rs (1 hunks)
- packages/injective-test-tube/src/module/exchange.rs (3 hunks)
- packages/test-tube/Cargo.toml (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
packages/test-tube/Cargo.toml (1)
7-7: Version bump looks good.The package version has been updated from 2.0.1 to 2.0.2, which is consistent with updating dependencies. This minor version bump follows semantic versioning principles.
packages/injective-test-tube/Cargo.toml (3)
7-7: LGTM: Package version updateThe package version has been incremented from "1.13.2-auction" to "1.13.2-auction.1". This minor version bump aligns with semantic versioning principles and the PR objectives.
23-23: LGTM: test-tube-inj dependency updateThe test-tube-inj dependency has been updated from version 2.0.1 to 2.0.2. This patch version bump likely includes bug fixes and aligns with the PR objectives.
14-14: Verify compatibility with cosmrs 0.20.0The cosmrs dependency has been updated from version 0.15.0 to 0.20.0. This major version bump aligns with the PR objectives. However, it's important to ensure that this update doesn't introduce any breaking changes to the project.
Please run the following script to check for any compatibility issues:
✅ Verification successful
Compatibility with cosmrs 0.20.0 verified
The cosmrs dependency has been successfully updated to version 0.20.0 without introducing any compiler errors or warnings. All usages of cosmrs in the codebase are consistent with the updated version, ensuring compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential breaking changes in cosmrs usage # Test: Search for cosmrs usage in the codebase rg --type rust 'use cosmrs::|cosmrs::' -A 5 # Test: Check if there are any compiler errors or warnings related to cosmrs cargo check 2>&1 | rg 'cosmrs'Length of output: 12007
packages/injective-test-tube/src/module/auction.rs (1)
67-68: Verify the default value forinj_basket_max_capThe test case has been correctly updated to include the new
inj_basket_max_capfield in theParamsstructure. This aligns with the PR objective of updating dependencies. However, it's initialized with an empty string.Please verify if an empty string is the correct default value for
inj_basket_max_cap. If it's just a placeholder, consider using a more meaningful default value or adding a comment explaining why it's empty.To help verify this, you can run the following script to check for any other occurrences or definitions of
inj_basket_max_cap:✅ Verification successful
Default value for
inj_basket_max_capis correctly setThe
inj_basket_max_capfield is only used in theauction.rsfile and is correctly initialized with an empty string in the test case. There are no other occurrences or struct definitions that require a different default value.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other occurrences or definitions of inj_basket_max_cap # Search for inj_basket_max_cap in all Rust files rg --type rust 'inj_basket_max_cap' # Search for any struct definitions containing inj_basket_max_cap ast-grep --lang rust --pattern 'struct $_ { $$$ inj_basket_max_cap: $_, $$$ }'Length of output: 225
packages/injective-test-tube/src/module/exchange.rs (2)
195-197: LGTM! Consistent use of new fields in error case.The duplication of the
base_decimalsandquote_decimalsfields in this error case test is correct and consistent with the previous usage.
Line range hint
1-524: Summary: Successful implementation of decimal places for spot marketsThe changes in this file successfully implement the addition of
base_decimalsandquote_decimalsfields to the spot market functionality. These modifications align well with the PR objective of updating the prost dependency and enhance theExchangemodule's capabilities.Key points:
- New fields are added consistently to
MsgInstantSpotMarketLaunchandSpotMarketstructs.- Test cases are updated to include and verify these new fields.
- The implementation uses appropriate values for INJ (18 decimals) and USDT (6 decimals).
The changes improve the accuracy of spot market representations and should provide better support for various token decimal places in the future.
packages/test-tube/Cargo.toml
Outdated
|
|
||
| # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | ||
|
|
||
| [dependencies] | ||
| base64 = "0.21.5" | ||
| cosmrs = { version = "0.20.0", features = [ "cosmwasm", "rpc" ] } | ||
| cosmwasm-std = { version = "2.1.0", features = [ "abort", "cosmwasm_1_2", "cosmwasm_1_3", "cosmwasm_1_4", "cosmwasm_2_0", "iterator", "stargate" ] } | ||
| prost = { version = "0.13.3", default-features = false, features = [ "prost-derive" ] } | ||
| prost = { version = "0.13.3", features = [ "prost-derive" ] } |
There was a problem hiding this comment.
💡 Codebase verification
Missing cosmrs Dependency Update
The cosmrs dependency remains at version 0.20.0 in both packages/test-tube/Cargo.toml and packages/injective-test-tube/Cargo.toml. This update was intended as part of the PR objectives but appears to be missing. Please update the cosmrs dependency accordingly to align with the PR goals.
🔗 Analysis chain
Prost dependency updated, but cosmrs update is missing.
The prost dependency has been modified by removing default-features = false. This change will include the default features of prost, which may introduce new functionality or behavior. Please ensure this is intentional and doesn't introduce any conflicts or unexpected behavior in the project.
However, I noticed that the cosmrs dependency version remains unchanged at 0.20.0. The PR objectives mentioned updating both prost and cosmrs dependencies. Could you please clarify if the cosmrs update is still planned or if it has been intentionally omitted?
To confirm the current state of dependencies, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check current versions of prost and cosmrs in all Cargo.toml files
rg --type toml 'prost.*version|cosmrs.*version' .
Length of output: 493
| ## 1.13.2-auction.1 - 2024-16-10 | ||
|
|
||
| ### Changed | ||
|
|
||
| - Bumped `[email protected]` |
There was a problem hiding this comment.
Fix the date format in the new changelog entry.
The new changelog entry looks good overall, following the established format and providing a clear description of the change. However, there's a small issue with the date format.
Please update the date format to follow the YYYY-MM-DD pattern. The current date "2024-16-10" is invalid as there is no 16th month. It should likely be "2024-10-16" (assuming October 16, 2024).
Apply this change:
-## 1.13.2-auction.1 - 2024-16-10
+## 1.13.2-auction.1 - 2024-10-16The rest of the changelog entry is well-formatted and clearly describes the version bump for the prost dependency.
📝 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.
| ## 1.13.2-auction.1 - 2024-16-10 | |
| ### Changed | |
| - Bumped `[email protected]` | |
| ## 1.13.2-auction.1 - 2024-10-16 | |
| ### Changed | |
| - Bumped `[email protected]` |
| @@ -175,7 +175,9 @@ mod tests { | |||
| sender: signer.address(), | |||
| ticker: "INJ/USDT".to_owned(), | |||
| base_denom: "inj".to_owned(), | |||
| base_decimals: 18u32, | |||
There was a problem hiding this comment.
There is a variable with decimals values why not use it ?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/injective-test-tube/libinjectivetesttube/go.mod (1)
86-100: Consider documenting telemetry updates.Multiple DataDog telemetry packages have been updated. Consider adding these updates to the changelog for better tracking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/injective-test-tube/libinjectivetesttube/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.github/workflows/unit_test.yml(2 hunks)packages/injective-test-tube/CHANGELOG.md(1 hunks)packages/injective-test-tube/Cargo.toml(1 hunks)packages/injective-test-tube/libinjectivetesttube/go.mod(10 hunks)packages/injective-test-tube/src/module/wasm.rs(1 hunks)packages/test-tube/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/test-tube/Cargo.toml
🧰 Additional context used
🪛 LanguageTool
packages/injective-test-tube/CHANGELOG.md
[duplication] ~12-~12: Possible typo: you repeated a word.
Context: ... ## 1.13.3 - 2024-16-10 ### Changed - Bumped [email protected] - Bumped [email protected] ## 1.13.2-auct...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (9)
packages/injective-test-tube/src/module/wasm.rs (2)
132-132: Removal of redundant?Sizedbound is correct.The removal of
?Sizedfrom theRestype constraint is appropriate sinceDeserializeOwnedalready impliesSized. This change simplifies the type bounds without affecting functionality.Let's verify that this change doesn't break existing query result types:
✅ Verification successful
Verified: Removal of
?Sizedbound is safeAll existing query result types in the codebase are concrete, sized types that implement
DeserializeOwned. No instances found where?Sizedwas actually needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for query method calls to verify return types rg -A 2 "\.query::<.*>"Length of output: 2917
Line range hint
132-145: Verify serialization compatibility with updated prost.Since this PR includes a prost version bump, please ensure that the JSON serialization in the query method remains compatible with the updated protobuf message formats, particularly for the
QuerySmartContractStateRequestandQuerySmartContractStateResponsetypes.Let's check for any protobuf message changes that might affect serialization:
.github/workflows/unit_test.yml (2)
20-22: LGTM! Consistent toolchain configuration.The Rust toolchain version has been consistently updated to 1.81.0 across both the test and lints jobs, maintaining uniformity in the CI environment.
Also applies to: 41-43
20-20: Verify Rust toolchain compatibility with updated dependencies.The Rust toolchain has been consistently updated to 1.81.0 across both jobs. Let's verify compatibility with the updated dependencies, particularly prost 0.13.4.
Also applies to: 41-41
✅ Verification successful
Rust toolchain 1.81.0 is compatible with updated dependencies
The toolchain update to 1.81.0 is compatible with prost 0.13.4, as evidenced by prost's development using Clippy 1.82 during the 0.13.4 release cycle. The consistent update across both jobs maintains proper workflow configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check minimum supported Rust version (MSRV) for prost 0.13.4 # Check Cargo.toml files for prost version specifications echo "Checking prost version in Cargo.toml files:" rg "prost.*=.*0.13.4" -A 2 -B 2 # Fetch prost's Cargo.toml to check MSRV echo "Fetching prost 0.13.4 metadata:" curl -s "https://raw.githubusercontent.com/tokio-rs/prost/v0.13.4/prost/Cargo.toml" | rg "rust-version"Length of output: 1179
Script:
#!/bin/bash # Check workspace and package-specific Rust version requirements echo "Checking workspace Cargo.toml for rust-version:" rg "rust-version.*=.*" Cargo.toml echo -e "\nChecking all Cargo.toml files for rust-version specifications:" fd Cargo.toml --exec rg -l "rust-version.*=.*" {} \; echo -e "\nShowing content around rust-version in all Cargo.toml files:" fd Cargo.toml --exec rg -A 2 -B 2 "rust-version.*=.*" {} \;Length of output: 541
Script:
#!/bin/bash # Check prost's release notes and documentation for MSRV echo "Checking prost's README for MSRV information:" curl -s "https://raw.githubusercontent.com/tokio-rs/prost/v0.13.4/README.md" | rg -i "rust|version" -A 2 -B 2 echo -e "\nChecking prost's release notes for version 0.13.4:" curl -s "https://api.github.com/repos/tokio-rs/prost/releases/tags/v0.13.4" | jq -r '.body' echo -e "\nChecking for any explicit MSRV files:" curl -s "https://api.github.com/repos/tokio-rs/prost/contents?ref=v0.13.4" | jq -r '.[].name' | grep -i "rust\|version"Length of output: 7767
packages/injective-test-tube/Cargo.toml (3)
7-7: Version bump aligns with semantic versioning.The version increment from 1.13.2-auction to 1.13.3 follows semantic versioning principles, correctly reflecting minor updates.
19-20: Prost update includes new feature.The prost update adds the
prost-derivefeature, which is good for derive macro support.
14-14: Verify compatibility with cosmrs and cosmwasm-std updates.The updates include significant version jumps:
- cosmrs: 0.15.0 → 0.20.0
- cosmwasm-std: 2.1.0 → 2.2 with reduced features
Run this script to check for potential breaking changes:
Also applies to: 16-16
✅ Verification successful
Dependencies update compatibility verified
The codebase is compatible with both updates:
- cosmrs 0.20.0: Uses only stable APIs (proto, crypto, tx) with maintained features
- cosmwasm-std 2.2: Uses core types with proper feature flags (cosmwasm_2_0, stargate), no deprecated features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for cosmrs and cosmwasm-std usage that might be affected by the updates # Check cosmrs usage echo "Checking cosmrs usage patterns..." rg -l 'use cosmrs::|::cosmrs::|cosmrs::' # Check cosmwasm_std features usage echo "Checking removed cosmwasm_std features usage..." rg -l 'abort|cosmwasm_1_2|cosmwasm_1_3|cosmwasm_1_4|iterator'Length of output: 668
Script:
#!/bin/bash # Check specific cosmrs usage patterns and cosmwasm_std features echo "=== Checking cosmrs specific imports and usage ===" rg -A 2 'use cosmrs::|::cosmrs::' echo -e "\n=== Checking cosmwasm_std in Cargo.toml files ===" fd Cargo.toml --exec grep -l "cosmwasm-std" {} \; --exec cat {} echo -e "\n=== Checking direct cosmwasm_std usage ===" rg 'use cosmwasm_std::|::cosmwasm_std::'Length of output: 5786
packages/injective-test-tube/CHANGELOG.md (1)
8-13:⚠️ Potential issueFix invalid date format in changelog.
The date format "2024-16-10" is invalid as there is no 16th month. It should be "2024-10-16" (October 16, 2024).
Apply this diff:
-## 1.13.3 - 2024-16-10 +## 1.13.3 - 2024-10-16Likely invalid or redundant comment.
🧰 Tools
🪛 LanguageTool
[duplication] ~12-~12: Possible typo: you repeated a word.
Context: ... ## 1.13.3 - 2024-16-10 ### Changed - Bumped[email protected]- Bumped[email protected]## 1.13.2-auct...(ENGLISH_WORD_REPEAT_RULE)
packages/injective-test-tube/libinjectivetesttube/go.mod (1)
279-279: Version alignment looks good.The injective-core version bump to v1.13.3 is consistent with the package version in Cargo.toml.
In this PR both prost and cosmrs are bumped.
Summary by CodeRabbit
New Features
Bug Fixes
Chores