Conversation
|
This PR has been marked stale due to 7 days of inactivity. |
Co-authored-by: Jennifer <jenpaff0@gmail.com>
Co-authored-by: 0xrusowsky <90208954+0xrusowsky@users.noreply.github.com>
|
Hi @0xrusowsky — your review approval was detected by Voight-Kampff, but this repository configures Ask another reviewer to |
update spec
Updates TIP-1088 to include the remaining tick-liquidity changes implemented by #6939. - Stops maintaining stored per-tick `totalLiquidity` at T9. - Derives `getTickLevel.totalLiquidity` on demand with checked summation. - Deprecates existing aggregate slots in place without migration. - Explicitly forbids O(N) liquidity scans in placement, fill, cancellation, and flip paths. - Documents the read/write gas and complexity changes. Validation: `git diff --check` Prompted by: @jenpaff
|
Hi @danrobinson — your review approval was detected by Voight-Kampff, but there is no email mapping for your GitHub account, so no approval prompt can be sent to your device. Your review did not trigger an approval prompt and is not counted toward this PR. Because Voight-Kampff does not know which email address belongs to |
|
This PR has been marked stale due to 7 days of inactivity. |
|
Closed due to inactivity. Reopen if still needed. |
|
This PR has been marked stale due to 7 days of inactivity. |
|
Closed due to inactivity. Reopen if still needed. |
|
cyclops audit |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
PR #6700 adds TIP-1088 for StablecoinDEX quote/swap fill parity and T11 deprecation of the stored per-tick totalLiquidity aggregate. The remaining actionable issues are inline.
Reviewer Callouts
- ⚡ Stale aggregate slots:
tips/tip-1088.md:72says staleTickLevel.totalLiquidityslots remain and need not be cleared. That can permanently strand pre-T11 slot occupancy and its TIP-1060 clearing credit; decide whether to clear slot 1 on first post-T11 write/delete or document the state bloat explicitly. - ⚡ Book-structure invariants become load-bearing: Once depth is derived from
head -> next, the spec should state invariants fortail, bitmap bits,best_*_tick, and non-orphaned live orders. The stored aggregate currently acts as a redundancy/canary that T11 removes. - ⚡ Quote mutability must remain
view: The external quote ABI isview; routing through a shared engine should not be implemented by dispatching quote entrypoints through a mutating helper that rejectsSTATICCALL.
| - Traverse the same reachable ticks and the same individual orders within those ticks, in the same priority order as `Execute` mode. Traversal occurs at **order granularity**, not tick-aggregate granularity. | ||
| - Apply the same rounding direction at every conversion step. | ||
| - Produce the same fill amounts and price progression as `Execute` mode for an identical state snapshot. | ||
| - Avoid all writes and side effects: |
There was a problem hiding this comment.
Simulate is specified by an enumerated no-write list, but the list does not explicitly cover the persistent TIP-1060 DEX storage-credit ledger (StorageCreditDeltas, dex_storage_credits, order-slot reuse/allocation). Current view(...) dispatch and the storage provider do not fail closed on missed writes, so an implementation that leaves storage-credit accrual/flush in the shared engine could let quotes/staticcalls persist free DEX storage credits; analogous missed writes could mint internal balances.
Recommended Fix: Explicitly include DEX storage-credit accrual/spend, order-id allocation, deleted-slot reuse, and storage-credit preservation state in the no-write invariant, and require a fail-closed read-only guard that aborts on any sstore, tstore, event/log, or journaled write during Simulate.
|
|
||
| 5. Tick liquidity compatibility at T11: | ||
| - The `getTickLevel` function signature and return shape remain unchanged. | ||
| - `getTickLevel.totalLiquidity` MUST equal the checked sum of `remaining` across all active orders in the tick's linked list. |
There was a problem hiding this comment.
getTickLevel.totalLiquidity overflow
The current totalLiquidity maintenance also enforces the only per-tick Σ remaining <= u128::MAX bound via a write-path checked_add. T11 forbids maintaining that aggregate and also forbids write-path traversal to validate it, while this line requires a checked uint128 sum on read. Bid-side books at negative ticks can keep quote escrow within the TIP-20 cap while base-denominated resting remaining exceeds u128::MAX, moving the overflow from placement to getTickLevel and making the getter revert for that tick until orders are cancelled/filled.
Recommended Fix: Keep an explicit write-path bound, define saturating/clamped getter semantics, or document the revert as an intended failure mode and update consumers/tests that currently treat getTickLevel as infallible.
| 6. API compatibility: | ||
| - External quote and swap method signatures remain unchanged. | ||
| - Quote precision now matches swap precision exactly for the same state snapshot. Parity is scoped to the fill math (tick/order traversal, rounding, and fill amounts); caller-level and token-specific constraints (e.g., TIP-20 transfer fees) and execute-only side effects are outside this guarantee. In particular, `swap()` places flip orders and `quote()` does not: a flip-order placement that raises a system error reverts `swap()` (post-T1A) but not `quote()`. Business-logic flip failures are swallowed and do not change the taker fill, so fill-math parity still holds; only execute-only revert outcomes differ. | ||
| - Gas costs change: `quote()` and `getTickLevel()` become more expensive because they traverse individual orders, while swap and order-management paths avoid the storage operations previously used to maintain `totalLiquidity`. Callers SHOULD NOT depend on specific gas costs for these functions. |
There was a problem hiding this comment.
getTickLevel traversal is unbounded and attacker-controlled
The spec acknowledges higher gas but gives no maximum list length, pagination, cap, or fallback. A maker can park many minimum-sized orders at a chosen tick and later cancel them, so a third party controls how many storage records this public getter must read. That turns a formerly O(1) view into an unbounded call that can revert/OOG for on-chain consumers and invariant checks even when the sum fits.
Recommended Fix: Preserve an O(1) depth value, add a bounded/paginated depth API with continuation state, or define capped/best-effort semantics for totalLiquidity above a documented traversal limit.
|
Must update copy to reflect that this will be in T12, not T11 |
9 participants
Description
Proposes TIP-1088 for T11.
Quote/swap parity —
quote()runs the same per-order fill engine asswap(), read-only: same traversal, rounding, andInsufficientLiquiditybehavior.
minAmountOut = quoteno longer reverts.Tick liquidity on demand (docs(tip-1088): derive tick liquidity on demand #6942) — write paths stop maintaining the
per-tick
totalLiquidityaggregate;getTickLevelderives it as a checkedsum over active orders. No O(N) scans in write paths; slots deprecated in
place at T11.