chore: add smaller fixes#21
Conversation
WalkthroughVersion bumped to 1.1.1. Migration code now uses plain contract name without crates.io prefix. Simulation margin in estimate_execution_buy_from_source uses input_quote_quantity. ExactOutputQuantity refund uses required_input. Tests updated: migration test runs unconditionally with artifact/name adjustments; an integration test expectation is corrected. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant S as Swap Contract
participant M as Market(s)
rect rgba(230,245,255,0.5)
note right of S: ExactOutputQuantity flow (changed refund)
U->>S: Execute swap (ExactOutput, coin_provided)
S->>M: Route swap to obtain exact output
M-->>S: Reports required_input for target output
S->>S: current_balance = required_input
S->>U: Refund = coin_provided - required_input
S-->>U: Success response incl. refund (if any)
end
sequenceDiagram
autonumber
participant Q as Query Client
participant S as Swap Contract
rect rgba(240,255,240,0.6)
note right of S: Simulation margin uses input_quote_quantity
Q->>S: estimate_execution_buy_from_source(is_simulation=true, input_quote_quantity)
S->>S: funds_for_margin = funds_in_contract + input_quote_quantity
S-->>Q: Estimation result (using updated margin)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/swap/src/contract.rs (1)
121-129: Migration should accept both legacy and new cw2 contract names.If older deployments stored cw2 name as "crates.io:swap-contract", the current match only on "swap-contract" will hard-fail migrations. Support both to avoid bricking upgrades.
Apply:
- match contract_version.contract.as_ref() { - "swap-contract" => match contract_version.version.as_ref() { + match contract_version.contract.as_ref() { + "swap-contract" | "crates.io:swap-contract" => match contract_version.version.as_ref() { "1.0.1" => { - set_contract_version(deps.storage, CONTRACT_NAME.to_string(), CONTRACT_VERSION)?; + set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?; } _ => return Err(ContractError::MigrationError {}), }, _ => return Err(ContractError::MigrationError {}), }
🧹 Nitpick comments (7)
contracts/swap/src/swap.rs (1)
91-97: Double-check Coin::new accepts the FPDecimal-derived refund_amount.refund is constructed via Coin::new(refund_amount, ...), where refund_amount is FPDecimal. If Coin::new expects an integer (Uint128/u128) in your cosmwasm_std version, this won’t compile or may require explicit conversion (e.g., refund_amount.int() or Into). If this compiled before, ignore; otherwise, make the conversion explicit for clarity.
Example tweak (only if needed):
- refund: Coin::new(refund_amount, source_denom.to_owned()), + refund: Coin::new(refund_amount.int().try_into().unwrap(), source_denom.to_owned()),contracts/swap/src/testing/migration_test.rs (2)
21-21: Artifact path rename: verify file exists in CI.Path switched to swap_contract-v101.wasm (underscore). Ensure the artifact is checked in at ../../contracts/swap/src/testing/test_artifacts/ with the new name, or the test will panic on read().
94-96: Optional: assert cw2 version post-migration via contract query.You already assert the code_id updated. Add a quick GetConfig query to assert the contract_version equals "1.1.1" to catch accidental version drift.
Example snippet (add after Line 96):
let cfg: crate::types::ConfigResponse = wasm .query(&swap_v101_address, &crate::msg::QueryMsg::GetConfig {}) .unwrap(); assert_eq!(cfg.contract_version, "1.1.1");contracts/swap/src/queries.rs (4)
161-167: Use roundedresult_quantityfor margin check to avoid overly conservative rejections
required_fundsis computed usingexpected_base_quantitybefore rounding to the market’s min tick. This can overshoot and cause unnecessary simulation failures. Consider basing the margin check on the actual order size (result_quantity) that would be placed after tick rounding.Apply this minimal change:
- let expected_base_quantity = available_swap_quote_funds / average_price; - let result_quantity = round_to_min_tick(expected_base_quantity, market.min_quantity_tick_size); + let expected_base_quantity = available_swap_quote_funds / average_price; + let result_quantity = round_to_min_tick(expected_base_quantity, market.min_quantity_tick_size); let fee_estimate = input_quote_quantity - available_swap_quote_funds; // check if user funds + contract funds are enough to create order - let required_funds = worst_price * expected_base_quantity * (FPDecimal::ONE + fee_percent); + let required_funds = worst_price * result_quantity * (FPDecimal::ONE + fee_percent);This mirrors the target-path behavior where you first round the base quantity, then validate funds against that rounded size.
167-173: Avoid panics in queries: replaceexpectwith error propagationUsing
expect("query own balance should not fail")will abort execution on transient node issues. Since the function already returnsStdResult, propagate the error instead. Same pattern exists in the target-path function.Apply these diffs in both places:
- let funds_in_contract = deps - .querier - .query_balance(contract_address, &market.quote_denom) - .expect("query own balance should not fail") - .amount - .into(); + let funds_in_contract: FPDecimal = deps + .querier + .query_balance(contract_address, &market.quote_denom)? + .amount + .into();This keeps the contract fail-safe without panicking on external query hiccups.
Also applies to: 228-234
376-383: Prefer slices over&Vec<T>in function signaturesAccepting
&[PriceLevel]is more idiomatic and flexible than&Vec<PriceLevel>; it also avoids unnecessary constraints on callers.-pub fn get_minimum_liquidity_levels( - _deps: &Deps<InjectiveQueryWrapper>, - levels: &Vec<PriceLevel>, +pub fn get_minimum_liquidity_levels( + _deps: &Deps<InjectiveQueryWrapper>, + levels: &[PriceLevel],No call-site changes needed due to
Derefcoercions.
451-568: Add a unit test to lock in the new simulation behavior (fees counted in available funds)Recommend a focused test that would have failed before this change (non-zero fee, small contract balance), and now passes.
Here’s a sketch you can adapt within your existing test module:
#[test] fn test_buy_from_source_simulation_uses_full_input_for_margin() { // Arrange: market with non-zero fee, small contract balance, // and orderbook such that worst_price > average_price. // Create deps/env, mock market, and orderbook levels so that: // - input_quote_quantity is just enough when fees are included // - using only available_swap_quote_funds would fail the margin check. // Assert: estimate_execution_buy_from_source(...) returns Ok(..) // after this change, whereas previous logic would have Err(..). }If useful, I can draft a full test using your inj mocks and helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcontracts/swap/src/testing/test_artifacts/swap_contract-v101.wasmis excluded by!**/*.wasm
📒 Files selected for processing (6)
contracts/swap/Cargo.toml(1 hunks)contracts/swap/src/contract.rs(2 hunks)contracts/swap/src/queries.rs(1 hunks)contracts/swap/src/swap.rs(1 hunks)contracts/swap/src/testing/integration_realistic_tests_exact_quantity.rs(1 hunks)contracts/swap/src/testing/migration_test.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/swap/src/testing/integration_realistic_tests_exact_quantity.rs (1)
contracts/swap/src/testing/test_utils.rs (1)
human_to_dec(801-803)
⏰ 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: Lints
- GitHub Check: Unit Tests
- GitHub Check: Lints
- GitHub Check: Unit Tests
🔇 Additional comments (7)
contracts/swap/src/swap.rs (1)
63-87: Fix uses required_input for refund — this is the right invariant.Using required_input rather than estimation.result_quantity ensures the refund excludes any rounding-up needed for tick sizes/quote-int casting, preventing over-refunds and underfunded executions. CurrentBalance is also aligned to required_input, which keeps subsequent step estimation consistent.
contracts/swap/src/contract.rs (1)
131-136: Good: expose new name/version via attributes.new_contract_name and new_contract_version attributes reflect the post-migration cw2 state and help observability.
contracts/swap/src/testing/migration_test.rs (1)
68-69: store_code name alignment looks consistent with artifact rename.Passing "swap_contract" to store_code aligns with the new underscore artifact naming convention; nice.
contracts/swap/src/testing/integration_realistic_tests_exact_quantity.rs (1)
801-804: Expectation tweak by 1 micro-unit is fine and likely driven by rounding changes.The USDT post-swap expectation from 96201.062128 to 96201.062127 matches the refund/round-up correction. Comment updated accordingly — good.
contracts/swap/Cargo.toml (1)
5-5: Version bump to 1.1.1 acknowledged.Ensure you tag the release and update any downstream deployment scripts referencing the artifact name change (underscore) to keep CI/CD green.
contracts/swap/src/queries.rs (2)
174-177: Simulation margin now adds full user input (incl. fees) — correct and consistent with target-path logicSwitching from adding
available_swap_quote_fundstoinput_quote_quantityin simulation makes the margin check reflect the actual funds the user will provide with the tx (principal + fees). This prevents false “Swap amount too high” errors when fees are non-zero and aligns withestimate_execution_buy_from_targetwhich already usesfunds_in_contract + required_input_quote_quantity. Looks good.
235-238: Symmetry check — goodThe target-path simulation already adds
required_input_quote_quantityto contract funds, which is the full amount the user must provide. Your source-path change restores symmetry between the two paths. No action needed.
| set_contract_version(deps.storage, CONTRACT_NAME.to_string(), CONTRACT_VERSION)?; | ||
| } |
There was a problem hiding this comment.
Compile error: passing String to set_contract_version.
set_contract_version expects &str params; CONTRACT_NAME.to_string() passes String by value and won’t compile. Use the &'static str directly.
Apply:
- set_contract_version(deps.storage, CONTRACT_NAME.to_string(), CONTRACT_VERSION)?;
+ set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?;📝 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.
| set_contract_version(deps.storage, CONTRACT_NAME.to_string(), CONTRACT_VERSION)?; | |
| } | |
| set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?; | |
| } |
🤖 Prompt for AI Agents
In contracts/swap/src/contract.rs around lines 124-125, the call to
set_contract_version passes CONTRACT_NAME.to_string() (a String) but
set_contract_version expects &str; remove the .to_string() and pass the string
constants directly (e.g., CONTRACT_NAME and CONTRACT_VERSION as &str / &'static
str) so the function receives &str parameters.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores