Conversation
|
Crate versions that have been updated:
Runtime version has not been increased. |
There was a problem hiding this comment.
Pull request overview
Adds a Curve-vs-Hydration stableswap math parity test suite by deploying a Vyper contract containing Curve’s StableSwap math onto the local EVM (Frontier) and comparing results across a broad set of scenarios, plus some looping “value extraction” checks.
Changes:
- Add a pure Vyper “CurveStableSwapMath” contract exposing Curve StableSwap invariant/swap/liquidity math for testing.
- Add Rust integration tests that deploy/call the Vyper contract and compare outputs against
hydra_dx_math::stableswap. - Wire the new integration test module into the integration test crate.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| scripts/test-contracts/vyper/CurveStableSwapMath.vy | New pure Vyper contract exposing Curve StableSwap math as callable functions for parity testing. |
| scripts/test-contracts/vyper/CurveStableSwapMath.bin | Compiled bytecode blob included for deployment from Rust tests. |
| integration-tests/src/stableswap_curve_comparison.rs | New integration tests deploying the Vyper contract and asserting parity/tolerances vs Hydration math + looping checks. |
| integration-tests/src/lib.rs | Registers the new test module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| N_COINS: uint256 = len(xp) | ||
| S: uint256 = 0 | ||
| for x: uint256 in xp: | ||
| S += x | ||
| if S == 0: | ||
| return 0 | ||
|
|
||
| Dprev: uint256 = 0 | ||
| D: uint256 = S | ||
| Ann: uint256 = amp * N_COINS | ||
| for _i: uint256 in range(255): | ||
| D_P: uint256 = D | ||
| for x: uint256 in xp: | ||
| D_P = D_P * D // (x * N_COINS) | ||
| Dprev = D | ||
| D = (Ann * S + D_P * N_COINS) * D // ((Ann - 1) * D + (N_COINS + 1) * D_P) |
There was a problem hiding this comment.
_get_D (and downstream _get_y / _get_y_D) can divide by zero when any element of xp is 0 (D_P * D // (x * N_COINS)). Since this contract exposes the math externally, it should explicitly validate inputs (e.g., assert N_COINS > 1 and assert x > 0 for all balances) or define a safe behavior for zero balances to avoid runtime reverts that are hard to diagnose.
| D2: uint256 = D1 | ||
| if token_supply > 0 and fee > 0: | ||
| _fee: uint256 = fee * N_COINS // (4 * (N_COINS - 1)) | ||
| adjusted: DynArray[uint256, 8] = [] | ||
| for i: uint256 in range(8): | ||
| if i >= N_COINS: | ||
| break | ||
| ideal_balance: uint256 = D1 * old_balances[i] // D0 | ||
| difference: uint256 = 0 | ||
| if ideal_balance > new_balances[i]: | ||
| difference = ideal_balance - new_balances[i] | ||
| else: | ||
| difference = new_balances[i] - ideal_balance | ||
| adjusted.append(new_balances[i] - (_fee * difference // FEE_DENOMINATOR)) | ||
| D2 = self._get_D(adjusted, amp) |
There was a problem hiding this comment.
Fee normalization uses fee * N_COINS // (4 * (N_COINS - 1)), which will revert for N_COINS == 1 (division by zero). Consider asserting N_COINS > 1 in calc_token_amount / calc_withdraw_one_coin (or guarding the fee path) so invalid pool sizes fail with a clear error.
| D0: uint256 = self._get_D(balances, amp) | ||
| D1: uint256 = D0 - token_amount * D0 // total_supply | ||
|
|
||
| new_y: uint256 = self._get_y_D(amp, i, balances, D1) | ||
| dy_0: uint256 = balances[i] - new_y | ||
|
|
||
| xp_reduced: DynArray[uint256, 8] = [] | ||
| for j: uint256 in range(8): | ||
| if j >= N_COINS: | ||
| break | ||
| dx_expected: uint256 = 0 | ||
| if j == i: | ||
| dx_expected = balances[j] * D1 // D0 - new_y | ||
| else: | ||
| dx_expected = balances[j] - balances[j] * D1 // D0 | ||
| xp_reduced.append(balances[j] - (_fee * dx_expected // FEE_DENOMINATOR)) | ||
|
|
||
| dy: uint256 = xp_reduced[i] - self._get_y_D(amp, i, xp_reduced, D1) | ||
| dy = dy - 1 # Withdraw less to account for rounding errors | ||
|
|
There was a problem hiding this comment.
calc_withdraw_one_coin can revert for valid-looking inputs due to missing guards:
total_supplycan be 0 (division by zero)token_amountcan exceedtotal_supply(underflow in D1)- if
token_amount * D0 // total_supply == 0, thenD1 == D0and laterdy = dy - 1will underflow.
Add explicit assertions (e.g.,assert total_supply > 0,assert token_amount <= total_supply,assert D1 < D0or early-return (0,0) whenD1 == D0) to avoid these reverts.
| Curve's get_dy — calculate swap output amount. | ||
| Returns the amount of coin j received for dx of coin i, after fee. | ||
| fee is in units of FEE_DENOMINATOR (10^10). Pass 0 to disable fees. | ||
| """ |
There was a problem hiding this comment.
get_dy lacks basic input validation (i != j, i/j < len(balances), dx > 0). Since it indexes balances[i]/balances[j] before any checks, invalid inputs will revert with an out-of-bounds error rather than a clear assertion. Consider adding the same index assertions used in _get_y plus assert dx > 0 to make failures explicit and consistent.
| """ | |
| """ | |
| N_COINS: uint256 = len(balances) | |
| assert i < N_COINS | |
| assert j < N_COINS | |
| assert i != j | |
| assert dx > 0 |
| fn assert_parity(label: &str, hydra: u128, curve: u128, max_tolerance: u128, expect_hydra_gte: bool) { | ||
| let diff = hydra.abs_diff(curve); | ||
| assert!( | ||
| diff <= max_tolerance, | ||
| "{}: diff {} exceeds tolerance {} (hydra={}, curve={})", | ||
| label, | ||
| diff, | ||
| max_tolerance, | ||
| hydra, | ||
| curve | ||
| ); | ||
| if expect_hydra_gte { | ||
| assert!( | ||
| hydra >= curve, | ||
| "{}: expected hydra ({}) >= curve ({})", | ||
| label, | ||
| hydra, | ||
| curve |
There was a problem hiding this comment.
assert_parity is defined as (hydra, curve, ...), but some callers pass (curve_out, hydra_out, ...) / (curve_shares, hydra_shares, ...). This makes assertion output misleading (it prints hydra= with the Curve value) and makes the expect_hydra_gte direction check easy to misuse. Consider standardizing call sites to always pass (hydra, curve) (or rename parameters / provide a dedicated helper for the “curve >= hydra” case).
| fn assert_parity(label: &str, hydra: u128, curve: u128, max_tolerance: u128, expect_hydra_gte: bool) { | |
| let diff = hydra.abs_diff(curve); | |
| assert!( | |
| diff <= max_tolerance, | |
| "{}: diff {} exceeds tolerance {} (hydra={}, curve={})", | |
| label, | |
| diff, | |
| max_tolerance, | |
| hydra, | |
| curve | |
| ); | |
| if expect_hydra_gte { | |
| assert!( | |
| hydra >= curve, | |
| "{}: expected hydra ({}) >= curve ({})", | |
| label, | |
| hydra, | |
| curve | |
| fn assert_parity( | |
| label: &str, | |
| first: u128, | |
| second: u128, | |
| max_tolerance: u128, | |
| expect_first_gte_second: bool, | |
| ) { | |
| let diff = first.abs_diff(second); | |
| assert!( | |
| diff <= max_tolerance, | |
| "{}: diff {} exceeds tolerance {} (first={}, second={})", | |
| label, | |
| diff, | |
| max_tolerance, | |
| first, | |
| second | |
| ); | |
| if expect_first_gte_second { | |
| assert!( | |
| first >= second, | |
| "{}: expected first ({}) >= second ({})", | |
| label, | |
| first, | |
| second |
| // Hydration should give slightly less due to +2 bias and -1 rounding | ||
| assert_parity( | ||
| &format!("{} swap {}% no fee", label, 100 / dx_fraction), | ||
| curve_out, | ||
| hydra_out, | ||
| MAX_SWAP_TOLERANCE, | ||
| true, // curve_out >= hydra_out | ||
| ); |
There was a problem hiding this comment.
The swap label uses 100 / dx_fraction as a percent. For dx_fraction = 10000 (0.01%) this formats as 0%, which makes failures harder to interpret. Consider formatting the swap size in basis points or as a fraction (e.g., dx_fraction itself) so the label matches the actual test case.
| use primitives::{AccountId, EvmAddress}; | ||
| use sp_core::U256; | ||
| use sp_runtime::Permill; | ||
| use xcm_emulator::{Network, TestExt}; |
There was a problem hiding this comment.
Network is imported but never used (use xcm_emulator::{Network, TestExt};). This will produce an unused-import warning (and could fail CI if warnings are denied). Consider removing Network from the import list.
| use xcm_emulator::{Network, TestExt}; | |
| use xcm_emulator::TestExt; |
|
Quick benchmark at commit f181c17 has been executed successfully. |
No description provided.