Re-add Swap::AlphaSqrtPrice for backwards compatibility; bump spec to…#2799
Re-add Swap::AlphaSqrtPrice for backwards compatibility; bump spec to…#2799sam0x17 wants to merge 3 commits into
Conversation
… 424 The balancer migration (v3.4.8-423) deleted the Swap::AlphaSqrtPrice storage map as collateral of the Uniswap-V3 -> balancer engine rewrite, where price became a derived quantity instead of stored state. External consumers (indexers, dashboards, wallets, SDKs) read that map directly to obtain the subnet price and broke. Re-add the storage map with its exact prior definition and maintain it as a read-only mirror of the derived balancer price (sqrt of current_price), refreshed on every price change (init, swap, protocol-liquidity adjust) and removed on dissolve. The migration now retains the V3 values as the initial seed instead of clearing them, so there is no gap at upgrade. No V3 engine logic is reintroduced; the map is purely for back-compat.
🚨🚨🚨 HOTFIX DETECTED 🚨🚨🚨It looks like you are trying to merge a hotfix PR into If you are trying to merge a hotfix PR, please complete the following essential steps:
If you do not complete these steps, your hotfix may be inadvertently removed in the future when branches are promoted to |
github-actions
Bot
added
the
hotfix
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let epsilon = | ||
| U64F64::saturating_from_num(1).safe_div(U64F64::saturating_from_num(1_000_000_000_u64)); | ||
| let sqrt_price = price.checked_sqrt(epsilon).unwrap_or_default(); | ||
| AlphaSqrtPrice::<T>::insert(netuid, sqrt_price); |
There was a problem hiding this comment.
[MEDIUM] New price-mirror storage write is not reflected in weights
refresh_alpha_sqrt_price performs a persistent storage write and is now called from hot paths including adjust_protocol_liquidity and swap_inner, but this PR does not update the affected runtime weights. Those paths are reachable from per-block emission handling and user-facing swap/stake extrinsics, so blocks can now do at least one extra storage write, plus the helper's price reads, beyond what the declared weights account for. Regenerate or manually update the affected weights and block-emission accounting before merging.
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE BASELINE scrutiny: author is a repo admin with long merged history; branch targets main but has a hotfix label and explicit hotfix rationale. No Findings
Prior-comment reconciliation
ConclusionVULNERABLE because the PR still adds a storage-mutating fixed-point price mirror on swap/emission paths without updating the runtime weight accounting. I found no malicious indicators. 📜 Previous run (superseded)
# 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
The swap-path refresh previously derived the price from reserves inside swap_inner, but a swap's reserve deltas are applied by the caller (stake_utils), outside this pallet — so it read pre-swap reserves and mirrored a stale price. Surface the swap step's computed final price and store that instead, which is the correct post-swap price without needing committed reserves. Init and emission (adjust_protocol_liquidity) still derive from current_price, where reserves are already up to date. Add a dedicated test covering init seeding, post-swap tracking, simulated-swap rollback, and clear-on-dissolve.
| pub(crate) fn store_alpha_sqrt_price(netuid: NetUid, price: U64F64) { | ||
| // Epsilon for the bisection sqrt: 1e-9 is well below the price precision consumers need. | ||
| let epsilon = | ||
| U64F64::saturating_from_num(1).safe_div(U64F64::saturating_from_num(1_000_000_000_u64)); | ||
| let sqrt_price = price.checked_sqrt(epsilon).unwrap_or_default(); | ||
| AlphaSqrtPrice::<T>::insert(netuid, sqrt_price); |
There was a problem hiding this comment.
[MEDIUM] New price-mirror storage write is not reflected in weights
store_alpha_sqrt_price now performs a fixed-point sqrt and writes Swap::AlphaSqrtPrice from initialization, adjust_protocol_liquidity, and the real/simulated swap path, but this PR does not update the benchmarked weights for the Subtensor extrinsics and block paths that call SwapInterface::swap or protocol-liquidity adjustment. That undercharges repeated swaps/emission for both CPU and an additional storage write, which is a runtime resource-accounting issue. Rerun/update the affected benchmarks or otherwise add the extra DB write and computation cost to the relevant weights before merging.
|
🔄 AI review updated — Skeptic: VULNERABLE |
1 participant
Description
The balancer migration shipped in
v3.4.8-423deleted theSwap::AlphaSqrtPricestorage map. It was removed as collateral of the Uniswap-V3 → balancer engine rewrite: under V3,AlphaSqrtPricewas the primary price state variable, but under the balancer price became a derived quantity (computed on the fly from reserves + weights incurrent_price), so the stored variable was dropped along with the rest of the V3 machinery (ticks, positions, liquidity, fee-growth accumulators).That map is a de-facto public interface: external consumers (indexers, dashboards, wallets, SDKs) read
Swap.AlphaSqrtPricedirectly viastate_getStorageto obtain the subnet price, exactly as they did under V3. Removing it without a deprecation path broke them.This PR re-adds it as a backwards-compatibility shim:
StorageMap<_, Twox64Concat, NetUid, U64F64, ValueQuery>), so consumers get the same key/hasher/value/units as before.sqrt(current_price)), refreshed at every price-change point: subnet init, swap, and protocol-liquidity adjustment; removed on dissolve.migrate_swapv3_to_balancermigration now retains the old V3 values as the initial seed (theremove_prefix("Swap", "AlphaSqrtPrice", …)line is dropped) instead of clearing them, so there is no gap at upgrade.No V3 engine logic is reintroduced — the map is purely a price mirror for back-compat. The other removed V3 maps (
Ticks,Positions,CurrentTick,CurrentLiquidity,TickIndexBitmapWords,FeeGlobalTao/Alpha,LastPositionId,EnabledUserLiquidity,SwapV3Initialized) are intentionally left removed — they have no balancer equivalent and restoring them would mean fabricating meaningless values.Also bumps
spec_version423 → 424.Related Issue(s)
N/A — addresses downstream reports of
Swap.AlphaSqrtPricereads returning empty after the balancer upgrade.Type of Change
Breaking Change
None. This restores a previously-broken interface and adds no new constraints. The storage layout is identical to
v3.4.7-422.Notes for reviewers
Why storage and not just the runtime API? A supported price API already exists —
current_alpha_price(netuid)(sincev3.1.6) andcurrent_alpha_price_all()(sincev3.3.10-380). Consumers should migrate to those, and the restored map is documented as back-compat-only. But the convenient bulk_allvariant is only a few months old, so a lot of existing tooling still reads raw storage; this shim unbreaks them now. If the map is ever dropped again it should go through an explicit deprecation notice rather than a silent removal.State bloat? Bounded and self-cleaning: one
U64F64(16 bytes) per subnet, capped bySubnetLimit(default 128) → ~2 KB of values. Same cardinality as existing per-subnet maps written every block (SubnetMovingPrice,RootProp). Created on init, removed on dissolve — does not grow with swaps, accounts, or time. None of the genuinely bloaty V3 structures are reintroduced.Consistency:
refresh_alpha_sqrt_priceruns inside the swap's transactional scope, so it is correctly rolled back on simulated swaps. For emitting subnets it is refreshed every block viaadjust_protocol_liquidity; for idle subnets the value stays correct because price only moves via swaps/emission.Checklist
cargo fmt+cargo clippy --all-targets --all-featuresrun clean on the affected crate (no new warnings)Testing
cargo test -p pallet-subtensor-swap --lib— 44 passed. The twomigrate_swapv3_to_balancertests were updated to assertAlphaSqrtPriceis retained (seeded) across the migration rather than cleared.SKIP_WASM_BUILD=1 cargo check -p node-subtensor-runtime(clean). The WASM build itself is left to CI (local clang here can't targetwasm32for thezstd-sysC dependency — an environment issue, not a code issue).