[Contract] Add CPU-Heavy Benchmark Contract Hard Cap Guards - #722
Merged
EDOHWARES merged 3 commits intoAug 1, 2026
Conversation
Every root-level cargo command failed on main, so all four Rust CI jobs were red regardless of what a PR changed. The cause was a series of merge commits resolved by accepting both sides, which duplicated content inside several files. This repairs the workspace members and their dependencies. Cargo.lock was unparseable: 238 packages appeared two or three times and three entries had been spliced together, one package's header landing on another's body. It was also stale, still carrying the whole server dependency tree even though the workspace is three contract crates. Regenerated, which takes it from ~710 packages to 181. ed25519-dalek is pinned to 2.2.0 because 3.0 changed its CryptoRng bound and breaks soroban-env-host 22.x. emergency_guard had `#[contract]` applied twice, once bare and once via cfg_attr, so the macro emitted EmergencyGuardClient and friends twice. It also carried a second DefaultEmergencyGuard whose inherent methods collided with the trait impl, a duplicated is_admin_public, an is_admin that took &Address against a trait declaring Address, an unimported log macro, and three storage writes referencing an undefined `pause_state` with the correct `.set(...)` orphaned below an emit block. rotate_admin never bound `threshold` and pushed the new admin a second time after the loop had already substituted it. staking_rewards had two functions truncated mid-body with an unrelated signature spliced in, pause_staking and rotate_admin, which left the impl block unclosed. PauseType::CLAIM_REWARDS was referenced in four places but never defined. initialize called guard setup twice, and the second call failed with AlreadyInitialized, so every test that touched setup() died on error SoroLabs#1. PauseType was imported twice. Three emergency_guard tests asserted InsufficientSignatures where the implementation returns Unauthorized. All three supply a non-admin approver while meeting the count threshold, and their names describe unauthorized access, so the assertions were stale rather than the behaviour. Changing the contract's error semantics belongs to whoever owns that feature, so the tests were updated to match. In staking_rewards/test.rs the body of test_granular_claim_rewards_pause had been appended to test_granular_pause_staking, leaving one test headless and the other staking tokens it had never minted. Both are restored. Result: cargo check --locked --all-targets, cargo fmt --all --check, cargo clippy --locked --all-targets --all-features -D warnings, and cargo test --locked all pass. 54 tests, 0 failures. Not fixed here: CI also runs `cargo build -p liquidity_pool`, but that crate is not a workspace member and does not compile. Its lib.rs has been inflated from ~1150 to 2905 lines by the same accept-both-sides merges, including one statement with six duplicated transfer calls. Choosing which side is canonical in a contract that moves funds is a maintainer decision, so it is left alone and documented in the PR.
Issue SoroLabs#650. The pool only offered an exact-output swap, `swap(to, buy_a, out, in_max)`, where the caller fixes the output and caps the input. A trader who wants to sell a fixed amount had no entry point and had to guess `out`, so the one quantity they could not bound was the one an attacker moves: front-running the transaction leaves the same input returning far less. Adds `swap_exact_in(to, buy_a, amount_in, min_amount_out)`. It quotes the output from live reserves and the current fee, then refuses the trade with `SlippageExceeded` unless `amount_out >= min_amount_out`, before any tokens move. Returns the output actually delivered. `min_amount_out = 0` opts out. `swap` keeps its signature. Its output is fixed by the caller and delivered exactly or not at all, so a minimum-output parameter there would be satisfied by construction; `in_max` is already that direction's bound. Documented rather than duplicated. Also adds the read-only quotes callers need to pick a floor: `get_amount_out`, `get_amount_in` and `get_reserves`. Both swap directions now share `amount_out_for_in` / `amount_in_for_out` and a `settle_swap` helper, so the transfer-and-reserve-update path exists once. Division truncates toward the pool in both directions, and a test asserts the constant product never weakens. New error `InvalidAmount = 15` for a non-positive `amount_in` or a negative `min_amount_out`; a negative floor would silently disable the check. Existing codes are untouched. 11 tests cover the quoted-output path, both directions, a rejected floor leaving state untouched, a full front-run sandwich, the zero-minimum opt-out, invalid amounts, an input too small to fill, pause interaction, the fee's effect on a quote, and the quote round trip. Prerequisite repairs, without which none of the above could build or run: liquidity_pool was not a workspace member and did not compile, so CI's `cargo build -p liquidity_pool` had nothing to build. Its lib.rs had been inflated from ~1150 to 2905 lines by merges resolved by accepting both sides: the Error enum carried three overlapping copies with conflicting codes, PoolState and DataKey were each declared four times, and `withdraw` had six duplicated transfer calls. The last clean revision is 3e50a23, which holds the full public API and is the second parent of the merge that broke it, so lib.rs is restored from there and re-added to the workspace. That revision needed two fixes of its own: a duplicated `get_pause_mask`, and `remove_guard_admin` resolving to the trait's `remove_admin` instead of the inherent one. test.rs had the same damage in two blocks. `test_emergency_guard_trait_impl` called `init_guard` on an already-initialized guard and then made twelve assertions through direct `<LiquidityPool as EmergencyGuardTrait>` calls, which touch instance storage outside any contract frame; both forms are restored to the client-based assertions that work. `test_granular_pause_operations` called a two-argument `guard_unpause` that no version of the contract has. Four behaviour gaps the newer tests already asserted, all previously masked by the crate not compiling: - `set_paused` looped over four pause bits, calling `require_auth` on the same admin each time, which the host rejects as `Auth(ExistingValue)`. It now applies one combined mask, extended to cover STAKE and CLAIM_REWARDS so it pauses what its name claims. - `stake`, `unstake` and `claim_rewards` had no pause check at all. - Rewards never accrued. `update_reward_state` defaulted a missing `LastRewardLedger` to the current ledger and returned early without storing it, so `current <= last` held forever. The checkpoint is now seeded on the first staking action, and the accumulator is projected by a read-only helper so `get_pending_rewards` reports accrual without writing. - Removing or rotating the primary admin left `DataKey::Admin` pointing at an address that was no longer a guard admin. Both paths now hand the role to a surviving admin, and membership and threshold refusals report `Unauthorized`. Test snapshots are regenerated; they were stale from the mangled revision and still referenced its GuardAdmins/GuardThreshold storage keys. cargo fmt --all --check, cargo check --locked --all-targets, cargo clippy --locked --all-targets --all-features -D warnings, cargo build -p liquidity_pool --target wasm32-unknown-unknown --release and cargo test --locked all pass. 113 tests, 0 failures, 59 of them in liquidity_pool.
…et panics
The benchmark contract guarded its inputs with bare `panic!("...")` calls, so
an oversized argument reached the caller as an opaque `UnreachableCodeReached`
VM trap naming neither the offending argument nor the limit it broke. Replace
the panics with a `#[contracterror] enum Error` and `Result` returns so each
guard reports a distinct, decodable code.
The caps themselves were also wrong. Measured against the release WASM build
with the default network budget (100M CPU instructions / 40MB memory),
`bubble_sort` is quadratic in host `Vec::get`/`Vec::set` calls and exceeds the
budget outright at 150 elements (136.8M CPU) — so the advertised cap of 300
(573.4M CPU) was never reachable and hitting it produced exactly the budget
panic the guard was supposed to prevent. Lower `MAX_SORT` to 100, verified at
60.1M CPU / 24% memory. The remaining caps measure well inside budget and are
unchanged; all figures are recorded alongside the constants.
Also:
- Guard `nested_loop_burn` overflow: `saturating_mul` already prevented a
caller from wrapping `outer * inner` past the cap, now covered by a test.
- Fix `bubble_sort` underflow on `n - i - 1` for empty/single-element lists.
- Expose the caps as public constants and document them in a new README.
- Add cpu_heavy to the workspace so it builds and is covered by CI; it was
orphaned by the previous workspace repair and would not compile at all.
Tests cover each guard's boundary on both sides, the error discriminants, and
that the worst-case legal call stays inside the default budget.
|
@giftexceed Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Collaborator
|
Nice implementation, LGTM! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #646.
Important
This PR is stacked on #717 and is not cut from
main. Until #717 merges, the diff below shows 3 commits / ~296 files — two of which (fix(ci): repair workspace...andfeature(pool-slippage)...) belong to #717. GitHub will narrow this to my single commit automatically once #717 lands.The stack is not optional:
maincurrently does not build. ItsCargo.lockhas three truncated/spliced records (pin-project-lite,schemars,serde) so it fails to parse, andemergency_guard— a path dependency ofstaking_rewards— fails to compile with 112 errors. #717 carries the fix for both. Review only thefeature(cpu-heavy-guard)commit (eb7b946).The problem
Two distinct defects, only the first of which the issue names.
1. The guards panicked instead of returning an error. Every entry point guarded its inputs with a bare
panic!("input too large"). Confirmed against the release WASM build, that reaches the caller as:No argument name, no limit, nothing actionable.
2. The caps were set above the budget they were meant to protect.
bubble_sortis quadratic in host calls (Vec::get/Vec::set), so it dominates every other workload by orders of magnitude. Measured against the release WASM at the default network budget (100,000,000 CPU instructions / 41,943,040 memory bytes):bubble_sort(300)— the oldMAX_SORTbubble_sort(200)bubble_sort(150)bubble_sort(100)— newMAX_SORTSo the advertised cap of 300 was never reachable, and any call approaching it produced exactly the host budget panic the guard was supposed to prevent.
The change
panic!guards with a#[contracterror] enum ErrorandResultreturns. Over-cap calls now yieldError(Contract, #1..#5)instead of trapping. Follows the existing convention intwap_oracle.MAX_SORTfrom 300 to 100. The remaining caps measured well inside budget and are unchanged:fibonacci_iterative(50_000)count_primes(20_000)bubble_sort([100 elements])nested_loop_burn(500_000, 1)combined_benchmark(10_000, 50, 5_000)All figures are recorded alongside the constants in
lib.rsso the next person tightening a cap has the data.Fixed in passing
bubble_sort.for j in 0..n - i - 1underflows on the last pass and on empty lists. Nown.saturating_sub(i + 1), with a test for empty and single-element input.cpu_heavywas orphaned from the workspace and did not compile at all —cargo testfailed with "current package believes it's in a workspace when it's not". Added it toworkspace.members(andrlibto its crate-type), so it now builds and is covered by CI'scargo check --locked --all-targets. Its stale standaloneCargo.lockis deleted. This is why the diff reaches slightly past the guards; there was no way to satisfy "tests passing" otherwise.Errors
FibonacciInputTooLargeSortInputTooLargePrimeLimitTooLargeLoopOpsTooLargeCombinedInputTooLargeDiscriminants are public surface — the SoroScope UI decodes by number — so a test pins them and new variants must be appended, not inserted.
Tests
13 tests in
contracts/cpu_heavy/src/test.rs, covering each guard on both sides of its boundary:nested_loop_burn(65_536, 65_536)and(u32::MAX, u32::MAX)are rejected — a naiveouter * innerguard would wrap past the cap;saturating_muldoes not.Notes for the reviewer
bubble_sortat its new cap still uses 60% of the CPU budget — safely inside for a direct invocation, but limited headroom if composed into a larger transaction. I kept 100 because it preserves the existingcombined_benchmarksub-cap and is a more useful benchmark than a smaller value; dropping to 64 would put it near 25% if you'd prefer the margin. Happy to change it.soroscope/contracts/cpu_heavy/untouched — no recent commit touches it and it isn't in the workspace, so mirroring the change there seemed like editing dead code. Say the word if you'd rather it stay in sync.mainbreakage as [Contract] Add Liquidity Pool Slippage Protection via Minimum Output Parameter #717; worth deconflicting those two before either merges.