fix: harden weighted-average rounding and support custody - #24
Conversation
- bump the Injective stack to 1.20.0 and Rust to 1.86.0 - update release builds to CosmWasm optimizer 0.17.0 - add TestTube migration coverage for both mainnet deployments - preserve FPDecimal coin conversion semantics under CosmWasm 3
📝 WalkthroughWalkthroughThe release updates Rust and dependency tooling, adds migrations from two legacy contract versions, centralizes exact swap arithmetic, and validates support-fund preservation and integer output amounts. Tests cover migrations, arithmetic, custody, conversions, and a rounding regression. ChangesSwap contract release
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SwapContract
participant BankModule
participant InjectiveSubaccount
participant OrderReply
participant SwapRecipient
SwapContract->>BankModule: query bank balance
SwapContract->>InjectiveSubaccount: query available deposit
SwapContract->>OrderReply: place atomic order
OrderReply-->>SwapContract: return order result
SwapContract->>BankModule: re-query spendable funds
SwapContract->>SwapRecipient: send floored output coin
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
contracts/swap/src/swap.rs (1)
292-310: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
Coininstead of using the fully qualified path.The logic in both helpers is correct.
ensure_minimum_outputspells the type ascosmwasm_std::Coinwhile line 10 already imports othercosmwasm_stditems. AddCointo that import list for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/swap/src/swap.rs` around lines 292 - 310, Update the existing cosmwasm_std import list to include Coin, then change ensure_minimum_output’s parameter type to use the imported Coin directly instead of cosmwasm_std::Coin; leave both helper implementations unchanged.contracts/swap/src/exact_math.rs (1)
126-134: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider rejecting a zero rounded ratio explicitly.
ratio_to_tickcan returnFPDecimal::ZEROwhentick_counttruncates to zero in theDowndirection.weighted_average_to_tickguards this case at line 112, butratio_to_tickdoes not.estimate_execution_buy_from_sourceuses theDownresult asresult_quantity, so a dust input produces a zero-quantity atomic order and an opaque exchange rejection. An explicit error keeps the failure diagnosable.♻️ Proposed guard
let rounded_raw = tick_count .checked_mul(U512::from(tick.num)) .ok_or_else(|| arithmetic_error("rounded ratio overflow"))?; + if rounded_raw.is_zero() { + return Err(arithmetic_error("rounded ratio must be positive")); + } + Ok(FPDecimal::from(to_u256(rounded_raw)?))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/swap/src/exact_math.rs` around lines 126 - 134, Update ratio_to_tick around the tick_count and rounded_raw calculation to reject a zero rounded ratio before returning FPDecimal::from(...). Match the existing zero-result validation used by weighted_average_to_tick, and return a descriptive arithmetic error so Down-direction dust inputs cannot produce a zero-quantity order.contracts/swap/src/queries.rs (1)
146-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the raw tick constant.
FPDecimal::from(primitive_types::U256::one())appears here and again at line 331 inestimate_execution_sell_from_target. Both express "one raw fixed-point unit". A named helper or constant inexact_mathmakes the intent explicit and prevents the two sites from diverging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/swap/src/queries.rs` around lines 146 - 148, Extract the repeated one-raw-fixed-point-unit value into a named helper or constant in exact_math, then replace the FPDecimal::from(primitive_types::U256::one()) usages in the current query flow and estimate_execution_sell_from_target with it. Preserve the existing FPDecimal value and calculation behavior.contracts/swap/src/testing/migration_test.rs (1)
60-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the missing-config migration failure path.
assert_storage_migrationalways savesCONFIGbefore callingmigrate, so it never exercises theget_config(deps.storage)?validation branch incontracts/swap/src/contract.rs(Line 134). Add a test that sets a supported legacycw2::ContractVersionwithout savingCONFIG, and asserts thatmigratereturns an error. This closes a coverage gap on a validation path meant to guard a mainnet migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/swap/src/testing/migration_test.rs` around lines 60 - 91, Add a migration test alongside assert_storage_migration that stores a supported legacy cw2::ContractVersion without saving CONFIG, invokes migrate, and asserts it returns an error. Keep the existing successful migration helper unchanged and target the get_config validation path in migrate.contracts/swap/src/contract.rs (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigration-source constants are duplicated across production and test code. The legacy contract name/version literals are declared once in
contracts/swap/src/contract.rsand re-declared independently incontracts/swap/src/testing/migration_test.rs. A future version bump in one place will not propagate to the other, letting tests validate a stale version pairing without failing.
contracts/swap/src/contract.rs#L20-L24: changeCODE_67_LEGACY_CONTRACT_NAME,CODE_67_LEGACY_CONTRACT_VERSION,CODE_1962_CONTRACT_NAME, andCODE_1962_CONTRACT_VERSIONtopub(crate).contracts/swap/src/testing/migration_test.rs#L23-L26: remove the locally duplicatedCODE_67_CW2_NAME,CODE_67_CW2_VERSION,CODE_1962_CW2_NAME,CODE_1962_CW2_VERSIONconstants and import thepub(crate)constants fromcontract.rsinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/swap/src/contract.rs` around lines 20 - 24, The migration contract constants are duplicated between production and test code; make the four constants in contracts/swap/src/contract.rs#L20-L24 pub(crate), then remove the duplicate constants in contracts/swap/src/testing/migration_test.rs#L23-L26 and import and reuse the production constants in the migration tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/rust.yaml:
- Around line 32-36: Update the Rust CI workflow by replacing the archived
actions-rs/toolchain@v1 step with dtolnay/rust-toolchain, preserving the minimal
profile and Rust 1.86.0 toolchain settings. Replace every actions-rs/cargo@v1
usage in the workflow with equivalent direct cargo commands while retaining the
existing CI operations and arguments.
In `@contracts/swap/src/state.rs`:
- Line 29: Update the error message in the visible map_err closure to use a
single negation, such as indicating that no swap route was found between
source_denom and target_denom. Before changing it, check tests for assertions
against the current message and update only those expected strings as needed.
---
Nitpick comments:
In `@contracts/swap/src/contract.rs`:
- Around line 20-24: The migration contract constants are duplicated between
production and test code; make the four constants in
contracts/swap/src/contract.rs#L20-L24 pub(crate), then remove the duplicate
constants in contracts/swap/src/testing/migration_test.rs#L23-L26 and import and
reuse the production constants in the migration tests.
In `@contracts/swap/src/exact_math.rs`:
- Around line 126-134: Update ratio_to_tick around the tick_count and
rounded_raw calculation to reject a zero rounded ratio before returning
FPDecimal::from(...). Match the existing zero-result validation used by
weighted_average_to_tick, and return a descriptive arithmetic error so
Down-direction dust inputs cannot produce a zero-quantity order.
In `@contracts/swap/src/queries.rs`:
- Around line 146-148: Extract the repeated one-raw-fixed-point-unit value into
a named helper or constant in exact_math, then replace the
FPDecimal::from(primitive_types::U256::one()) usages in the current query flow
and estimate_execution_sell_from_target with it. Preserve the existing FPDecimal
value and calculation behavior.
In `@contracts/swap/src/swap.rs`:
- Around line 292-310: Update the existing cosmwasm_std import list to include
Coin, then change ensure_minimum_output’s parameter type to use the imported
Coin directly instead of cosmwasm_std::Coin; leave both helper implementations
unchanged.
In `@contracts/swap/src/testing/migration_test.rs`:
- Around line 60-91: Add a migration test alongside assert_storage_migration
that stores a supported legacy cw2::ContractVersion without saving CONFIG,
invokes migrate, and asserts it returns an error. Keep the existing successful
migration helper unchanged and target the get_config validation path in migrate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cc14ff0-944c-470e-bff7-17495c22beda
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcontracts/swap/src/testing/test_artifacts/mainnet_code_1962.wasmis excluded by!**/*.wasmcontracts/swap/src/testing/test_artifacts/mainnet_code_67.wasmis excluded by!**/*.wasm
📒 Files selected for processing (22)
.github/workflows/rust.yamlCargo.tomlChangelog.mdbuild_release.shcontracts/swap/Cargo.tomlcontracts/swap/src/contract.rscontracts/swap/src/error.rscontracts/swap/src/exact_math.rscontracts/swap/src/helpers.rscontracts/swap/src/lib.rscontracts/swap/src/queries.rscontracts/swap/src/state.rscontracts/swap/src/swap.rscontracts/swap/src/testing/custody_tests.rscontracts/swap/src/testing/exact_math_tests.rscontracts/swap/src/testing/migration_test.rscontracts/swap/src/testing/mod.rscontracts/swap/src/testing/test_utils.rscontracts/swap/src/testing/types_tests.rscontracts/swap/src/testing/weighted_average_rounding_regression.rscontracts/swap/src/types.rsrust-toolchain.toml
| - name: Install Rust toolchain | ||
| uses: actions-rs/toolchain@v1 | ||
| with: | ||
| profile: minimal | ||
| toolchain: 1.78.0 | ||
| toolchain: 1.86.0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
if rg -n 'actions-rs/(toolchain|cargo)`@v1`' .github/workflows; then
echo "Archived actions remain" >&2
exit 1
fiRepository: InjectiveLabs/swap-contract
Length of output: 157
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- workflow files ---'
git ls-files '.github/workflows/*'
printf '%s\n' '--- actions-rs references ---'
rg -n -C 3 'actions-rs/(toolchain|cargo)`@v1`' .github/workflows || true
printf '%s\n' '--- workflow sections ---'
sed -n '1,145p' .github/workflows/rust.yamlRepository: InjectiveLabs/swap-contract
Length of output: 157
🌐 Web query:
actions-rs/toolchain GitHub archived actions-rs/cargo maintenance status
💡 Result:
The entire actions-rs organization, including the actions-rs/toolchain and actions-rs/cargo repositories, has been archived by the owner [1][2][3][4]. As of October 13, 2023, these repositories are in a read-only state and are no longer maintained [5][6]. The organization was archived due to the primary maintainer stepping back, and the community has widely recognized these actions as unmaintained [3][4][7][8]. Users are advised to migrate to alternative, actively maintained solutions [3]. For example, dtolnay/rust-toolchain is a common replacement for actions-rs/toolchain, while standard cargo commands are often used in place of actions-rs/cargo [3].
Citations:
- 1: https://github.com/actions-rs/toolchain
- 2: https://github.com/actions-rs
- 3: [ci] Replace unmaintained actions-rs GH actions tauri-apps/tauri#8078
- 4: Agenda Item: UNOfficial Github Actions @actions-rs for Rust Community has been Archived rust-lang/infra-team#96
- 5: Project unmaintained actions-rs/toolchain#216
- 6: https://github.com/actions-rs/toolchain/releases
- 7: https://users.rust-lang.org/t/github-actions-rs-maintenance/85264
- 8: Are there maintainers? actions-rs/meta#43
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- file metadata ---'
stat .github/workflows/rust.yaml 2>&1 || true
printf '%s\n' '--- requested workflow ranges ---'
for range in '1,75' '90,135'; do
printf '%s\n' "--- lines $range ---"
sed -n "${range}p" .github/workflows/rust.yaml 2>&1 || true
done
printf '%s\n' '--- exact action references ---'
grep -nE 'actions-rs/(toolchain|cargo)`@v1`' .github/workflows/rust.yaml 2>&1 || trueRepository: InjectiveLabs/swap-contract
Length of output: 157
🌐 Web query:
GitHub Actions archived action still runs actions-rs toolchain Node runtime actionlint too old to run
💡 Result:
The actions-rs organization, including actions-rs/toolchain, was archived by its owner in October 2023 and is unmaintained [1][2][3]. Because these actions were authored using older Node.js runtimes (such as Node 12) that have long been deprecated by GitHub Actions, they trigger deprecation warnings in CI workflows [1][4][5]. While these archived actions may still execute in some environments, they are technically unsupported and incompatible with the current GitHub Actions runtime requirements [6][4]. As of mid-2026, GitHub Actions has largely transitioned to Node.js 24 [7][8]. Using unmaintained actions that rely on obsolete Node.js versions poses significant risks of pipeline failure as GitHub continues to remove support for older runtimes [7][8]. To resolve these issues and remove deprecation warnings, you should replace the deprecated actions with maintained alternatives [1][6]: 1. Replace actions-rs/toolchain with dtolnay/rust-toolchain, which is the industry-standard, maintained alternative [1][4][9]. 2. Replace other actions like actions-rs/cargo, actions-rs/clippy-check, and actions-rs/audit-check with direct shell commands (e.g., cargo build, cargo clippy) or community-supported forks [1][6]. The error mentioning actionlint being "too old" likely refers to the fact that your local or CI-based linter (actionlint) is flagging the use of these deprecated, archived actions or the underlying obsolete Node.js runtime configuration in your workflow files [1][4]. Updating your workflow syntax to use the recommended modern replacements will resolve these linting errors and ensure your pipelines remain compatible with current and future GitHub Actions environments [1][9].
Citations:
- 1: [ci] Replace unmaintained actions-rs GH actions tauri-apps/tauri#8078
- 2: Project unmaintained actions-rs/toolchain#216
- 3: https://github.com/actions-rs/toolchain
- 4: Replace unmaintained actions-rs/toolchain action in CI workflows wasm-bindgen/wasm-pack#1246
- 5: Node.JS 12 deprecation on GitHub Actions actions-rs/toolchain#219
- 6: redpanda-data/redpanda@d43fbae
- 7: https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/
- 8: Node 20 -> Node 24 migration feature flagging, opt-in and opt-out environment variables actions/runner#3948
- 9: Properly remove all deprecations actions-rs/toolchain#224
Replace the archived actions-rs actions. The actions-rs organization has been archived since October 2023. Replace actions-rs/toolchain@v1 with dtolnay/rust-toolchain and replace actions-rs/cargo@v1 with direct cargo commands to avoid unsupported CI dependencies.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 33-33: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 zizmor (1.28.0)
[warning] 33-33: action or reusable workflow from archived repository (archived-uses): repository is archived
(archived-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/rust.yaml around lines 32 - 36, Update the Rust CI
workflow by replacing the archived actions-rs/toolchain@v1 step with
dtolnay/rust-toolchain, preserving the minimal profile and Rust 1.86.0 toolchain
settings. Replace every actions-rs/cargo@v1 usage in the workflow with
equivalent direct cargo commands while retaining the existing CI operations and
arguments.
Source: Linters/SAST tools
| SWAP_ROUTES | ||
| .load(storage, key) | ||
| .map_err(|_| StdError::generic_err(format!("No swap route not found from {source_denom} to {target_denom}",))) | ||
| .map_err(|_| StdError::msg(format!("No swap route not found from {source_denom} to {target_denom}",))) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the double negative in the error message.
The message reads "No swap route not found from ... to ...". This states the opposite of the condition. Use a single negation.
🐛 Proposed fix
- .map_err(|_| StdError::msg(format!("No swap route not found from {source_denom} to {target_denom}",)))
+ .map_err(|_| StdError::msg(format!("No swap route found from {source_denom} to {target_denom}")))Check whether any test asserts on the current text before you change it.
📝 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.
| .map_err(|_| StdError::msg(format!("No swap route not found from {source_denom} to {target_denom}",))) | |
| .map_err(|_| StdError::msg(format!("No swap route found from {source_denom} to {target_denom}"))) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/swap/src/state.rs` at line 29, Update the error message in the
visible map_err closure to use a single negation, such as indicating that no
swap route was found between source_denom and target_denom. Before changing it,
check tests for assertions against the current message and update only those
expected strings as needed.
1 participant
Summary
Root cause
Fixed-point multiplication and division truncated the exact weighted-average remainder before upward tick rounding. A crafted ask book could therefore make a value just above a tick appear exactly aligned with the lower tick, understate the average price, and oversize an atomic buy. The estimator admitted the order against the contract's complete quote balance, allowing successful settlement to debit support that the caller did not provide.
Summary by CodeRabbit
New Features
Bug Fixes
Tooling
Documentation