docs(clob): apply Mike's feedback to README and code comments#6
Open
mikemaccana wants to merge 5 commits intoquiknode-labs:mainfrom
Open
docs(clob): apply Mike's feedback to README and code comments#6mikemaccana wants to merge 5 commits intoquiknode-labs:mainfrom
mikemaccana wants to merge 5 commits intoquiknode-labs:mainfrom
Conversation
…ized-exchange-clob
Adds a teaching-grade central limit order book under defi/clob/anchor.
The port brings the source program from Anchor 0.32.1 to Anchor 1.0.0 and
conforms it to the solana-anchor-claude-skill ruleset so it can sit
alongside the other Financial Software examples.
Why this example belongs in program-examples:
- The existing DeFi corpus covers constant-product AMMs and peer-to-peer
escrow; a CLOB rounds out the set so readers can see how limit-order
exchanges work on Solana without having to read Openbook/Phoenix's much
larger zero-copy codebases.
- It demonstrates several patterns the simpler examples don't: PDAs
authoring token vaults, per-user per-market state, and an unsettled-
balance + settle step that mirrors how real exchanges decouple
matching from fund movement.
Program side (Anchor 1.0 migration + skill rules):
- declare_id! kept; every handler uses `context` rather than `ctx`.
- `context.bumps.x` direct field access, no `.get("x").unwrap()`.
- All #[account] structs derive InitSpace and store `pub bump: u8`,
saved in the init handler.
- Every `space = ...` uses `T::DISCRIMINATOR.len() + T::INIT_SPACE` —
no magic 8, no hand-sized byte math, no custom discriminator consts.
- Clock::get()? instead of the anchor_lang::solana_program path.
- Token accounts use anchor_spl::token_interface for Token-2022 support.
- PlaceOrderAccountConstraints and SettleFundsAccountConstraints box
their InterfaceAccount fields — without boxing the BPF frame exceeds
the 4 KB stack-offset limit. A comment on each documents the reason.
- CpiContext::new / new_with_signer now take `token_program.key()`,
matching the Anchor 1.0 signature change.
- MAX_ORDERS_PER_SIDE and MAX_OPEN_ORDERS_PER_USER are named
constants with rationale, instead of the magic 100 / 20 in the source.
- Dead matching-engine helpers from the upstream `utils/matching.rs`
are removed: they were never invoked by place_order and contained an
obvious quantity-accounting bug. The README's "Scope note" flags that
a real matching engine is the natural next extension — better to be
honest about the limit than to ship broken code.
Test side (Mike-canonical pattern):
- node:test via `npx tsx --test --test-reporter=spec`.
- solana-kite `connect()` / `createWallets` / `createTokenMint` /
`sendTransactionFromInstructions` — no @coral-xyz/anchor, no
web3.js v1, no ts-mocha, no chai, no bs58.
- @solana/kit types (TransactionSigner, Address, lamports).
- Codama-generated TS client under dist/clob-client (built by the
Anchor.toml `test` script via `npx create-codama-clients`).
- TOKEN_EXTENSIONS_PROGRAM from solana-kite, PDAs via
`connection.getPDAAndBump`, token accounts via
`connection.getTokenAccountAddress`.
- 9 tests cover the full happy path (initialize, create user, bid,
ask, cancel, settle, cancel+settle buyer) plus two failure cases
(invalid price, non-owner cancel).
Known limitation called out in the README: surfpool (Anchor 1.0's
default local validator) does not accept the websocket RPC methods
Kit uses for transaction confirmation; `anchor test --validator legacy`
is required until surfpool catches up.
…conventions Bring the CLOB example in line with the repo's Anchor 1.0 + LiteSVM Rust test convention established by tokens/escrow and defi/asset-leasing. Previously CLOB shipped a TypeScript suite backed by Codama-generated clients and @solana/kit, which was a one-off in the current examples set. Why: - Every other defi/*/anchor and the tokens/escrow example now uses LiteSVM-driven Rust tests that `include_bytes!` the built .so, share a common test-stack (litesvm + solana-kite + solana-signer), and run under a plain `cargo test`. Contributors moving between examples had to relearn the CLOB harness (Codama client generation, surfpool-vs-legacy validator selection, tsx + node:test runner). - The JS suite also required `anchor test --validator legacy` because Anchor 1.0's default surfpool validator does not expose the websocket RPC methods Kit uses for confirmation. Switching to LiteSVM Rust sidesteps that entirely — no validator at all. Changes: - Anchor.toml: drop anchor_version, package_manager, [hooks], [test] blocks. Set `[scripts] test = "cargo test"` matching asset-leasing. Keep solana_version = 3.1.8 pinned so BPF toolchain stays in lock-step. - programs/clob/Cargo.toml: add [dev-dependencies] for litesvm 0.11, solana-signer 3.0, solana-keypair 3.0.1, solana-kite 0.3. Versions match the rest of the repo to avoid drift. - programs/clob/tests/test_clob.rs: 13 LiteSVM tests covering initialize_market (happy path + zero-tick + oversized-fee rejection), create_user_account, place_order (bid locks quote, ask locks base, zero-price / unaligned-tick / below-min rejections), cancel_order (owner refund credited to unsettled, non-owner rejected), and settle_funds (drains unsettled balance from vault to user ATA for both an ask cancel and a bid cancel + full refund round-trip). - programs/clob/src/lib.rs and instructions/*.rs: rename the verbose `*AccountConstraints` struct suffix to the plain `InitializeMarket`, `PlaceOrder`, etc. naming that every other Anchor example in the repo uses. Rename handler functions from bare `initialize_market` to `handle_initialize_market` to match escrow and asset-leasing. - README.md: replace the TS/Codama test instructions with the Rust LiteSVM flow; drop the surfpool caveat (no longer relevant). - Remove tests/clob.test.ts, package.json, tsconfig.json: no JS/TS scaffolding needed now that testing is pure Rust. - .gitignore: drop the now-unused `dist` entry (the Codama client output directory). Scope note carried into the tests: the program does not run a matching engine, it only keeps the book and escrow the funds. The test file's header comment calls this out so the reader does not look for cross-order settlement tests that cannot exist yet. Test result: `anchor build && cargo test` → 13 passed, 0 failed.
Previously place_order booked orders and escrowed funds but never crossed them — a CLOB with no matching is pointless. This commit completes the job: incoming orders walk the opposite side of the book using price-time priority, match at the resting (maker's) price, credit fills to unsettled_* balances, and route a configurable taker fee to a dedicated fee vault. Matching semantics ------------------ - A taker bid walks asks lowest-first; a taker ask walks bids highest-first. Fills stop when either the taker is exhausted or the next resting order's price fails the limit check. - Fills happen at the MAKER'S price (price improvement for the taker). The taker's locked-up-front quote that isn't spent is refunded to their unsettled_quote. - Time priority is implicit in the OrderBook's sorted Vecs: at the same price, the earliest insertion is at the lower index and fills first. - Any unmatched remainder rests on the book as a new maker order with the original limit price. Fee model --------- Single taker_fee (basis points) deducted from the gross quote of each fill and routed to a new market-owned fee_vault (one CPI per place_order, aggregated across fills). Makers never pay an explicit maker fee. See programs/clob/src/instructions/place_order.rs for the trade-offs vs a taker-funded (extra-transfer) model. New instruction --------------- withdraw_fees: authority-gated drain of the fee vault into the authority's quote token account. No-ops on an empty vault so it is safe to call on a schedule. Remaining accounts pattern -------------------------- Maker Order PDAs and their owners' UserAccount PDAs are passed as remaining_accounts in pairs, in book-walk order. The program re-verifies each pair against the live book and rejects mismatches. Tests ----- 13 existing LiteSVM tests untouched and still pass; 10 new tests cover: fully-crossing bid, fully-crossing ask, partial-fill of resting order, partial-fill of taker, multi-level crossing with price priority, time priority at a tie, price-improvement rebate, fee maths, withdraw_fees drain, and settle_funds after matching.
The previous README was a ~78-line stub. This version describes the program as it actually exists today, including the matching engine landed in ea96084. Structure matches the rest of the overhaul: 1. What does this program do? (onchain mechanics first, with tradfi terms — limit order, order book, maker/taker, price-time priority — briefly explained in plain English before they get used) 2. Glossary (account, PDA, CPI, bps, bid/ask, tick size, unsettled balance, price improvement, remaining_accounts, etc.) 3. Accounts and PDAs (four program PDAs + three vaults; full field lists; note the vaults are not PDAs, they are regular token accounts whose authority is the Market PDA) 4. Instruction lifecycle walkthrough (six instructions, in the order a user encounters them; per-ix signers / accounts / PDAs created / token-flow diagrams / state changes / checks) 5. The matching engine — step by step (the critical section: how place_order uses remaining_accounts; the plan/apply/clean/ fee/rest five-step structure; fee math; price improvement; worked fill walkthroughs including a multi-maker sweep) 6. Full-lifecycle worked examples (clean match + settle, partial fill + remainder, cancel + settle round trip) 7. Safety and edge cases (full error table; guarded design choices; what the example does NOT do) 8. Running the tests (all 23 tests listed and categorised; CI note confirming anchor build runs before anchor test) 9. Extending the program (easy / moderate / harder) 1433 lines. No code changes.
Five consistent lessons from earlier reviews, applied to the CLOB program. 1. 'Token' not 'SPL Token' — tokens are the default on Solana, no qualifier is needed unless specifically contrasting with native SOL. Replaced 'SPL Token' / 'SPL token' throughout README, state/market.rs, and tests. In tests, 'classic SPL Token vs Token-2022' becomes 'Classic Token Program vs Token Extensions Program' — more precise and drops the SPL prefix that's noise. 2. Glossary removed. The old section enumerated every Solana term (Account, Lamport, Signer, PDA, Bump, CPI, ...) which duplicates what https://solana.com/docs/terminology already covers. Replaced with a one-line pointer there, plus a short inline 'Terms' block that defines only genuinely CLOB-specific vocabulary (base/quote, tick size, unsettled balance, fee vault, price improvement, remaining accounts). 3. No Ethereum references. Old README described an SPL token as 'Solana's ERC-20 equivalent'. Removed — explain Solana on its own terms. 4. Accurate finance framing. Added a sentence at the top noting that a CLOB is the same matching mechanism every major equity / futures / FX / crypto exchange (NYSE, NASDAQ, LSE, CME, Binance, Coinbase, Openbook, Phoenix) uses. Previously the README framed the program as 'two users who want to swap tokens' — accurate but understated. A CLOB is real finance infrastructure and the README now says so. Renamed the 'Tradfi background' subsection to 'Finance background'. 5. 'Instruction handler' not 'instruction' when referring to the code. An instruction is the call data submitted in a transaction; the instruction handler is the Rust function that processes it. Updated 'the program has six instructions', 'later instructions can validate', 'no instruction flips this', 'close in the same instruction', etc. Phrasing like 'a user calls the place_order instruction' is left alone because it genuinely refers to the call. Additional cleanups bundled in: - 'on-chain' / 'off-chain' → 'onchain' / 'offchain' in README, matching the repo-wide normalisation in commit fa93ce0. - 'SPL Token program' → 'Token program' in CPI descriptions. - 'SPL token accounts' → 'token accounts' in the vaults section. - Code comment in place_order.rs describes basis points as 'the universal rate convention on every major exchange' instead of the vague 'TradFi and CEXes'. - Code comment in market.rs: 'program instructions can drain it' → 'program instruction handlers can drain it'. - initialize_market.rs: 'Basis-points' → 'Basis points' (punctuation). Section numbers in the README renumbered (2/3→2, 4→3, 5→4, ...) after the Glossary removal; all cross-references updated. Quasar port: NOT included in this change. Quasar's account macros require fixed-layout structs (it does not support Vec<T> fields on #[account] structs — see basics/favorites/quasar/src/state.rs for the explicit call-out). The CLOB's OrderBook is a Vec<OrderEntry> pair and UserAccount has a Vec<u64> open_orders, both of which rely on dynamic insertion and removal. On top of that, place_order iterates through remaining_accounts to deserialize and mutate multiple maker Order / UserAccount PDAs per call — a pattern none of the existing Quasar examples (escrow, token-swap, counter) demonstrate. Porting would need a fresh fixed-capacity OrderBook design and a new cross-account mutation pattern. Left as a follow-up so the terminology and finance framing fixes ship first; a separate PR can tackle the Quasar port once the architectural approach is agreed. Tests: all 23 existing LiteSVM tests in programs/clob/tests/test_clob.rs still pass locally after the sweep.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0410c68. Configure here.
| pub owner: Signer<'info>, | ||
|
|
||
| pub token_program: Interface<'info, TokenInterface>, | ||
| } |
There was a problem hiding this comment.
Missing vault has_one constraints allows fee vault draining
High Severity
The SettleFunds account struct has no has_one constraints linking base_vault or quote_vault to the market's stored vault addresses. Since fee_vault and quote_vault share the same mint (quote) and the same authority (market PDA), a user with unsettled_quote > 0 can pass the fee_vault address as quote_vault. The transfer_checked CPI only validates mint and authority, so it succeeds — draining accumulated fees to the caller instead of withdrawing from the correct vault. The PlaceOrder struct has the same gap for base_vault, quote_vault, base_mint, and quote_mint.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0410c68. Configure here.
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.
Summary
Applies the same five-point feedback Mike gave on the asset-leasing
README to the CLOB (central limit order book) program: Token vs SPL
Token, glossary cleanup, no Ethereum references, accurate finance
framing, instruction-handler terminology.
Changes
README rewrite (
defi/clob/anchor/README.md)Token, not SPL Token. Tokens are the default on Solana; no
qualifier needed. Replaced throughout — vault descriptions, CPI
descriptions, the 'SPL token' definition block.
Glossary removed. The old section re-defined standard Solana
terms (Account, Lamport, Signer, PDA, Bump, CPI, Discriminator,
ATA) that are all covered at https://solana.com/docs/terminology.
Replaced with a one-line pointer there plus a short inline terms
block for genuinely CLOB-specific vocabulary (base/quote, tick
size, unsettled balance, fee vault, price improvement, remaining
accounts).
No Ethereum references. Old README described an SPL token as
"Solana's ERC-20 equivalent". Removed — Solana explained on its
own terms.
Accurate finance framing. Added an explicit opening sentence
noting a CLOB is standard financial-market infrastructure — NYSE,
NASDAQ, LSE, CME, and every major crypto venue run on one. Two
existing Solana CLOBs (Openbook v2, Phoenix) linked as reference
reads. Renamed the "Tradfi background" subsection to
"Finance background".
"Instruction handler" when referring to code. "The program
has six instructions" → "six instruction handlers"; "later
instructions can validate" → "later instruction handlers can
validate"; etc. Left phrasing like "a user calls the
place_orderinstruction" alone because it refers to the call, not the code.
Additional cleanup bundled in:
on-chain/off-chain→onchain/offchain(matches therepo-wide normalisation from commit
fa93ce0d).SPL Token program→Token programin CPI descriptions.removal; all in-page cross-references updated.
Code comment sweep
Same feedback applied to Rust comments across the program:
programs/clob/src/state/market.rs— "program instructions(notably withdraw_fees) can drain it" → "program instruction
handlers (notably withdraw_fees) can drain it".
programs/clob/src/instructions/initialize_market.rs—Basis-points→Basis points(punctuation tidy).programs/clob/src/instructions/place_order.rs— code commenton basis points: "standard in TradFi and CEXes" → "the universal
rate convention on every major exchange (NYSE, CME, Binance,
Coinbase, ...)".
programs/clob/tests/test_clob.rs— "classic SPL Token vsToken-2022 via
TokenInterface" → "Classic Token Program vsToken Extensions Program via
TokenInterface" — drops theredundant "SPL" prefix and names the two programs precisely.
No function, file, or struct names changed.
Quasar port — deferred
Quasar's
#[account]macro requires fixed-layout structs and doesnot support
Vec<T>fields on account state (seebasics/favorites/quasar/src/state.rsfor the explicit call-outabout this limitation). The CLOB's
OrderBookusesVec<OrderEntry>on each side with dynamic insertion/removal, andUserAccounthas aVec<u64>of open order IDs. Additionally,place_orderwalksremaining_accountsat runtime to deserialiseand mutate a variable number of maker
OrderandUserAccountPDAs per call — a pattern none of the existing Quasar examples
(escrow, token-swap, counter) demonstrate.
A faithful port would need a fresh fixed-capacity order-book design
(bounded arrays with a length prefix) and a new cross-account
mutation pattern, plus a 1800-line Rust test port. Left as a
follow-up PR so the terminology and finance-framing fixes ship
first and the Quasar architectural call can be discussed
separately.
Verification
Locally, from
defi/clob/anchor/:All 23 existing LiteSVM tests pass:
Note
High Risk
Introduces a brand-new onchain trading program (order matching, custody vaults, fee accounting) plus extensive integration tests; correctness and security bugs here could lead to loss of funds or unfair matching/fee behavior.
Overview
Adds a new Anchor CLOB (central limit order book) example under
defi/clob/anchor, including market/user/order/orderbook state, a price-time priority matching engine inplace_order, and separate handlers forcancel_order,settle_funds, andwithdraw_fees(fee vault + authority-gated withdrawal).Includes a large teaching-focused
defi/clob/anchor/README.md, workspace/tooling files (Anchor.toml,Cargo.toml,.gitignore), and a comprehensive LiteSVM test suite validating matching behavior, vault locking/settlement, and fee math. Updates the repoREADME.mdto list and link the new CLOB example.Reviewed by Cursor Bugbot for commit 0410c68. Bugbot is set up for automated code reviews on this repo. Configure here.