Feat/update to 1.16.4 1#260
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR pins workspace packages to explicit versions, upgrades CosmWasm/cw and Injective crates, replaces StdError::generic_err with StdError::msg across modules, removes Bank AllBalances mock support, migrates many amounts to Uint256, and updates mock/multi-test APIs and manifests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Querier as WasmMockQuerier
participant Bank as BankQueryHandler
participant Contract as ContractInfo
Caller->>Querier: handle_query(request)
alt Bank total supply path
Querier->>Bank: Query::Supply(denom)
Bank-->>Querier: SupplyResponse{ amount: Uint256 }
Querier-->>Caller: SystemResult::Ok(ContractResult::from(Ok::<Binary, StdError>(resp)))
else Bank balance path
Querier->>Bank: Query::Balance(address, denom)
Bank-->>Querier: BalanceResponse{ amount: Uint256 }
Querier-->>Caller: SystemResult::Ok(ContractResult::from(Ok::<Binary, StdError>(resp)))
else Contract info path
Querier->>Contract: ContractInfo(address)
Contract-->>Querier: ContractInfoResponse::new(..., None, None) 6 args
Querier-->>Caller: SystemResult::Ok(ContractResult::from(Ok::<Binary, StdError>(resp)))
else Unsupported
Querier-->>Caller: SystemResult::Err(StdError::msg(...))
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
build.sh (1)
1-1: Harden the build script and make the optimizer tag configurable.Add strict bash flags and allow overriding the optimizer version via OPTIMIZER_TAG.
Apply this diff:
#!/bin/bash +set -euo pipefail +IFS=$'\n\t' @@ -# Run the optimizer with the specified or default contract +# Run the optimizer with the specified or default contract +OPTIMIZER_TAG="${OPTIMIZER_TAG:-0.16.1}" docker run --rm -v "$(pwd)":/code \ -v "$HOME/.cargo/git":/usr/local/cargo/git \ --mount type=volume,source="$(basename "$(pwd)")_cache",target=/code/target \ --mount type=volume,source=registry_cache,target=/usr/local/cargo/registry \ - cosmwasm/optimizer${ARCH}:0.16.1 /code/contracts/$CONTRACT + cosmwasm/optimizer${ARCH}:${OPTIMIZER_TAG} /code/contracts/$CONTRACTAlso applies to: 34-40
.gitignore (1)
29-29: Generalize Cargo.lock ignore for all packages.If multiple crates under packages/ may produce lockfiles, prefer a glob.
Apply this diff:
-/packages/injective-cosmwasm/Cargo.lock +packages/*/Cargo.lockpackages/injective-cosmwasm/src/exchange/types.rs (4)
101-117: Validate hex content in MarketId::new (not just prefix/length).Currently only prefix/length are checked; add a hex decode check to fail early on invalid characters.
Apply this diff:
if market_id.len() != 66 { return Err(StdError::msg("Invalid length: market_id must be exactly 66 characters")); } + if hex::decode(&market_id[2..]).is_err() { + return Err(StdError::msg("Invalid hex: market_id must be 0x followed by 64 hex chars")); + }
186-203: Validate hex content in SubaccountId::new.Mirror the MarketId check for subaccount IDs.
Apply this diff:
if subaccount_id.len() != 66 { return Err(StdError::msg("Invalid length: subaccount_id must be exactly 66 characters")); } + if hex::decode(&subaccount_id[2..]).is_err() { + return Err(StdError::msg("Invalid hex: subaccount_id must be 0x followed by 64 hex chars")); + }
283-299: Simplify ShortSubaccountId::validate and avoid string re-parsing.Use numeric bounds directly; this reduces allocations and avoids parse churn.
Apply this diff:
- pub fn validate(&self) -> StdResult<Self> { - let as_decimal = match u32::from_str_radix(self.as_str(), 16) { - Ok(dec) => Ok(dec), - Err(_) => Err(StdError::msg(format!( - "Invalid value: ShortSubaccountId was not a hexadecimal number: {}", - &self.0 - ))), - }; - - match as_decimal?.to_string().parse::<u16>() { - Ok(value) if value <= MAX_SHORT_SUBACCOUNT_NONCE => Ok(self.clone()), - _ => Err(StdError::msg(format!( - "Invalid value: ShortSubaccountId must be a number between 0-999, but {} was received", - &self.0 - ))), - } - } + pub fn validate(&self) -> StdResult<Self> { + let value = u32::from_str_radix(self.as_str(), 16).map_err(|_| { + StdError::msg(format!( + "Invalid value: ShortSubaccountId was not a hexadecimal number: {}", + &self.0 + )) + })?; + if value <= MAX_SHORT_SUBACCOUNT_NONCE as u32 { + Ok(self.clone()) + } else { + Err(StdError::msg(format!( + "Invalid value: ShortSubaccountId must be a number between 0-999, but {} was received", + &self.0 + ))) + } + }
495-518: Make tests resilient to StdError formatting.Asserting exact to_string() is brittle; assert on substrings.
Apply this diff example (replicate similarly for others):
-assert_eq!( - wrong_prefix_err.to_string(), - "kind: Other, error: Invalid prefix: market_id must start with 0x" -); +assert!(wrong_prefix_err.to_string().contains("Invalid prefix: market_id must start with 0x"));packages/injective-testing/src/multi_test/chain_mock.rs (2)
341-345: Simplify copy_binary.Use Binary::from to avoid extra allocation/copy code.
Apply this diff:
-fn copy_binary(binary: &Binary) -> Binary { - let mut c: Vec<u8> = vec![0; binary.to_vec().len()]; - c.clone_from_slice(binary); - Binary::new(c) -} +fn copy_binary(binary: &Binary) -> Binary { + Binary::from(binary.as_slice()) +}
66-82: Optional: use StdResult in response containers.If you’ve standardized on StdError, consider switching ExecuteResponse/QueryResponse to StdResult to avoid stringifying errors.
Also applies to: 97-115
packages/injective-testing/src/multi_test/address_generator.rs (1)
94-94: Use ADDRESS_LENGTH constant instead of magic number.Minor readability improvement.
Apply this diff:
- let address_short = to_hex_string(&keccak[ADDRESS_BYTE_INDEX..], 40); // get rid of the constant 0x04 byte + let address_short = to_hex_string(&keccak[ADDRESS_BYTE_INDEX..], ADDRESS_LENGTH); // get rid of the constant 0x04 bytepackages/injective-math/Cargo.toml (1)
14-17: Consider using workspace-level dependency management for consistency.Moving from workspace dependencies to explicit versions in each package can lead to version inconsistencies across crates. This makes dependency updates more error-prone and harder to maintain. Consider keeping critical dependencies like
cosmwasm-std,schemars, andserdeat the workspace level.packages/injective-cosmwasm/Cargo.toml (1)
2-2: Fix malformed author entries.The author entries are missing angle brackets around email addresses. They should follow the format
"Name <email>".Apply this diff to fix the author format:
-authors = [ "Albert Chon albert@injectivelabs.org", "F Grabner friedrich@injectivelabs.org", "Markus Waas markus@injectivelabs.org", "Jose Luis joseluis@injectivelabs.org" ] +authors = [ "Albert Chon <albert@injectivelabs.org>", "F Grabner <friedrich@injectivelabs.org>", "Markus Waas <markus@injectivelabs.org>", "Jose Luis <joseluis@injectivelabs.org>" ]packages/injective-math/src/fp_decimal/mod.rs (1)
104-104: Error message formatting could be clearer.While the migration from
StdError::generic_errtoStdError::msgis correct, the error message format could be improved for clarity.Apply this diff to improve the error message:
- return Err(StdError::msg(format!("Value {} must be >= {}", fp_decimal.num, 0))); + return Err(StdError::msg(format!("Cannot convert negative FPDecimal to Decimal256: value is {}", fp_decimal)));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.gitignore(1 hunks)Cargo.toml(2 hunks)build.sh(1 hunks)packages/injective-cosmwasm/Cargo.toml(1 hunks)packages/injective-cosmwasm/src/exchange/types.rs(5 hunks)packages/injective-cosmwasm/src/exchange_mock_querier.rs(8 hunks)packages/injective-cosmwasm/src/msg.rs(1 hunks)packages/injective-cosmwasm/src/test_helpers.rs(2 hunks)packages/injective-math/Cargo.toml(1 hunks)packages/injective-math/README.md(1 hunks)packages/injective-math/src/fp_decimal/from_str.rs(2 hunks)packages/injective-math/src/fp_decimal/mod.rs(1 hunks)packages/injective-math/src/utils.rs(4 hunks)packages/injective-testing/Cargo.toml(1 hunks)packages/injective-testing/src/multi_test/address_generator.rs(3 hunks)packages/injective-testing/src/multi_test/chain_mock.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/injective-cosmwasm/src/exchange_mock_querier.rs (1)
packages/injective-math/src/fp_decimal/mod.rs (9)
from(17-22)from(26-37)from(41-53)from(57-59)from(63-66)from(70-72)from(76-78)from(82-84)from(89-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Suite
- GitHub Check: Lints
- GitHub Check: Test Suite
- GitHub Check: Lints
🔇 Additional comments (28)
build.sh (1)
7-7: Comment update looks correct.Referencing pCargo.toml from the workspace root aligns with the repo changes.
packages/injective-cosmwasm/src/test_helpers.rs (2)
9-9: Import cleanup LGTM.Dropping TransactionInfo import is consistent with setting transaction: None.
54-55: Confirm no tests rely on transaction index.Setting transaction to None is fine; ensure any tests/assertions that previously read Env.transaction.* are updated or removed.
packages/injective-math/README.md (1)
65-65: Docs update LGTM.Reference to pCargo.toml matches the workspace manifest change.
packages/injective-cosmwasm/src/msg.rs (1)
236-238: Switch to StdError::msg is correct.Change aligns with the crate-wide error style.
Please confirm any tests expecting StdError::generic_err were updated to assert on message or use is_err().
packages/injective-cosmwasm/src/exchange/types.rs (1)
109-114: Consistent migration to StdError::msg.The replacements are consistent and maintain behavior.
Also applies to: 193-199, 286-299, 433-434
packages/injective-testing/src/multi_test/chain_mock.rs (5)
2-5: Imports/storage/error-type updates LGTM.Using MockStorage and StdError matches cw-multi-test and cosmwasm-std updates.
22-22: Type alias update LGTM.MockedInjectiveApp now correctly uses MockStorage.
197-198: execute now returning StdError is appropriate.Mapping anyhow to StdError via msg is acceptable in tests.
237-245: query now returning StdError is appropriate.Consistent with execute.
285-287: sudo behavior clarified.Returning StdError for unexpected sudo is fine for tests.
packages/injective-testing/src/multi_test/address_generator.rs (1)
16-23: Signature migrations to StdError LGTM.Matches the broader move away from anyhow in public surfaces.
Also applies to: 35-38, 54-61
packages/injective-math/Cargo.toml (2)
9-9: Verify the pre-release version follows your versioning strategy.The version
0.3.5-1uses a pre-release identifier. Ensure this aligns with your semantic versioning strategy and consider whether this should be0.3.5-rc1,0.3.5-beta1, or simply0.3.5depending on your release conventions.
19-28: Production release profile looks good.The release profile settings are appropriate for production builds with comprehensive optimizations (
lto = true,opt-level = 3) and safety checks (overflow-checks = true). Good balance between performance and safety.packages/injective-cosmwasm/Cargo.toml (1)
12-21: Version alignment looks consistent.Good that
injective-mathdependency version (0.3.5-1) matches the version in its ownCargo.toml. The CosmWasm upgrade to 3.0.2 with appropriate feature flags is properly configured.packages/injective-cosmwasm/src/exchange_mock_querier.rs (4)
248-254: Good adoption of Uint256 for bank supply.The change from
Uint128toUint256for the bank supply amount aligns well with CosmWasm 3.0's support for larger token amounts and is future-proof for high-supply tokens.
1189-1199: Supply handler correctly uses Uint256.The
create_bank_supply_handlerfunction properly accepts and usesUint256for the supply amount, maintaining consistency with the default handler.
1311-1311: ContractInfoResponse constructor updated — verify docs.
Single call updated to 6 args in packages/injective-cosmwasm/src/exchange_mock_querier.rs (ContractInfoResponse::new(self.code_id, self.creator.to_owned(), None, false, None, None)). Confirm this ordering matches the new constructor and add/update documentation for the new parameter.
1280-1280: No change required: the nestedOkis intentional. The innerOk::<Binary, StdError>(resp)constructs theStdResult<Binary>needed byContractResult::from, and the outerSystemResult::Okthen wraps it into theQuerierResult. This matches the pattern elsewhere (whereto_json_binaryreturns aStdResult<Binary>).packages/injective-math/src/fp_decimal/from_str.rs (1)
19-19: Consistent error handling migration.The migration from
StdError::generic_errtoStdError::msgis consistently applied throughout the file. All error messages are preserved, maintaining backward compatibility for error handling logic.Also applies to: 29-30, 33-33, 40-40
packages/injective-math/src/utils.rs (3)
32-32: Error constructor update aligns with CosmWasm 3.0.2 best practices.The change from
StdError::generic_errtoStdError::msgfollows the current CosmWasm 3.0.2 error handling patterns. The functionality remains identical - both constructors create aGenericErrvariant with the provided message, butStdError::msgis the modern, idiomatic approach.
41-41: Consistent error constructor modernization.The systematic replacement of
StdError::generic_errwithStdError::msgacross all validation error paths maintains consistency with the updated CosmWasm dependencies. The error messages and validation logic remain unchanged.Also applies to: 46-46, 55-55, 60-60
69-69: Error wrapper function updated correctly.The
band_error_to_humanfunction now usesStdError::msgwhich is consistent with the other error constructor updates in this file. The wrapping behavior and message formatting are preserved.packages/injective-testing/Cargo.toml (2)
2-2: Version bump and author addition look good.The version increment to
1.1.13-1and addition of Jose Luis Bernal Castillo as an author are appropriate changes for this dependency update.Also applies to: 8-8
11-23: Dependency migration from workspace to explicit versions completed correctly.The transition from workspace-shared dependencies to explicit version pins is well-executed:
- CosmWasm ecosystem: Updated to 3.0.2 with proper feature flags (
cosmwasm_1_2,cosmwasm_1_3,cosmwasm_1_4,cosmwasm_2_0,iterator,stargate)- Injective dependencies: Updated to consistent versions (
1.16.4-1for test-tube and std,0.3.5-1for cosmwasm and math)- Other dependencies: Appropriately pinned with necessary features
This aligns with the broader workspace restructuring mentioned in the PR objectives.
Cargo.toml (3)
2-2: Workspace members reduced appropriately.Removing
"packages/*"from workspace members aligns with the AI summary indicating a shift to explicit versioned dependencies rather than path-based workspace dependencies. This change supports the migration strategy where packages now use explicit version pins.
14-16: CosmWasm and Injective dependency updates are well-coordinated.Key updates include:
- CosmWasm std: 2.1.0 → 3.0.2 with comprehensive feature enablement
- CW ecosystem: cw-multi-test (2.1.0 → 3.0.0), cw-storage-plus (2.0.0 → 3.0.1), cw2 (2.0.0 → 3.0.0)
- Injective packages: Transitioned from path-based to versioned dependencies (0.3.5-1 and 1.16.4-1 series)
- Prost: Minor version bump (0.13.4 → 0.13.5)
These version updates are consistent with CosmWasm 2.0+ patterns where contracts can maintain compatibility across wasmvm versions.
Also applies to: 20-24, 26-26
52-55: Confirm intent: top-level patch.crates-io is commented out but per-crate local path overrides are activeRoot Cargo.toml: injective-* patch entries are commented out.
Active local path overrides found:
- contracts/injective-cosmwasm-stargate-example/Cargo.toml
- injective-cosmwasm = { path = "../../packages/injective-cosmwasm" }
- injective-math = { path = "../../packages/injective-math" }
- contracts/injective-cosmwasm-mock/Cargo.toml
- injective-cosmwasm = { path = "../../packages/injective-cosmwasm" }
- injective-math = { path = "../../packages/injective-math" }
- [dev-dependencies] injective-testing = { path = "../../packages/injective-testing" }
- contracts/dummy/Cargo.toml
- injective-cosmwasm = { path = "../../packages/injective-cosmwasm" }
- contracts/atomic-order-example/Cargo.toml
- injective-cosmwasm = { path = "../../packages/injective-cosmwasm" }
- injective-math = { path = "../../packages/injective-math" }
Found .cargo/config.toml at:
- ./.cargo/config.toml
- ./packages/injective-math/.cargo/config.toml
- ./contracts/atomic-order-example/.cargo/config.toml
- ./contracts/injective-cosmwasm-mock/.cargo/config.toml
Decide whether to re-enable a global patch in root Cargo.toml (and remove per-crate path overrides) or keep the current per-crate local path setup; confirm intent for me to resolve this review.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
3 participants
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
Documentation