Skip to content

Add DeepBook core account wrapper#1091

Merged
emmazzz merged 9 commits into
mainfrom
emma/deepbook-core-account-wrapper
Jul 6, 2026
Merged

Add DeepBook core account wrapper#1091
emmazzz merged 9 commits into
mainfrom
emma/deepbook-core-account-wrapper

Conversation

@emmazzz

@emmazzz emmazzz commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add a new deepbook_core_account Move package that wraps DeepBook core trading APIs with account-model custody.
  • Store one stable embedded balance manager plus deposit/withdraw/trade caps in a DeepbookCoreAccountApp account data slot.
  • Fund the embedded manager temporarily for order and swap calls, then sweep free base/quote/DEEP balances back into the account.
  • Add permissionless settled withdrawal through account-registry app auth, so any caller can redeem settled DeepBook amounts into the target account once the app is authorized.

Key decisions

  • The balance manager is lazily attached on owner-authorized first use through the DeepBook registry-authorized app path.
  • Owner-authorized wrapper APIs take AccountWrapper + Auth instead of exposing balance-manager custody to callers.
  • Permissionless settled withdrawal takes AccountRegistry and uses DeepbookCoreAccountApp app auth; it requires an already-initialized account slot because a never-initialized embedded manager cannot own settled orders.
  • Tests use whitelisted pools for settlement/swap custody checks so expected values focus on account-manager movement instead of fee math.

Test plan

  • npx -y prettier-move -c packages/deepbook_core_account/sources/deepbook_core_account.move packages/deepbook_core_account/tests/deepbook_core_account_tests.move --write
  • sui move build --path packages/deepbook_core_account --build-env testnet
  • sui move test --path packages/deepbook_core_account --build-env testnet --gas-limit 100000000000

@emmazzz emmazzz force-pushed the emma/deepbook-core-account-wrapper branch 2 times, most recently from c3e4314 to 0d39c6a Compare June 26, 2026 22:34
@emmazzz emmazzz marked this pull request as ready for review June 26, 2026 22:39
@emmazzz emmazzz force-pushed the emma/deepbook-core-account-wrapper branch from 0d39c6a to 06f0814 Compare June 26, 2026 22:42
@emmazzz emmazzz force-pushed the emma/deepbook-core-account-wrapper branch from 2ea041b to f4f5932 Compare July 1, 2026 21:14
@emmazzz emmazzz marked this pull request as draft July 1, 2026 23:09
@emmazzz emmazzz closed this Jul 1, 2026
@emmazzz emmazzz reopened this Jul 1, 2026
@emmazzz emmazzz marked this pull request as ready for review July 1, 2026 23:12
@0xaslan

0xaslan commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Code review

No fund-loss or theft path found — custody is sound (the embedded manager's owner is a derived-object address no one can sign for, so the slot-held caps are the sole authority). The findings below are a keeper-liveness gap, an API-semantics footgun, and cleanup, ordered most severe first. Every finding was verified against the actual callee implementations (account.move, balance_manager.move, pool.move, vault.move, margin_manager.move).

Findings

1. Permissionless settled-withdraw aborts when an initialized manager has nothing settled — and that branch is untestedsources/deepbook_core_account.move:301

vault::settle_balance_manager_permissionless asserts ENoBalanceToSettle (vault.move:115–119), while the authed variant's settle_balance_manager no-ops on zero settled. A keeper batching withdraw_settled_amounts_permissionless across accounts in one PTB gets the entire PTB reverted by a single account whose orders are still resting. The existing test (permissionless_withdraw_settled_amounts_noops_for_uninitialized_account) only covers the uninitialized early-return; no test drives initialized-but-nothing-settled. Suggest pre-checking settled balances in the wrapper (early return, matching the uninitialized path) or documenting the abort so keepers call per-account.

2. Cancel/withdraw paths call ensure() and mint a manager + 3 caps + DeepbookCoreAccountInitialized event on semantic no-opssources/deepbook_core_account.move:220, 247, 273

cancel_live_orders(vector[]) or withdraw_settled_amounts on a never-traded account permanently creates custody state and tells indexers the account onboarded to DeepBook. A never-initialized manager can't own orders or settled amounts, so replacing ensure with the same if (!account_data::is_initialized(account)) return that withdraw_settled_amounts_permissionless already uses is behavior-preserving for initialized accounts (ensure is a no-op there) and makes the uninitialized policy consistent across all non-funding paths. ensure then stays only where funding actually flows in (place_*).

3. max_*_in is not a cap — it's withdrawn in full up-frontsources/deepbook_core_account.move:107-109

Core has no max-cost concept (pool.place_limit_order draws what it needs from the manager and aborts internally if short), so these params are the wrapper's own funding declarations, withdrawn from the account in full before the pool is called. The name suggests "at most"; the semantics are "fund exactly this much, temporarily". A caller who treats max_quote_in as a protective upper bound (e.g. passes 1000 with 100 in the account, for an order needing 50) aborts with EBalanceTooLow before the pool is ever reached. The most surprising instance is a DEEP-quoted (or DEEP-based) pool: withdraw_or_zero<QuoteAsset> and withdraw_or_zero<DEEP> drain the same balance, so a caller thinking in separate "quote budget" and "fee budget" pots must hold the sum — even though the pooled manager balance correctly covers both, and max_deep_in = 0 with the fee budget folded into max_quote_in works fine there. Suggest renaming (e.g. base_funding_in) or a doc comment stating "this exact amount is withdrawn from the account upfront; unused funds are swept back", ideally with a note on the DEEP-collision case.

4. Unconditional 3-type settle wastes a Bag write + accumulator lookup per unused asset on the hottest pathsources/deepbook_core_account.move:114-116, 172-174

settle<T> performs set_last_settlement_ms (a settlements-Bag dynamic-field write) and an accumulator read before its amount == 0 early return, so a one-sided order with pay_with_deep = false wastes 2 of 3 settles per placement. Gating each settle on max_*_in > 0 is safe — the sweep-back deposit is pure stored-balance and needs no prior settle. (Predict follows the same per-type settle discipline.)

5. The sweep→deposit epilogue is copy-pasted in all six mutating entrypoints; the 9-line funding prologue is byte-identical between the two order functionssources/deepbook_core_account.move:141 (epilogue also at 196, 224, 251, 277, 302)

A missed edit in one of six sites would silently strand funds in the manager. A public(package) fun sweep_back<Base, Quote>(account: &mut Account, ctx: &mut TxContext) (borrow_mut internally → sweep → deposit) compiles under Move's borrow rules — the current code already sequences those borrows identically — and removes the 3-Coin tuple that only travels between two adjacent calls. Same for a no-return fund_manager helper for the prologue (a reference-returning variant would not type-check, which is presumably why it was inlined).

6. Raw 0 literals alongside the defined zero-constants in teststests/deepbook_core_account_tests.move:205 (and ~11 similar lines)

ZERO_BALANCE/NO_OPEN_ORDER_COUNT are used in some assertions and raw 0 in others for the same concepts (e.g. manager-balance checks at lines 108-110 vs 205-206). Trivial consistency fix per the repo test conventions.

Checked and cleared

  • Parameter forwarding into pool.place_limit_order / place_market_order / cancel_* matches the core signatures exactly — no transposition.
  • The wrapper_id event field is correct (receive_address().to_id() round-trips to the wrapper's object ID).
  • Settle-before-withdraw ordering in the order paths is correct; the sweep-only paths legitimately skip settle (deposit is pure stored-balance).
  • The permissionless path's app auth can only move an account's own settled funds into its own custody, and cannot force-create managers on arbitrary accounts.
  • Registry deauth being creation-only matches the margin precedent (disable_version is the actual killswitch).
  • DEEP-quoted/based pools behave correctly on the happy path: both funding deposits pool in the same manager balance, which core draws from for quote and fees alike; the double sweep and double settle of the shared type are benign no-ops.
  • Move.toml token pin (rev = "main") and root/clock/ctx parameter placement both match established repo conventions.

One PR-description nit: the summary says the manager is funded "for order and swap calls," but the diff contains no swap wrappers — worth updating the description or noting swaps as follow-up.

Standalone verdict: the package is isolated (nothing existing is touched), so it's safe as a standalone PR once findings 1–2 are addressed — 1 is a keeper-facing abort that would otherwise ship into the public API surface. Finding 3 is naming/docs only, but worth settling pre-deploy since the trading signatures are a permanent public API.

🤖 Generated with Claude Code

@emmazzz emmazzz merged commit 3068ae8 into main Jul 6, 2026
5 checks passed
@emmazzz emmazzz deleted the emma/deepbook-core-account-wrapper branch July 6, 2026 16:28
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.

2 participants