F/v1.16.4 1#61
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRemoved a workspace dependency entry; bumped multiple Rust crates; repointed the Go module graph to InjectiveLabs forks; added an Exchange denom-decimals query and auction Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test Harness
participant Exchange as Exchange Module
participant Node as RPC/ABCI
Note over Test,Exchange: denom-decimals query flow (new)
Test->>Exchange: query_denom_decimals(Request)
Exchange->>Node: RPC /injective.exchange.v1beta1.Query/DenomDecimals
Node-->>Exchange: DenomDecimalsResponse
Exchange-->>Test: DenomDecimalsResponse
sequenceDiagram
autonumber
participant Test as Test Harness
participant Helpers as module/helpers.rs
participant Node as RPC/ABCI
participant Gov as GovernanceModule
Note over Test,Helpers: add_exchange_admin migrated to v2
Test->>Helpers: build v2 QueryExchangeParamsRequest
Helpers->>Node: call /injective.exchange.v2.Query/QueryExchangeParams
Node-->>Helpers: QueryExchangeParamsResponse (v2)
Helpers->>Helpers: modify params & encode v2 MsgUpdateParams Any
Helpers->>Gov: submit MsgSubmitProposal containing encoded Any
Gov-->>Test: proposal submitted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/test-tube/src/runner/app.rs (1)
418-422: Avoid unwrap() when parsing fee/denom — use fallible conversions
- packages/test-tube/src/runner/app.rs:339-341 — remove
denom.parse().unwrap()andgas_price.amount.to_string().parse::<u128>().unwrap()(these can panic). Convert numerics directly or useTryFrom/TryIntoorFromStrwith proper error propagation instead ofunwrap().packages/injective-test-tube/libinjectivetesttube/go.mod (2)
3-3: Invalidgodirective (patch versions not allowed).
go.modonly accepts major.minor (e.g.,go 1.23). Usinggo 1.23.9will failgo modcommands.Apply:
-go 1.23.9 +go 1.23
1-328: Fix go.mod: use major.minor go directive, correct InjectiveLabs casing, and fix invalid pseudo-version
- Change
go 1.23.9→go 1.23. (packages/injective-test-tube/libinjectivetesttube/go.mod:3)- Fix replace-path casing:
github.com/injectivelabs/cometbft/api→github.com/InjectiveLabs/cometbft/api. (line 306)- Correct the pseudo-version for
github.com/syndtr/goleveldb—v1.0.1-0.20220721030215-126854af5e6v0dcontains non-hex chars; use the canonical tag or a valid pseudo-version and rungo mod tidyto regenerate. (line 275)
🧹 Nitpick comments (7)
packages/injective-test-tube/src/module/wasm.rs (1)
68-75: Drop redundant format! around amount; use to_string() directly
format!("{}", c.amount.to_string())allocates unnecessarily. Simplify toc.amount.to_string()in both funds mappers.- amount: format!("{}", c.amount.to_string()), + amount: c.amount.to_string(),Also applies to: 96-101
packages/injective-test-tube/src/module/exchange.rs (2)
274-274: Rustfmt: remove stray blank line in testsThis trips the CI fmt check. Run cargo fmt or drop the extra newline.
- +
930-932: Trailing blank lines at EOFFormatting differences detected by CI. Please run
cargo fmt --all.packages/injective-test-tube/src/runner/app.rs (2)
491-495: fmt failure in CI on this blockCI reports rustfmt differences near here. Run
cargo fmt --allto normalize formatting.
493-495: Uint256 arithmetic assertion looks good; minor type-safety nit
amount.amountis Uint128; subtraction with Uint256 relies on implicit conversion. ConsiderUint256::from(amount.amount)for clarity.- assert_eq!(Uint256::new(bob_balance), Uint256::new(initial_balance) - amount.amount); + assert_eq!( + Uint256::new(bob_balance), + Uint256::new(initial_balance) - Uint256::from(amount.amount) + );packages/injective-test-tube/libinjectivetesttube/go.mod (2)
300-301: Injective Core pinned to v1.16.4 via replace—consider pinning require too.You keep a zero‑timestamp placeholder in require and rely on replace for the real version. Optional: require the actual semver for clearer provenance, or add a comment noting why the placeholder is intentional.
Example:
- github.com/InjectiveLabs/injective-core v0.0.0-00010101000000-000000000000 + // kept in sync with replace -> InjectiveFoundation/injective-core v1.16.4 + github.com/InjectiveLabs/injective-core v1.16.4
322-323: Zero‑timestamp placeholder for block‑sdk/v2.Same note as injective‑core: optional to pin the require to the actual version for readability, since you already replace to
v2.5.1-evm-comet1-inj.
📜 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 (10)
Cargo.toml(0 hunks)packages/injective-test-tube/Cargo.toml(1 hunks)packages/injective-test-tube/libinjectivetesttube/go.mod(8 hunks)packages/injective-test-tube/src/module/auction.rs(1 hunks)packages/injective-test-tube/src/module/exchange.rs(2 hunks)packages/injective-test-tube/src/module/wasm.rs(2 hunks)packages/injective-test-tube/src/runner/app.rs(2 hunks)packages/test-tube/Cargo.toml(1 hunks)packages/test-tube/src/runner/app.rs(1 hunks)packages/test-tube/src/utils.rs(1 hunks)
💤 Files with no reviewable changes (1)
- Cargo.toml
🧰 Additional context used
🪛 GitHub Actions: Unit tests + lints
packages/injective-test-tube/src/module/exchange.rs
[error] 271-271: Rust fmt check failed (cargo fmt --all -- --check): formatting differences detected. Run 'cargo fmt --all' to fix.
[error] 927-927: Rust fmt check failed (cargo fmt --all -- --check): formatting differences detected. Run 'cargo fmt --all' to fix.
packages/injective-test-tube/src/runner/app.rs
[error] 491-491: Rust fmt check failed (cargo fmt --all -- --check): formatting differences detected. Run 'cargo fmt --all' to fix.
packages/test-tube/src/runner/app.rs
[error] 338-338: Rust fmt check failed (cargo fmt --all -- --check): formatting differences detected. Run 'cargo fmt --all' to fix.
packages/injective-test-tube/Cargo.toml
[warning] 1-1: Patch for non-root package will be ignored. Specify patch at the workspace root.
⏰ 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). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Test Suite
🔇 Additional comments (8)
packages/test-tube/src/utils.rs (1)
18-21: LGTM: amount now serialized via to_string()Consistent with Uint-based amounts and avoids lossy casts.
packages/injective-test-tube/src/module/auction.rs (1)
63-67: New Params field bidders_whitelist verified in testLooks correct with default
vec![]. Ensure this matches the Injective chain proto you target so queries don’t decode-fail on older nodes.packages/injective-test-tube/src/runner/app.rs (1)
155-155: LGTM: import Uint256 for big-int assertionsImport aligns tests with Uint256 arithmetic.
packages/test-tube/Cargo.toml (1)
7-15: Deps bump to 2.0.9-1 and cosmwasm-std 3.0.2 looks fineRemoval of the deprecated "abort" feature is aligned with cw-std 3.x. Ensure all workspace crates compile against 3.0.2 (CI seems fine).
packages/injective-test-tube/Cargo.toml (1)
14-21: Version bumps LGTM; confirm cw1-whitelist compat with cw-std 3.xCrate versions align with the rest of the PR. Double-check
cw1-whitelist = "1.1.2"is compatible for the imported msg types with cosmwasm-std 3.0.2 in this workspace.Also applies to: 26-27
packages/injective-test-tube/libinjectivetesttube/go.mod (3)
55-66: Pre‑release cosmossdk.io client/v2.You’re requiring
cosmossdk.io/client/v2 v2.0.0-beta.3while replacing to an InjectiveLabs fork. If the beta’s APIs shifted, indirects may drift. Confirm compatibility or pin a stable tag if available.
286-296: Good: centralized replaces to InjectiveLabs Cosmos SDK.Consolidating
cosmossdk.io/*to a single stack version reduces diamond deps and ABI drift.
298-298: LGTM: wasmd replace to InjectiveLabs fork.Pin looks consistent with the rest of the EVM/Comet stack.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/injective-test-tube/libinjectivetesttube/go.mod (1)
3-3: Invalid Go version syntax ingodirective (will break tooling).Use major.minor only; patch versions are not allowed.
-go 1.23.9 +go 1.23
♻️ Duplicate comments (2)
packages/injective-test-tube/libinjectivetesttube/go.mod (2)
306-306: Module path casing likely wrong (can cause “module declares its path as …” errors).- github.com/cometbft/cometbft/api => github.com/injectivelabs/cometbft/api v1.0.0-inj.2 + github.com/cometbft/cometbft/api => github.com/InjectiveLabs/cometbft/api v1.0.0-inj.2
275-275: Malformed pseudo‑version for goleveldb in require (replace fixes it but keep require sane).- github.com/syndtr/goleveldb v1.0.1-0.20220721030215-126854af5e6v0d // indirect + github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 // indirectAlso applies to: 324-324
🧹 Nitpick comments (6)
packages/injective-test-tube/src/module/helpers.rs (6)
5-14: Prefer enum over magic number for vote option.Use the typed enum to avoid brittle numeric literals.
use injective_std::{ shim::Any, types::cosmos::{ base::v1beta1::Coin as BaseCoin, - gov::v1::{MsgSubmitProposal, MsgVote}, + gov::v1::{MsgSubmitProposal, MsgVote, VoteOption}, gov::v1beta1::MsgSubmitProposal as MsgSubmitProposalV1Beta1, },gov.vote( MsgVote { proposal_id: u64::from_str(&proposal_id).unwrap(), voter: validator.address(), - option: 1i32, + option: VoteOption::Yes as i32, metadata: "".to_string(), }, validator, )gov.vote( MsgVote { proposal_id: u64::from_str(&proposal_id).unwrap(), voter: validator.address(), - option: 1i32, + option: VoteOption::Yes as i32, metadata: "".to_string(), }, validator, )Also applies to: 81-90, 144-153
27-31: gRPC path string is brittle; prefer typed query or a shared constant.Small typo in path breaks at runtime. If possible, route through a typed client or centralize the path in a const.
34-36: Handle missing params more defensively.
unwrap()will panic if the chain returns no params; add context or bail gracefully in helpers.- let mut exchange_params = res.params.unwrap(); + let mut exchange_params = res + .params + .expect("exchange v2 params not found in QueryExchangeParamsResponse")
58-61: Hard‑coded deposit and denom.Consider deriving from chain config (denom) and using helper to format 18‑dp amounts to avoid typos across helpers.
72-79: Event attribute indexing is fragile.Relying on
attributes[0]may break if ordering changes. Search by key (e.g.,proposal_id) instead.- let proposal_id = res - .events - .iter() - .find(|e| e.ty == "submit_proposal") - .unwrap() - .attributes[0] - .value - .clone(); + let proposal_id = res + .events + .iter() + .find(|e| e.ty == "submit_proposal") + .and_then(|e| e.attributes.iter().find(|a| a.key == "proposal_id")) + .expect("submit_proposal event missing proposal_id attribute") + .value + .clone();Also applies to: 135-142
38-40: Nit: comment typo.“int the future” → “in the future”.
📜 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 (4)
packages/injective-test-tube/Cargo.toml(1 hunks)packages/injective-test-tube/libinjectivetesttube/go.mod(7 hunks)packages/injective-test-tube/src/module/exchange.rs(0 hunks)packages/injective-test-tube/src/module/helpers.rs(4 hunks)
💤 Files with no reviewable changes (1)
- packages/injective-test-tube/src/module/exchange.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/injective-test-tube/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
packages/injective-test-tube/src/module/helpers.rs (1)
packages/injective-test-tube/src/runner/app.rs (1)
app(301-302)
⏰ 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). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Test Suite
🔇 Additional comments (4)
packages/injective-test-tube/src/module/helpers.rs (2)
68-71: Ensure the proposal actually passes before relying on new params.
launch_price_feed_oracleadvances time;add_exchange_admindoes not. For consistency and to avoid flakiness, consider advancing time or awaiting state transition after voting.Also applies to: 155-157
42-56: Migration to v2 MsgUpdateParams wiring looks correct.TYPE_URL and encoding align with v2; LGTM.
packages/injective-test-tube/libinjectivetesttube/go.mod (2)
287-296: Cosmos SDK stack realignment via replace rules looks coherent.The
cosmossdk.io/*andgithub.com/cosmos/cosmos-sdkreplacements point consistently toInjectiveLabsv0.50.13‑evm‑comet1‑inj.6.Also applies to: 307-307
300-300: Org switch for injective-core via replace.Looks fine if the target module’s declared path matches expectations. Please confirm
v1.16.4is the intended pin for this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/injective-test-tube/libinjectivetesttube/testenv/setup.go (1)
153-153: UTC genesis time: LGTM; consider deterministic clock for reproducible tests.Nice cleanup overall (dropping the fmt import/print). Keeping InitChain/Header times in UTC is correct. If test reproducibility across runs matters, allow overriding the clock (env/flag) instead of hard-coding time.Now().
Minimal tweak within this line:
- now := time.Now().UTC() + // Allow deterministic runs with INJ_TEST_TIME (RFC3339); fallback to now. + now := func() time.Time { + if s := os.Getenv("INJ_TEST_TIME"); s != "" { + if t, err := time.Parse(time.RFC3339, s); err == nil { + return t.UTC() + } + } + return time.Now().UTC() + }()Additional supporting change (imports):
import ( "encoding/json" + "os" "strings" "time"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/injective-test-tube/libinjectivetesttube/testenv/setup.go(2 hunks)
⏰ 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). (1)
- GitHub Check: Test Suite
🔇 Additional comments (1)
packages/injective-test-tube/libinjectivetesttube/testenv/setup.go (1)
166-166: Verify Height=0 context usage; prefer Height=1 after InitChain.Found one NewUncachedContext(... Header{Height: 0}) at packages/injective-test-tube/libinjectivetesttube/testenv/setup.go:166 and no obvious BlockHeight comparisons in the repo scan — Height=0 is pre‑first‑block, so either set to 1 or ensure the flow runs before InitChain/BeginBlock.
Optional change:
- ctx := appInstance.NewUncachedContext(false, cmtproto.Header{Height: 0, ChainID: "injective-777", Time: now}) + ctx := appInstance.NewUncachedContext(false, cmtproto.Header{Height: 1, ChainID: "injective-777", Time: now})
* due to HumanReadableUpgradeBlockHeightPerChainID set to 490 currently for devnet chain ids, we need to prevent this from happening for the first blocks * hopefully can remove this again in the future
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/injective-test-tube/libinjectivetesttube/testenv/setup.go (3)
153-153: Use a deterministic genesis time to avoid flaky tests and non-reproducible snapshots.
Using time.Now() makes replaying tests and snapshot diffs noisy. Prefer a fixed or injectable time.- now := time.Now().UTC() + // Deterministic genesis time for stable tests/snapshots. + now := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)
167-168: Tie ctx header height to InitialHeight to reflect lastBlockHeight semantics.
If the intent is lastBlockHeight = initialHeight - 1, compute it; don’t hardcode 499.- ctx := appInstance.NewUncachedContext(false, cmtproto.Header{Height: 499, ChainID: "injective-777", Time: now}) + ctx := appInstance.NewUncachedContext(false, cmtproto.Header{ + Height: initialHeight - 1, + ChainID: "injective-777", + Time: now, + })
155-163: Extract initialHeight into a single variable and reuse it for InitChain and the ctx header.InitChain uses InitialHeight: 500 while the ctx header below is Height: 499 (packages/injective-test-tube/libinjectivetesttube/testenv/setup.go:162,167). Declare initialHeight := int64(500) once and use InitialHeight: initialHeight and ctx header Height: initialHeight-1.
_, err = appInstance.InitChain( &abci.InitChainRequest{ ChainId: "injective-777", Validators: []abci.ValidatorUpdate{}, ConsensusParams: DefaultConsensusParams, AppStateBytes: stateBytes, Time: now, - InitialHeight: 500, + // Set once and reuse (see ctx header below). + // CometBFT treats lastBlockHeight as initialHeight-1. + InitialHeight: initialHeight, }, )Declare the variable before the InitChain call:
+ // Keep initial height in one place to avoid magic numbers. + initialHeight := int64(500)Also update the context creation to use the derived value: replace Height: 499 with Height: initialHeight-1.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/injective-test-tube/libinjectivetesttube/testenv/setup.go(2 hunks)
⏰ 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). (1)
- GitHub Check: Test Suite
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/injective-test-tube/src/module/helpers.rs (5)
35-38: Helper unexpectedly mutates unrelated param (max_derivative_order_side_count)This function is named “add_exchange_admin” but also changes max_derivative_order_side_count to 300. That’s a surprising side effect and can skew tests.
Remove the mutation or make it optional:
- exchange_params.max_derivative_order_side_count = 300u32;
39-41: Hard-coded gov module address may break across networks/upgradesinj10d07y265… can change with address derivation/prefix changes or module wiring. Derive authority if exposed by the module, accept it as a parameter, or centralize in a single constant behind a feature per network.
73-81: Brittle proposal_id extraction (attributes[0])Index 0 isn’t guaranteed to be the proposal_id attribute. Search by key for stability.
Apply:
- let proposal_id = res - .events - .iter() - .find(|e| e.ty == "submit_proposal") - .unwrap() - .attributes[0] - .value - .clone(); + let proposal_id = res + .events + .iter() + .find(|e| e.ty == "submit_proposal") + .and_then(|e| { + e.attributes.iter().find_map(|a| { + if a.key == "proposal_id" { Some(a.value.clone()) } else { None } + }) + }) + .expect("submit_proposal event missing proposal_id attribute");
137-145: Repeat: make proposal_id extraction robustSame issue as above; search by key instead of taking attributes[0].
Apply:
- let proposal_id = res - .events - .iter() - .find(|e| e.ty == "submit_proposal") - .unwrap() - .attributes[0] - .value - .to_owned(); + let proposal_id = res + .events + .iter() + .find(|e| e.ty == "submit_proposal") + .and_then(|e| { + e.attributes.iter().find_map(|a| { + if a.key == "proposal_id" { Some(a.value.clone()) } else { None } + }) + }) + .expect("submit_proposal event missing proposal_id attribute");
193-195: expiry = -1 may be invalidUnless the module explicitly documents -1 as “no expiry,” use 0 or omit when supported. Negative durations often fail validation.
- expiry: -1i64, + // 0 => no expiry (if supported by module); otherwise set to a valid future timestamp/height + expiry: 0i64,
🧹 Nitpick comments (8)
packages/injective-test-tube/src/runner/app.rs (2)
278-296: Avoid hardcoding 503; derive from the live height to de-flake tests.Using a constant couples the test to current env details. Read the baseline from the app instead.
- let mut current_block_height = 503i64; - assert_eq!(app.get_block_height(), current_block_height); + let mut current_block_height = app.get_block_height(); + assert_eq!(app.get_block_height(), current_block_height);
496-499: Switch to Uint256 math is correct.Arithmetic and comparison are type-safe. Minor:
Uint256::from(bob_balance)reads a bit clearer thannew, but optional. (docs.rs)- Uint256::new(bob_balance), - Uint256::new(initial_balance) - amount.amount + Uint256::from(bob_balance), + Uint256::from(initial_balance) - amount.amountpackages/injective-test-tube/src/module/helpers.rs (6)
21-21: Don’t ship helpers behind #[allow(dead_code)]; gate themPrefer cfg(test) or a “test-support” feature instead of globally suppressing dead code. This avoids leaking test-only helpers into downstream builds.
Apply to each helper:
-#[allow(dead_code)] +#[cfg(any(test, feature = "test-support"))]Also applies to: 94-94, 173-173
28-33: Avoid hard-coded gRPC pathsLiteral “/injective.exchange.v2.Query/QueryExchangeParams” is brittle. Prefer the typed query variant used elsewhere (see packages/injective-test-tube/src/runner/app.rs Lines 302-303) or at least hoist the path into a single const to prevent drift.
Also applies to: 30-31
59-62: Governance deposits are set to 1e20 INJ—likely unnecessary and brittleSuch a large fixed deposit risks failures if the proposer account funding changes. Consider querying gov params for min_deposit and using that value (plus a small buffer), or gate the amount behind a config.
Also applies to: 127-130
157-159: increase_time(10) likely won’t surpass voting_periodUse the chain’s gov voting_period (v1 params) to compute the delta (plus 1s) and advance time accordingly; also ensure a block is committed after the jump if required by the app harness.
Example:
- app.increase_time(10u64); + // Derive from gov params instead of a magic number + // let voting_secs = gov_v1_params.voting_period.unwrap().seconds as u64 + 1; + // app.increase_time(voting_secs);
123-126: Avoid hard-coded type_url stringPrefer a library-provided TYPE_URL (if exposed) or a single const to reduce drift risk.
- type_url: "/injective.oracle.v1beta1.GrantPriceFeederPrivilegeProposal" - .to_string(), + // e.g., GrantPriceFeederPrivilegeProposal::TYPE_URL.to_string() + type_url: "/injective.oracle.v1beta1.GrantPriceFeederPrivilegeProposal".to_string(),
33-33: Prefer expect(..) or Result over unwrap()Swap unwrap() for expect(..) to surface context on failure.
- .unwrap(); + .expect("query exchange params failed");(Consider returning Result from helpers to avoid panics in harnessed tests.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/injective-test-tube/src/module/helpers.rs(5 hunks)packages/injective-test-tube/src/module/wasm.rs(2 hunks)packages/injective-test-tube/src/runner/app.rs(5 hunks)packages/test-tube/src/runner/app.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/test-tube/src/runner/app.rs
- packages/injective-test-tube/src/module/wasm.rs
🧰 Additional context used
🧬 Code graph analysis (2)
packages/injective-test-tube/src/runner/app.rs (1)
packages/test-tube/src/runner/app.rs (2)
get_block_height(144-146)new(36-50)
packages/injective-test-tube/src/module/helpers.rs (1)
packages/injective-test-tube/src/runner/app.rs (1)
app(303-304)
⏰ 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). (2)
- GitHub Check: Lints
- GitHub Check: Test Suite
🔇 Additional comments (6)
packages/injective-test-tube/src/runner/app.rs (2)
155-155: Importing Uint256 is correct for new Coin/amount arithmetic.Matches cosmwasm-std ≥3 where Coin.amount is Uint256 and Uint256::new(...) exists. LGTM. (docs.rs)
Please confirm the workspace pins cosmwasm-std to 3.x to avoid type mismatches across crates.
204-209: Block height expectations updated to new init height — OK.The assertions align with the updated testenv that starts around height 500. No issues.
packages/injective-test-tube/src/module/helpers.rs (4)
5-14: Mixed gov/v1 and gov/v1beta1 + v2 exchange: confirm cross-version behaviorYou’re submitting a v2 exchange update via gov/v1, while the price-feeder path uses gov/v1beta1 content. Please confirm Injective v1.16.4 governance still accepts v1beta1 content for the oracle proposal in the same app/runtime. If not, consider moving the price-feeder content to the current gov surface (or gate by feature).
82-92: Gov vote usage looks correctVoting YES with the validator account after submission is fine.
119-135: v1beta1 submit_proposal path: verify continued supportThe v1beta1 MsgSubmitProposal flow + v1beta1 oracle content is intentional, but confirm it’s still supported end-to-end in your target env; otherwise migrate content to the current gov surface.
160-170: Relay price feed payload LGTMVector lengths align and decimal string pass-through matches the message shape.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/injective-test-tube/libinjectivetesttube/testenv/setup.go (3)
134-135: Use keyed struct literals for both entries (consistency/readability).Mixing keyed and positional literals is valid but easy to misorder later. Prefer keyed for both.
-exchangeGen.DenomDecimals = []exchangetypesv2.DenomDecimals{{Denom: "inj", Decimals: 18}, {"usdt", 6}} +exchangeGen.DenomDecimals = []exchangetypesv2.DenomDecimals{ + {Denom: "inj", Decimals: 18}, + {Denom: "usdt", Decimals: 6}, +}Also verify these decimals match what tests assert (inj=18, usdt=6).
155-155: Consider deterministic genesis time for test stability.Using time.Now().UTC() is fine, but a fixed time avoids flakiness if any duration math changes. Optional.
- now := time.Now().UTC() + now := time.Unix(0, 0).UTC()
164-165: InitialHeight and header alignment: good; consider centralizing constant.Setting InitialHeight=500 and priming ctx at 499 is correct. Factor 500 into a const to prevent drift.
+ const initHeight int64 = 500 ... - InitialHeight: 500, + InitialHeight: initHeight,packages/injective-test-tube/src/module/auction.rs (4)
137-137: Remove debug println in tests (noise).Keep tests quiet unless failing.
- println!("{:?}", basket_response_after_increase); + // debug: println!("{:?}", basket_response_after_increase);
148-156: Strengthen the denom-decimals assertion.Length==2 passes even if mapping is wrong. Assert the actual decimals for each denom.
- assert_eq!(decimals.denom_decimals.len(), 2); + use std::collections::HashMap; + let map: HashMap<_, _> = decimals + .denom_decimals + .into_iter() + .map(|d| (d.denom, d.decimals)) + .collect(); + assert_eq!(map.get("inj"), Some(&18)); + assert_eq!(map.get("usdt"), Some(&6));
157-177: Deposit flow looks correct; optional: prefer v2 message for consistency.You’re using v1beta1::MsgDeposit while other new paths use v2. Functionally fine, but consider the v2 message to reduce version mixing.
- use injective_std::types::injective::exchange::v1beta1::MsgDeposit; + use injective_std::types::injective::exchange::v2::MsgDeposit; ... - let _ = exchange - .deposit( + let _ = exchange + .deposit_v2( MsgDeposit {
179-181: Time advance math: guard against underflow on future changes.If block_time_sec ever equals closing_time due to env tweaks, subtracting then adding 1 stays safe, but a saturating add would be more explicit. Optional.
Wrap with a checked calc or assert precondition before increase_time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/injective-test-tube/libinjectivetesttube/testenv/setup.go(3 hunks)packages/injective-test-tube/src/module/auction.rs(4 hunks)packages/injective-test-tube/src/module/exchange.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/injective-test-tube/src/module/auction.rs (2)
packages/injective-test-tube/src/runner/app.rs (2)
app(303-304)new(25-34)packages/injective-test-tube/src/module/exchange.rs (1)
new(9-11)
⏰ 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). (3)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Lints
- GitHub Check: Test Suite
🔇 Additional comments (8)
packages/injective-test-tube/src/module/exchange.rs (1)
275-275: Test import consolidation looks good.Brings Exchange into scope for the tests; consistent with added coverage.
packages/injective-test-tube/libinjectivetesttube/testenv/setup.go (1)
169-169: LGTM on context header pre-height.Height 499 ensures the next BeginBlock is 500, matching InitialHeight.
packages/injective-test-tube/src/module/auction.rs (6)
41-43: New test deps are appropriate.Bringing Auction, Exchange, and Coin into scope fits the extended flow.
50-51: Good: using v2 QueryDenomDecimalsRequest.Matches the new exchange query surface (once routed to v2 path).
After fixing the exchange route, ensure the test still passes.
52-53: Account import is needed for signer/trader setup.All good.
57-58: Initializing Exchange in the test is correct.Keeps the test self-contained with both Auction and Exchange usage.
69-70: Whitelist field assertion: nice catch.Asserting bidders_whitelist avoids silent schema drift.
182-188: Post-close assertions: good end-to-end check.Verifies basket carries funds and round increments as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/injective-test-tube/src/module/auction.rs (6)
64-71: Avoid brittle struct equality for Params; assert fields.Comparing the entire Params object will break on new fields/defaults. Assert specific fields instead.
Apply this diff:
- assert_eq!( - response.params, - Some(Params { - auction_period: 604800, - min_next_bid_increment_rate: 2_500_000_000_000_000u128.to_string(), - inj_basket_max_cap: "10000000000000000000000".to_string(), - bidders_whitelist: vec![], - }) - ); + let p = response.params.expect("auction params must be present"); + assert_eq!(p.auction_period, 604800); + assert_eq!(p.min_next_bid_increment_rate, 2_500_000_000_000_000u128.to_string()); + assert_eq!(p.inj_basket_max_cap, "10000000000000000000000"); + assert!(p.bidders_whitelist.is_empty());
140-148: Strengthen denom-decimals checks; don’t rely on ordering.Verify actual values (inj=18, usdt=6) and prefer expect over unwrap.
Apply this diff:
- // validate if coins on basket - let decimals = exchange - .query_denom_decimals(&QueryDenomDecimalsRequest { - denoms: vec!["inj".to_string(), "usdt".to_string()], - }) - .unwrap(); - - assert_eq!(decimals.denom_decimals.len(), 2); + let initial_round = round; // capture for later relative assertions + // validate denom decimals + let decimals = exchange + .query_denom_decimals(&QueryDenomDecimalsRequest { + denoms: vec!["inj".to_string(), "usdt".to_string()], + }) + .expect("denom decimals query failed"); + use std::collections::HashMap; + let map: HashMap<_, _> = decimals + .denom_decimals + .iter() + .map(|d| (d.denom.clone(), d.decimals)) + .collect(); + assert_eq!(map.get("inj"), Some(&18)); + assert_eq!(map.get("usdt"), Some(&6));
151-154: Use correct decimal scale for initial balances (USDT=6).USDT amount is minted with 18‑dec scale; prefer 6 to match the configured decimals (and compute amounts programmatically).
Apply this diff:
- let trader = app - .init_account(&[ - Coin::new(10_000_000_000_000_000_000_000u128, "inj"), - Coin::new(100_000_000_000_000_000_000u128, "usdt"), - ]) + let inj_amount = 10_000u128 * 10u128.pow(18); // 10_000 INJ + let usdt_amount = 100_000u128 * 10u128.pow(6); // 100_000 USDT + let trader = app + .init_account(&[ + Coin::new(inj_amount, "inj"), + Coin::new(usdt_amount, "usdt"), + ]) .unwrap();
157-169: Prefer expect over unwrap and surface intent.Make failures easier to diagnose in CI.
Apply this diff:
- let _ = exchange + let _ = exchange .deposit( MsgDeposit { sender: trader.address().to_string(), subaccount_id: auction_subaccount.to_string(), amount: Some(BaseCoin { denom: "inj".to_string(), amount: "1000000000000000000000".to_string(), }), }, &trader, ) - .unwrap(); + .expect("deposit INJ into auction subaccount");
171-173: Compute delta from current time; avoid underflow with saturating ops.Safer if env timings shift.
Apply this diff:
- let block_time_sec = app.get_block_time_seconds() as u64; - app.increase_time(basket_response_after_increase.auction_closing_time - block_time_sec + 1); + let now = app.get_block_time_seconds() as u64; + let delta = basket_response_after_increase + .auction_closing_time + .saturating_sub(now) + .saturating_add(1); + app.increase_time(delta);
174-180: Make final checks more robust (relative round; verify INJ present).Avoid hard-coding round==2 and relying on single-coin basket length.
Apply this diff:
- let basket_res = auction + let basket_res = auction .query_current_auction_basket(&QueryCurrentAuctionBasketRequest {}) .expect("query_current_auction_basket should succeed"); - - assert_eq!(basket_res.amount.len(), 1); - assert_eq!(basket_res.auction_round, 2); + // Basket should contain INJ after deposit + assert!( + basket_res.amount.iter().any(|c| c.denom == "inj"), + "basket must contain INJ after deposit" + ); + // Round advanced by two closings in this test flow + assert_eq!(basket_res.auction_round, initial_round + 2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/injective-test-tube/src/module/auction.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/injective-test-tube/src/module/auction.rs (3)
packages/injective-test-tube/src/runner/app.rs (2)
app(303-304)new(25-34)packages/injective-test-tube/src/module/exchange.rs (1)
new(9-11)packages/test-tube/src/runner/app.rs (1)
new(36-50)
⏰ 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). (2)
- GitHub Check: Lints
- GitHub Check: Test Suite
🔇 Additional comments (3)
packages/injective-test-tube/src/module/auction.rs (3)
41-43: LGTM: test imports for Exchange flow and denom-decimals.The added imports are scoped to the test module and look correct.
Also applies to: 50-52
57-57: LGTM: construct Exchange from app.Matches the Module::new pattern used elsewhere.
149-149: Replace hard-coded auction subaccount with a canonical constant/helperNo canonical helper/constant was found in the repo; replace the literal at packages/injective-test-tube/src/module/auction.rs:149 with a shared named constant or helper (e.g., AUCTION_SUBACCOUNT) or add/export one from an appropriate module — if the value is protocol-defined, verify the exact value before centralizing.
2 participants
Summary by CodeRabbit
New Features
Chores
Bug Fixes / Behavior
Tests
Style