Skip to content

fix(stableswap)!: fix peg ordering by accepting and co-sorting asset-peg pairs#1424

Open
khuzama98 wants to merge 9 commits into
masterfrom
fix/stableswap-peg-ordering
Open

fix(stableswap)!: fix peg ordering by accepting and co-sorting asset-peg pairs#1424
khuzama98 wants to merge 9 commits into
masterfrom
fix/stableswap-peg-ordering

Conversation

@khuzama98
Copy link
Copy Markdown
Contributor

Description

Fixes a peg-ordering bug in create_pool_with_pegs where passing assets and peg
sources in different orders caused silent misalignment after internal sorting.

Root cause: do_create_pool sorted assets internally, but the corresponding
peg_source array was not co-sorted — so peg_source[i] could end up paired with
the wrong asset after sort.

Changes:

  • Breaking: create_pool_with_pegs now accepts BoundedVec<(AssetId, PegSource), _>
    instead of separate assets and peg_source parameters. Pairing is now explicit at
    the call site, making misalignment impossible.
  • Assets and peg sources are co-sorted by asset ID before pool creation; do_create_pool
    no longer sorts internally (callers are now responsible for providing sorted assets).
  • create_pool (non-peg variant) sorts assets at dispatch.
  • Added debug_assert! in get_target_pegs verifying assets arrive pre-sorted.

Related Issue

Fixes: #1164

Motivation and Context

A pool created via create_pool_with_pegs with assets provided in non-ascending order
would store a peg source at an index that no longer matched its intended asset after the
internal sort. This caused incorrect peg values to be applied to assets, a correctness
issue with direct impact on pool pricing.

How Has This Been Tested?

  • added peg-ordering unit tests to 3-asset pools with generic test helpers
    (pallets/stableswap/src/tests/peg_ordering.rs)
  • Updated all affected unit test suites: peg.rs, peg_one.rs, peg_ordering.rs,
    pegs_with_different_decimals.rs, update_peg_source.rs, update_max_peg_update.rs,
    remove_liquidity.rs
  • Added integration regression test in integration-tests/src/stableswap.rs that
    creates a pool with out-of-order assets and asserts correct peg assignment post-sort

Checklist:

  • I have updated the documentation if necessary.
  • I have added tests to cover my changes, regression test if fixing an issue.
  • This is a breaking change.

@github-actions
Copy link
Copy Markdown

Crate versions that have been updated:

  • runtime-integration-tests: v1.78.0 -> v1.78.1
  • pallet-hsm: v1.7.0 -> v1.7.1
  • pallet-stableswap: v7.3.0 -> v8.0.0
  • hydradx-runtime: v410.0.0 -> v411.0.0

Runtime version has been increased.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a correctness bug in the Stableswap pallet where assets were internally sorted during pool creation but peg sources were not co-sorted, causing silent peg/asset misalignment and incorrect pricing.

Changes:

  • Breaking API change: create_pool_with_pegs now takes explicit (asset_id, peg_source) pairs and co-sorts them by asset_id before storage.
  • Moves/clarifies sorting responsibility: create_pool sorts assets at dispatch; do_create_pool no longer sorts internally and expects pre-sorted assets.
  • Adds/updates extensive unit + integration regression coverage for peg ordering, and bumps versions accordingly.

Reviewed changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
runtime/hydradx/src/lib.rs Bumps runtime spec_version to reflect the breaking change.
runtime/hydradx/Cargo.toml Bumps runtime crate version.
pallets/stableswap/src/lib.rs Changes peg-pool creation API to asset/peg pairs, co-sorts pairs, removes internal sorting in do_create_pool, and adds a sorted-assets debug assertion for peg resolution.
pallets/stableswap/src/benchmarks.rs Updates benchmarks to call create_pool_with_pegs using (asset, peg) pairs.
pallets/stableswap/README.md Updates documentation to describe (asset_id, peg_source) pairing and sorting semantics.
pallets/stableswap/Cargo.toml Major version bump for the breaking pallet API.
pallets/stableswap/src/tests/mod.rs Registers new peg_ordering test module.
pallets/stableswap/src/tests/mock.rs Updates test ExtBuilder to construct (asset, peg) pairs when creating peg pools.
pallets/stableswap/src/tests/peg.rs Updates tests to the new peg-pool creation signature.
pallets/stableswap/src/tests/peg_one.rs Updates tests to the new peg-pool creation signature.
pallets/stableswap/src/tests/peg_ordering.rs Adds dedicated regression tests ensuring peg sources are co-sorted with assets and pricing behavior matches sorted-reference pools.
pallets/stableswap/src/tests/pegs_with_different_decimals.rs Updates tests to the new peg-pool creation signature.
pallets/stableswap/src/tests/remove_liquidity.rs Updates tests to the new peg-pool creation signature.
pallets/stableswap/src/tests/update_peg_source.rs Updates tests to the new peg-pool creation signature.
pallets/stableswap/src/tests/update_max_peg_update.rs Updates tests to the new peg-pool creation signature.
pallets/hsm/src/tests/mock.rs Updates HSM test ExtBuilder to call create_pool_with_pegs using (asset, peg) pairs.
pallets/hsm/src/benchmarks.rs Updates HSM benchmarks to call create_pool_with_pegs using (asset, peg) pairs.
pallets/hsm/Cargo.toml Patch bump reflecting dependent breaking change adaptation.
integration-tests/src/stableswap.rs Updates existing peg-pool creation usage and adds an integration regression test for reversed-order asset inputs.
integration-tests/src/omnipool_liquidity_mining.rs Updates peg-pool creation usage to the new signature.
integration-tests/src/hsm.rs Updates peg-pool creation usage to the new signature.
integration-tests/Cargo.toml Patch bump for integration tests crate.
Cargo.lock Updates lockfile versions for the bumped crates.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pallets/stableswap/src/tests/peg_ordering.rs Outdated
peg_sources.len(),
"Pool assets and peg sources must have the same length"
);
debug_assert!(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next to debug assert, shall we make it prod runtime failure too?

just like we did recently for trades #1429

@@ -0,0 +1,626 @@
// This file is part of HydraDX.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name of this is off, it must be with underscore so pallet_stableswap.rs

so this should not be a new file, rather a modification of our existing weight files.

We possibly have a bug in the benchmark workflow file, could you please check it out that too and fix? thanks

/// - `ShareAssetInPoolAssets`: If the share asset is among the pool assets.
/// - `AssetNotRegistered`: If one or more assets are not registered in the AssetRegistry.
/// - `InvalidAmplification`: If the amplification parameter is invalid.
/// - `IncorrectInitialPegs`: If the initial pegs are incorrect.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is hte reason this was removed? i still see IncorrectInitialPegs error returned when creating pool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stableswap asset order dependency issue

3 participants