feat: add 1.18 upgrade + various fixes#263
Conversation
📝 WalkthroughWalkthroughAdds Injective v2 exchange support and open_notional_cap usage, introduces add_exchange_admin_v2 (governance flow), updates workspace dependencies and CI Rust toolchain, extends query/test surfaces (spot & derivative), and adjusts numeric scaling and test assertions. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/Basic.yml (1)
25-68:⚠️ Potential issue | 🟠 MajorUpdate deprecated GitHub Actions to Node20-compatible versions.
actions-rs/cargo@v1,actions-rs/toolchain@v1, andactions/checkout@v2are archived and unmaintained since October 2023. Additionally,actions/checkout@v2was built for Node.js 12, which GitHub removed from runners on August 14, 2023. These outdated actions may break on current GitHub runners.Migrate to maintained replacements across all three jobs (test, integration, lints):
- Replace
actions/checkout@v2withactions/checkout@v4- Replace
actions-rs/toolchain@v1withactions-rust-lang/setup-rust-toolchain@v1- Replace
actions-rs/cargo@v1with directcargocommands🔧 Example update (apply similarly to all jobs)
- - name: Checkout sources - uses: actions/checkout@v2 + - name: Checkout sources + uses: actions/checkout@v4 - - name: Install stable toolchain - uses: actions-rs/toolchain@v1 + - name: Install stable toolchain + uses: actions-rust-lang/setup-rust-toolchain@v1 with: profile: minimal toolchain: 1.87.0 target: wasm32-unknown-unknown override: true - - name: Run all Rust tests - uses: actions-rs/cargo@v1 - with: - command: test - toolchain: 1.87.0 - args: --workspace --all-targets --locked + - name: Run all Rust tests + run: cargo test --workspace --all-targets --locked env: RUST_BACKTRACE: 1
🤖 Fix all issues with AI agents
In `@contracts/injective-cosmwasm-stargate-example/src/query.rs`:
- Around line 29-32: The function handle_query_spot_market currently calls
ExchangeQuerierV2::derivative_market(...) which is incorrect for a spot query;
update handle_query_spot_market to call
ExchangeQuerierV2::spot_market(market_id.to_string()) instead of
derivative_market, keeping the existing querier construction
(ExchangeQuerierV2::new(&deps.querier)) and wrapping the result with
to_json_binary(...) to preserve the StdResult<Binary> return.
In `@contracts/injective-cosmwasm-stargate-example/src/testing/test_bank.rs`:
- Around line 31-83: The test is misnamed and uses the wrong query/response
types and assertion: rename the function to test_query_derivative_market, stop
treating a derivative market query as a spot query, and replace
QueryMsg::QuerySpotMarket with the derivative query variant (e.g.,
QueryMsg::QueryDerivativeMarket or the repo's QueryMsg variant for derivative
markets) when calling wasm.query; change the expected response type from
ParamResponse<BankParams> to the correct derivative market response type (the
struct returned by the derivative market query) and assert on real market fields
like ticker, quote_denom, or market_id (references: get_perpetual_market_id,
exchange.instant_perpetual_market_launch_v2, wasm.query,
ParamResponse<BankParams>, default_send_enabled).
🧹 Nitpick comments (3)
contracts/injective-cosmwasm-stargate-example/src/utils.rs (1)
381-390: Consider gating verbose printlns to keep CI logs clean.
If these are only for ad‑hoc debugging, an env flag keeps normal runs quieter.🧹 Optional gating
- println!( - "Adding derivative order with price: {}, quantity: {}, margin: {}", - price, quantity, order_margin - ); - println!( - "OLD price: {}, quantity: {}, margin: {}", - order.price.as_str(), - order.quantity.as_str(), - margin - ); + if std::env::var("INJ_TEST_DEBUG").is_ok() { + println!( + "Adding derivative order with price: {}, quantity: {}, margin: {}", + price, quantity, order_margin + ); + println!( + "OLD price: {}, quantity: {}, margin: {}", + order.price.as_str(), + order.quantity.as_str(), + margin + ); + }contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs (2)
88-88: Redundant.to_string()call ondec_to_protoresult.
dec_to_protoalready returns aString, so the additional.to_string()call is unnecessary.♻️ Suggested fix
- maker_fee_rate: dec_to_proto(maker_fee_rate).to_string(), + maker_fee_rate: dec_to_proto(maker_fee_rate),
342-344: Disabled test requires follow-up.The
test_query_derivative_market_mid_price_and_tobtest is now ignored with#[ignore = "TODO fix me"]. This should be tracked and addressed before the PR is fully complete.Would you like me to open an issue to track fixing this disabled test?
| #[test] | ||
| #[cfg_attr(not(feature = "integration"), ignore)] | ||
| fn test_query_spot_market() { | ||
| let env = Setup::new(ExchangeType::Derivative); | ||
| let wasm = Wasm::new(&env.app); | ||
| let exchange = Exchange::new(&env.app); | ||
| let ticker = "INJ/USDT".to_string(); | ||
| let initial_margin_ratio = FPDecimal::must_from_str("0.195"); | ||
| let maintenance_margin_ratio = FPDecimal::must_from_str("0.05"); | ||
| let min_price_tick_size = FPDecimal::must_from_str("0.1"); | ||
| let min_quantity_tick_size = FPDecimal::must_from_str("1000000000000000"); | ||
| let min_notional = FPDecimal::must_from_str("0.001"); | ||
| let quote_denom = QUOTE_DENOM.to_string(); | ||
| let maker_fee_rate = FPDecimal::must_from_str("-0.0001"); | ||
| let taker_fee_rate = FPDecimal::must_from_str("0.001"); | ||
|
|
||
| // add_exchange_admin(&env.app, &env.validator, env.owner.address()); | ||
|
|
||
| exchange | ||
| .instant_perpetual_market_launch_v2( | ||
| v2::MsgInstantPerpetualMarketLaunch { | ||
| sender: env.owner.address(), | ||
| ticker: ticker.to_owned(), | ||
| quote_denom: quote_denom.to_owned(), | ||
| oracle_base: BASE_DENOM.to_string(), | ||
| oracle_quote: quote_denom.to_owned(), | ||
| oracle_scale_factor: 6u32, | ||
| oracle_type: 2i32, | ||
| maker_fee_rate: dec_to_proto(maker_fee_rate), | ||
| taker_fee_rate: dec_to_proto(taker_fee_rate), | ||
| initial_margin_ratio: dec_to_proto(initial_margin_ratio), | ||
| maintenance_margin_ratio: dec_to_proto(maintenance_margin_ratio), | ||
| min_price_tick_size: dec_to_proto(min_price_tick_size), | ||
| min_quantity_tick_size: dec_to_proto(min_quantity_tick_size), | ||
| min_notional: dec_to_proto(min_notional), | ||
| reduce_margin_ratio: dec_to_proto(initial_margin_ratio), | ||
| open_notional_cap: Some(OpenNotionalCap { | ||
| cap: Some(Cap::Uncapped(OpenNotionalCapUncapped {})), | ||
| }), | ||
| }, | ||
| &env.owner, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let derivative_market_id = get_perpetual_market_id(&exchange, ticker.to_owned()); | ||
|
|
||
| let query_msg = QueryMsg::QuerySpotMarket { | ||
| market_id: derivative_market_id.to_string(), | ||
| }; | ||
|
|
||
| let contract_response: ParamResponse<BankParams> = wasm.query(&env.contract_address, &query_msg).unwrap(); | ||
| assert!(contract_response.params.default_send_enabled); | ||
| } |
There was a problem hiding this comment.
Test has multiple issues: incorrect naming, wrong query type, and meaningless assertion.
This test has several problems:
-
Misleading name: The function is named
test_query_spot_marketbut it launches and queries a derivative perpetual market. -
Setup mismatch: Uses
ExchangeType::Derivativebut then manually launches another perpetual market with the same ticker "INJ/USDT", which likely already exists from the setup. -
Wrong query: Uses
QueryMsg::QuerySpotMarket(Line 77) to query what is actually a derivative market ID. This likely works due to the bug inhandle_query_spot_marketthat actually queriesderivative_market(). -
Incorrect assertion: The response is cast to
ParamResponse<BankParams>(Line 81) and assertsdefault_send_enabled, which is completely unrelated to market query responses. This assertion will fail or give false positives.
Consider fixing the test:
- Rename to
test_query_derivative_marketif testing derivative markets - Use
QueryMsg::TestDerivativeMarketQueryfor derivative market queries - Assert on actual market response fields (ticker, quote_denom, etc.)
🤖 Prompt for AI Agents
In `@contracts/injective-cosmwasm-stargate-example/src/testing/test_bank.rs`
around lines 31 - 83, The test is misnamed and uses the wrong query/response
types and assertion: rename the function to test_query_derivative_market, stop
treating a derivative market query as a spot query, and replace
QueryMsg::QuerySpotMarket with the derivative query variant (e.g.,
QueryMsg::QueryDerivativeMarket or the repo's QueryMsg variant for derivative
markets) when calling wasm.query; change the expected response type from
ParamResponse<BankParams> to the correct derivative market response type (the
struct returned by the derivative market query) and assert on real market fields
like ticker, quote_denom, or market_id (references: get_perpetual_market_id,
exchange.instant_perpetual_market_launch_v2, wasm.query,
ParamResponse<BankParams>, default_send_enabled).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs`:
- Around line 342-345: The test function
test_query_derivative_market_mid_price_and_tob is disabled with #[ignore = "TODO
fix me"]; remove or change that attribute to re-enable the test (restore the
original #[test] behaviour and, if intended, re-enable the
#[cfg_attr(not(feature = "integration"), ignore)] pattern instead of a permanent
ignore), run the test suite to confirm it passes, and add a follow-up task by
creating an issue or leaving a clear TODO comment referencing the issue number
explaining what needed fixing if it flaps; ensure you update any CI
configuration if this is an integration test so it runs in the appropriate
pipeline.
🧹 Nitpick comments (2)
contracts/injective-cosmwasm-stargate-example/src/testing/test_auction.rs (1)
64-64: Remove the tautological auction_round bound.
Uint64can’t exceedu64::MAX, so this assertion never fails and adds no test value. Consider removing it or replacing with a meaningful expectation (e.g., non-zero or monotonic).🔧 Proposed clean-up
- assert!(response.auction_round <= Uint64::from(u64::MAX));contracts/injective-cosmwasm-stargate-example/src/utils.rs (1)
382-392: Consider gating or removing verbose printlns
These logs can add noise to test output once the debugging need passes.🧹 Optional cleanup
- println!( - "Adding derivative order with price: {}, quantity: {}, margin: {}", - price, quantity, order_margin - ); - println!( - "OLD price: {}, quantity: {}, margin: {}", - order.price.as_str(), - order.quantity.as_str(), - margin - );
| #[ignore = "TODO fix me"] | ||
| #[test] | ||
| #[cfg_attr(not(feature = "integration"), ignore)] | ||
| //#[cfg_attr(not(feature = "integration"), ignore)] | ||
| fn test_query_derivative_market_mid_price_and_tob() { |
There was a problem hiding this comment.
Track and re-enable the ignored mid‑price/TOB test
Please add a follow-up task so this doesn’t stay permanently disabled.
Would you like me to draft a quick issue or help re-enable this test?
🤖 Prompt for AI Agents
In
`@contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs`
around lines 342 - 345, The test function
test_query_derivative_market_mid_price_and_tob is disabled with #[ignore = "TODO
fix me"]; remove or change that attribute to re-enable the test (restore the
original #[test] behaviour and, if intended, re-enable the
#[cfg_attr(not(feature = "integration"), ignore)] pattern instead of a permanent
ignore), run the test suite to confirm it passes, and add a follow-up task by
creating an issue or leaving a clear TODO comment referencing the issue number
explaining what needed fixing if it flaps; ensure you update any CI
configuration if this is an integration test so it runs in the appropriate
pipeline.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs (1)
88-89:⚠️ Potential issue | 🟡 MinorRemove redundant
.to_string()on line 88.
dec_to_protoalready returns aString, so appending.to_string()is redundant. Line 88 should match line 89 and all other field assignments in this struct (and the same pattern appears at lines 152-153).Fix
- maker_fee_rate: dec_to_proto(maker_fee_rate).to_string(), + maker_fee_rate: dec_to_proto(maker_fee_rate),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs` around lines 88 - 89, Remove the redundant .to_string() call on the struct field assignment where maker_fee_rate is set; dec_to_proto already returns a String, so change the assignment that currently uses maker_fee_rate: dec_to_proto(maker_fee_rate).to_string() to match taker_fee_rate: dec_to_proto(taker_fee_rate). Also update the identical redundant .to_string() usage found in the second instance (the similar assignments around the later block where maker_fee_rate/taker_fee_rate are set) so both places call dec_to_proto(...) without .to_string().
🧹 Nitpick comments (2)
contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs (2)
178-181: Consider addingmin_price_tick_sizeassertion for consistency.The
test_query_derivative_marketfunction (line 118) assertsmin_price_tick_size, but this v2 variant does not. If intentional (e.g., v2 API handles it differently), please add a comment. Otherwise, consider adding the assertion for parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs` around lines 178 - 181, The test_query_derivative_market v2 variant currently checks min_quantity_tick_size, maker_fee_rate, taker_fee_rate, and initial_margin_ratio but omits asserting min_price_tick_size; update the test in test_exchange_derivative.rs to either add an assertion comparing response_market.min_price_tick_size to the expected min_price_tick_size (for parity with the v1 test_query_derivative_market) or, if v2 intentionally omits/changes this field, add a clarifying comment in the test near response_market explaining why min_price_tick_size is not asserted; reference response_market and the test_query_derivative_market function when making the change.
429-464: Consider adding a comment explaining the test flow.The test places a
Sellorder (line 436) then asserts"isBuy":truein the transient query result (line 464). This is presumably becauseTestTraderTransientDerivativeOrdersinternally places a buy order that matches against the sell. A brief comment explaining this two-step flow would improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs` around lines 429 - 464, Add a short inline comment above the test block explaining the two-step interaction: that add_derivative_order_as(...) places a persistent OrderType::Sell on the book, and the ExecuteMsg::TestTraderTransientDerivativeOrders (TestTraderTransientDerivativeOrders) call creates a transient buy that matches against that sell, which is why the emitted transient event's "isBuy":true refers to the transient order; place this comment near the add_derivative_order_as and the wasm.execute(...) call so readers understand why the assertion on "isBuy" expects true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs`:
- Around line 88-89: Remove the redundant .to_string() call on the struct field
assignment where maker_fee_rate is set; dec_to_proto already returns a String,
so change the assignment that currently uses maker_fee_rate:
dec_to_proto(maker_fee_rate).to_string() to match taker_fee_rate:
dec_to_proto(taker_fee_rate). Also update the identical redundant .to_string()
usage found in the second instance (the similar assignments around the later
block where maker_fee_rate/taker_fee_rate are set) so both places call
dec_to_proto(...) without .to_string().
---
Duplicate comments:
In
`@contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs`:
- Around line 342-345: The test function
test_query_derivative_market_mid_price_and_tob is permanently disabled with
#[ignore = "TODO fix me"]; either re-enable and fix it or add a tracking issue
link: remove the #[ignore = "TODO fix me"] attribute from
test_query_derivative_market_mid_price_and_tob, run the test, and resolve any
failures in the related query or helper functions until it passes, or replace
the ignore with a clear comment that references a created issue/ID (and include
that issue link/ID in the comment) so the TODO isn't forgotten.
---
Nitpick comments:
In
`@contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs`:
- Around line 178-181: The test_query_derivative_market v2 variant currently
checks min_quantity_tick_size, maker_fee_rate, taker_fee_rate, and
initial_margin_ratio but omits asserting min_price_tick_size; update the test in
test_exchange_derivative.rs to either add an assertion comparing
response_market.min_price_tick_size to the expected min_price_tick_size (for
parity with the v1 test_query_derivative_market) or, if v2 intentionally
omits/changes this field, add a clarifying comment in the test near
response_market explaining why min_price_tick_size is not asserted; reference
response_market and the test_query_derivative_market function when making the
change.
- Around line 429-464: Add a short inline comment above the test block
explaining the two-step interaction: that add_derivative_order_as(...) places a
persistent OrderType::Sell on the book, and the
ExecuteMsg::TestTraderTransientDerivativeOrders
(TestTraderTransientDerivativeOrders) call creates a transient buy that matches
against that sell, which is why the emitted transient event's "isBuy":true
refers to the transient order; place this comment near the
add_derivative_order_as and the wasm.execute(...) call so readers understand why
the assertion on "isBuy" expects true.
|
Lets fix contract test before margin |
2 participants
Summary by CodeRabbit
New Features
Tests
Chores