Limit order tests: CleanLimitBook L1 no-transfer assertion + non-owner claim rejection (#263 / #264 follow-ups) #271

New issue

2026-06-02T06:59:59Z

PlasticDigits commented

2026-06-02 06:59:59 +00:00

(Migrated from gitlab.com)

Summary

Close two test-coverage gaps in the limit-order park → claim lifecycle left after GitLab #263 (CleanLimitBook) and #264 (match-time dust flush). Both are tests-only deliverables — no contract behavior changes unless a test exposes a regression.

Bundled scope:

#263 follow-up: Assert CleanLimitBook emits zero CW20/bank transfer submessages in the clean tx (refunds only on ClaimExpiredLimitOrder).
#264 follow-up: Add a non-owner ClaimExpiredLimitOrder rejection integration test (mirror existing cancel batch guard).

Current codebase

Park / clean path — no CW20 in park tx (L1, L15)

CleanLimitBook (limit_book_clean.rs → execute_clean_limit_book in contract.rs) walks the book and calls park_limit_order_for_clean; response is attributes + wasm events only — no .add_message() CW20 transfers.
Invariant L15 and L1 document: parks move rows into EXPIRED_LIMIT_CLAIMS with PENDING_ESCROW_* unchanged; maker CW20 refund happens only on ClaimExpiredLimitOrder.
Existing tests (#263):
- clean_limit_book_parks_expired_head_default_config — parks, checks attrs, claim rows, order off book.
- clean_limit_book_force_dust_bid_then_claim_refunds — force-parks via governance threshold, then maker claims and balance increases — indirectly proves refund was deferred, but does not assert zero transfers / unchanged maker balance during the clean tx.
- Hybrid park test expired_bid_parked_on_hybrid_walk_claim_refunds_maker does assert maker balance unchanged after park ("maker must not receive token1 until ClaimExpiredLimitOrder") — pattern to reuse for clean.
Unit coverage: orderbook::tests::park_expired_bid_unlinks_and_records_claim_without_pending_delta covers taker-walk park, not CleanLimitBook execute path.

Claim owner guard (L1, L11)

execute_claim_expired_limit_order (contract.rs ~L1222): if row.owner != info.sender { return Err(ContractError::Unauthorized {}) }.
execute_claim_expired_limit_orders (limit_batch_withdraw.rs ~L166): same per-row owner check; all-or-nothing revert.
Existing rejection tests:
- cancel_limit_order_non_owner_rejected — single cancel (L3).
- batch_cancel_foreign_owner_reverts_whole_tx — batch cancel foreign owner → Unauthorized, orders remain (L11).
- Missing: single ClaimExpiredLimitOrder non-owner rejection; also missing: batch ClaimExpiredLimitOrders foreign-owner twin (optional stretch — same guard, same pattern as cancel batch).
Happy paths: batch_claim_expired_two_orders_one_tx, match_dust_flush_bid_hybrid_then_maker_claims, clean_limit_book_force_dust_bid_then_claim_refunds.

Why this is needed

L1 regression signal: A future refactor of execute_clean_limit_book or shared park helpers could accidentally attach a CW20 Transfer (e.g. copying cancel/refund logic). Balance-delta checks after claim do not catch premature refunds in the clean tx itself.
Authorization symmetry: Cancel paths have explicit non-owner tests; claim paths are equally security-sensitive (CW20 debit from pair custody) but currently rely on code review only for the single-claim entry point.
Audit matrix completeness: docs/contracts-security-audit.md rows L1, L11, L15 should cite explicit integration tests for these guards on the clean and claim execute paths.

Constraints and guardrails

Tests only — do not change pair wasm behavior unless a test fails and reveals a bug (file a separate fix issue).
No new governance knobs — threshold / cap semantics unchanged from #263 / #264.
Reuse existing helpers — execute_clean_limit_book, place_bid, place_expired_bids, query_cw20_balance, wasm_attr_in_action_event, hybrid park setup patterns.
cw-multi-test note: Submessages from Response are auto-executed; assert no CW20 balance change to maker during clean and/or scan tx events for absence of CW20 transfer wasm actions (see recommended direction).
Stretch (same issue, lower priority): batch_claim_expired_foreign_owner_reverts_whole_tx — only if trivial once single-claim test exists.

Relevant files

Area	Path
Clean execute	`smartcontracts/contracts/pair/src/limit_book_clean.rs`, `contract.rs` (`execute_clean_limit_book`)
Claim execute	`contract.rs` (`execute_claim_expired_limit_order`), `limit_batch_withdraw.rs` (`execute_claim_expired_limit_orders`)
Park helper	`smartcontracts/contracts/pair/src/orderbook.rs` (`park_limit_order_for_clean`)
Integration tests	`smartcontracts/tests/src/limit_order_tests.rs`
Test helpers	`smartcontracts/tests/src/helpers.rs` (if adding shared assert helper)
Invariant docs	`docs/contracts-security-audit.md` (L1, L11, L15, L16)
User docs	`docs/limit-orders.md` § Permissionless limit book clean, § Claim expired

Acceptance criteria

Integration test proves CleanLimitBook tx does not CW20-transfer to maker (balance +/or event scan) while parked claim row exists.
Same test (or paired test) proves maker can claim afterward — refund deferred, not lost.
Integration test proves non-owner ClaimExpiredLimitOrder → Unauthorized; claim row and balances unchanged.
make test-contracts / cargo test -p cl8y-dex-tests limit_order green.
docs/contracts-security-audit.md invariant rows cite new test function names.

Test plan — functional paths

Path	Expectation
Force-dust clean (#263 config)	Clean parks 1 row; zero CW20 to maker; claim refunds
Time-expired clean (default 0/0 config)	Clean parks head; zero CW20 to maker; claim refunds
Non-owner single claim	`Unauthorized`; row persists
Owner claim after rejected attacker attempt	Still succeeds; single refund
(Stretch) Batch claim foreign owner	Whole tx reverts; both rows persist

Test plan — attack / abuse / hack vectors

Vector	Verification
Permissionless keeper steals escrow via clean	Clean tx: no CW20 out; pair balance − reserves − pending unchanged (L1)
Attacker claims victim's parked dust (#264 flush)	Non-owner claim rejected; dust remains in `EXPIRED_LIMIT_CLAIMS`
Attacker claims after force-clean (#263)	Same rejection
Double refund (clean + claim by different senders)	Clean never pays; only owner claim transfers
Premature pending decrement	`PENDING_ESCROW_*` unchanged across clean (query if helper exists)
Batch claim partial grief	(Stretch) Foreign id in batch reverts all claims — no partial CW20

Verification criteria

cargo test -p cl8y-dex-tests clean_limit_book_emits_no_cw20 (or chosen name) passes.
cargo test -p cl8y-dex-tests claim_expired_limit_order_non_owner_rejected passes.
Grep audit doc: new tests listed under L1, L15 (and L11 if batch stretch landed).
No wasm code diff unless regression found (then split fix into separate MR).

## Summary Close two **test-coverage gaps** in the limit-order park → claim lifecycle left after GitLab **#263** (`CleanLimitBook`) and **#264** (match-time dust flush). Both are **tests-only** deliverables — no contract behavior changes unless a test exposes a regression. **Bundled scope:** 1. **#263 follow-up:** Assert `CleanLimitBook` emits **zero** CW20/bank transfer submessages in the clean tx (refunds only on `ClaimExpiredLimitOrder`). 2. **#264 follow-up:** Add a **non-owner** `ClaimExpiredLimitOrder` rejection integration test (mirror existing cancel batch guard). ## Current codebase ### Park / clean path — no CW20 in park tx (**L1**, **L15**) - **`CleanLimitBook`** (`limit_book_clean.rs` → `execute_clean_limit_book` in `contract.rs`) walks the book and calls `park_limit_order_for_clean`; response is **attributes + wasm events only** — no `.add_message()` CW20 transfers. - **Invariant L15** and **L1** document: parks move rows into `EXPIRED_LIMIT_CLAIMS` with **`PENDING_ESCROW_*` unchanged**; maker CW20 refund happens only on **`ClaimExpiredLimitOrder`**. - **Existing tests (#263):** - `clean_limit_book_parks_expired_head_default_config` — parks, checks attrs, claim rows, order off book. - `clean_limit_book_force_dust_bid_then_claim_refunds` — force-parks via governance threshold, then maker claims and balance increases — **indirectly** proves refund was deferred, but **does not** assert zero transfers / unchanged maker balance **during** the clean tx. - Hybrid park test `expired_bid_parked_on_hybrid_walk_claim_refunds_maker` **does** assert maker balance unchanged after park (`"maker must not receive token1 until ClaimExpiredLimitOrder"`) — pattern to reuse for clean. - **Unit coverage:** `orderbook::tests::park_expired_bid_unlinks_and_records_claim_without_pending_delta` covers taker-walk park, not `CleanLimitBook` execute path. ### Claim owner guard (**L1**, **L11**) - **`execute_claim_expired_limit_order`** (`contract.rs` ~L1222): `if row.owner != info.sender { return Err(ContractError::Unauthorized {}) }`. - **`execute_claim_expired_limit_orders`** (`limit_batch_withdraw.rs` ~L166): same per-row owner check; all-or-nothing revert. - **Existing rejection tests:** - `cancel_limit_order_non_owner_rejected` — single cancel (**L3**). - `batch_cancel_foreign_owner_reverts_whole_tx` — batch cancel foreign owner → `Unauthorized`, orders remain (**L11**). - **Missing:** single `ClaimExpiredLimitOrder` non-owner rejection; **also missing:** batch `ClaimExpiredLimitOrders` foreign-owner twin (optional stretch — same guard, same pattern as cancel batch). - **Happy paths:** `batch_claim_expired_two_orders_one_tx`, `match_dust_flush_bid_hybrid_then_maker_claims`, `clean_limit_book_force_dust_bid_then_claim_refunds`. ## Why this is needed - **L1 regression signal:** A future refactor of `execute_clean_limit_book` or shared park helpers could accidentally attach a CW20 `Transfer` (e.g. copying cancel/refund logic). Balance-delta checks after claim do not catch **premature** refunds in the clean tx itself. - **Authorization symmetry:** Cancel paths have explicit non-owner tests; claim paths are equally security-sensitive (CW20 debit from pair custody) but currently rely on code review only for the single-claim entry point. - **Audit matrix completeness:** `docs/contracts-security-audit.md` rows **L1**, **L11**, **L15** should cite explicit integration tests for these guards on the **clean** and **claim** execute paths. ## Constraints and guardrails - **Tests only** — do not change pair wasm behavior unless a test fails and reveals a bug (file a separate fix issue). - **No new governance knobs** — threshold / cap semantics unchanged from #263 / #264. - **Reuse existing helpers** — `execute_clean_limit_book`, `place_bid`, `place_expired_bids`, `query_cw20_balance`, `wasm_attr_in_action_event`, hybrid park setup patterns. - **cw-multi-test note:** Submessages from `Response` are auto-executed; assert **no CW20 balance change to maker** during clean **and/or** scan tx events for absence of CW20 `transfer` wasm actions (see recommended direction). - **Stretch (same issue, lower priority):** `batch_claim_expired_foreign_owner_reverts_whole_tx` — only if trivial once single-claim test exists. ## Relevant files | Area | Path | |------|------| | Clean execute | `smartcontracts/contracts/pair/src/limit_book_clean.rs`, `contract.rs` (`execute_clean_limit_book`) | | Claim execute | `contract.rs` (`execute_claim_expired_limit_order`), `limit_batch_withdraw.rs` (`execute_claim_expired_limit_orders`) | | Park helper | `smartcontracts/contracts/pair/src/orderbook.rs` (`park_limit_order_for_clean`) | | Integration tests | `smartcontracts/tests/src/limit_order_tests.rs` | | Test helpers | `smartcontracts/tests/src/helpers.rs` (if adding shared assert helper) | | Invariant docs | `docs/contracts-security-audit.md` (**L1**, **L11**, **L15**, **L16**) | | User docs | `docs/limit-orders.md` § Permissionless limit book clean, § Claim expired | ## Recommended solution direction ### 1. `CleanLimitBook` zero-transfer assertion (#263) 1. Add helper e.g. `assert_no_cw20_transfers_in_tx(app, &maker, &token, before_balance)` or `assert_events_have_no_cw20_transfer(events)` in test helpers. 2. New test **`clean_limit_book_emits_no_cw20_transfers`** (or extend `clean_limit_book_force_dust_bid_then_claim_refunds`): - Setup: force-dust config **or** time-expired head (either path parks ≥1 row). - Record maker CW20 balance + optional `PendingEscrow` query before clean. - Execute `CleanLimitBook` as permissionless keeper. - Assert: maker balance **unchanged**; no CW20 `transfer` events to maker in tx; `PENDING_ESCROW_*` unchanged; `ExpiredLimitRefund` row exists; **then** claim moves balance (existing assertion). 3. Optionally duplicate for **time-expired-only** path (`clean_limit_book_parks_expired_head_default_config` scenario) — one test covering both force + time paths is sufficient if both park helpers share code path. 4. Update **L15** test citation row in `docs/contracts-security-audit.md`. ### 2. Non-owner claim rejection (#264 context / L1 claim guard) 1. New test **`claim_expired_limit_order_non_owner_rejected`**: - Park a row (hybrid walk on expired bid, `CleanLimitBook`, or dust flush — simplest: reuse expired-bid hybrid park from existing tests). - `attacker` executes `ClaimExpiredLimitOrder { order_id }` → `Unauthorized`. - Assert: `ExpiredLimitRefund` row still present; maker balance unchanged; attacker balance unchanged. 2. Mirror structure of `cancel_limit_order_non_owner_rejected` and `batch_cancel_foreign_owner_reverts_whole_tx`. 3. **Stretch:** `batch_claim_expired_foreign_owner_reverts_whole_tx` with two parked ids owned by `env.user`, batch submitted by `attacker`. 4. Update **L1** / **L11** test citation rows if new test names are added. ## Acceptance criteria - [ ] Integration test proves **`CleanLimitBook` tx does not CW20-transfer to maker** (balance +/or event scan) while parked claim row exists. - [ ] Same test (or paired test) proves maker **can** claim afterward — refund deferred, not lost. - [ ] Integration test proves **non-owner** `ClaimExpiredLimitOrder` → `Unauthorized`; claim row and balances unchanged. - [ ] `make test-contracts` / `cargo test -p cl8y-dex-tests limit_order` green. - [ ] `docs/contracts-security-audit.md` invariant rows cite new test function names. ## Test plan — functional paths | Path | Expectation | |------|-------------| | Force-dust clean (#263 config) | Clean parks 1 row; zero CW20 to maker; claim refunds | | Time-expired clean (default 0/0 config) | Clean parks head; zero CW20 to maker; claim refunds | | Non-owner single claim | `Unauthorized`; row persists | | Owner claim after rejected attacker attempt | Still succeeds; single refund | | (Stretch) Batch claim foreign owner | Whole tx reverts; both rows persist | ## Test plan — attack / abuse / hack vectors | Vector | Verification | |--------|----------------| | Permissionless keeper steals escrow via clean | Clean tx: no CW20 out; pair balance − reserves − pending unchanged (**L1**) | | Attacker claims victim's parked dust (#264 flush) | Non-owner claim rejected; dust remains in `EXPIRED_LIMIT_CLAIMS` | | Attacker claims after force-clean (#263) | Same rejection | | Double refund (clean + claim by different senders) | Clean never pays; only owner claim transfers | | Premature pending decrement | `PENDING_ESCROW_*` unchanged across clean (query if helper exists) | | Batch claim partial grief | (Stretch) Foreign id in batch reverts all claims — no partial CW20 | ## Verification criteria - [ ] `cargo test -p cl8y-dex-tests clean_limit_book_emits_no_cw20` (or chosen name) passes. - [ ] `cargo test -p cl8y-dex-tests claim_expired_limit_order_non_owner_rejected` passes. - [ ] Grep audit doc: new tests listed under **L1**, **L15** (and **L11** if batch stretch landed). - [ ] No wasm code diff unless regression found (then split fix into separate MR).

PlasticDigits commented

2026-06-02 07:00:00 +00:00

(Migrated from gitlab.com)

marked as related to #263

PlasticDigits commented

2026-06-02 07:00:01 +00:00

(Migrated from gitlab.com)

marked as related to #264

Brouie commented

2026-06-02 12:45:13 +00:00

(Migrated from gitlab.com)

mentioned in merge request !734

Brouie commented

2026-06-02 12:45:35 +00:00

(Migrated from gitlab.com)

Knocked out #271 — both coverage gaps, tests-only, no wasm change. Branch qa/271-claim-clean-test-coverage off main, commit 6457bac, MR !734.

Three new tests in smartcontracts/tests/src/limit_order_tests.rs:

clean_limit_book_emits_no_cw20_transfers (#263 / L15)
claim_expired_limit_order_non_owner_rejected (#264 / L1)
batch_claim_expired_foreign_owner_reverts_whole_tx (L11, the stretch)

Acceptance criteria:

CleanLimitBook tx does NOT CW20-transfer to maker while a parked claim row exists -> clean_limit_book_emits_no_cw20_transfers checks it BOTH ways: event scan (no action=transfer to maker) AND maker balance flat across the clean tx. The scanner is self-checked against the real transfer in the claim tx, so the clean assertion can't pass vacuously.
Same test proves maker can claim afterward, refund deferred not lost -> owner claim pays exactly remaining. Paying the FULL remaining also proves PENDING_ESCROW was untouched by the clean (no premature decrement) — covers the "premature pending decrement" abuse row without a pending query (none is exposed).
Non-owner ClaimExpiredLimitOrder -> Unauthorized, claim row and balances unchanged -> claim_expired_limit_order_non_owner_rejected: attacker claim reverts Unauthorized; parked row still present with same remaining/owner; maker AND attacker balances unchanged; owner then claims fine (single refund).
make test-contracts / cargo test -p cl8y-dex-tests limit_order green -> make test-contracts 405 passed / 0 failed (was 402, +3). cargo test -p cl8y-dex-tests limit_order: 71 passed / 0 failed.
docs/contracts-security-audit.md rows cite new test fn names -> L1 cites claim_expired_limit_order_non_owner_rejected, L11 cites batch_claim_expired_foreign_owner_reverts_whole_tx, L15 cites clean_limit_book_emits_no_cw20_transfers.

Verification criteria:

cargo test -p cl8y-dex-tests clean_limit_book_emits_no_cw20_transfers passes
cargo test -p cl8y-dex-tests claim_expired_limit_order_non_owner_rejected passes
Grep audit doc: new tests listed under L1, L15, and L11 (batch stretch landed)
No wasm code diff — diff is exactly limit_order_tests.rs + contracts-security-audit.md, zero contract src touched. clippy -p cl8y-dex-tests --tests clean.

Functional paths covered: force-dust clean (zero CW20 -> claim refunds); non-owner single claim rejected; owner claim after rejected attacker attempt still pays once; (stretch) foreign-owner batch claim reverts whole tx, both rows persist.

Attack/abuse vectors covered: permissionless keeper steals escrow via clean -> no CW20 out in clean tx; attacker claims victim's parked dust -> rejected, row remains; attacker claims after force-clean -> same rejection; double refund (clean + claim by different senders) -> clean never pays, only owner claim transfers; premature pending decrement -> claim pays full remaining (proves pending intact); batch claim partial grief -> foreign id reverts all, no partial CW20.

Scope note: covered the force-dust clean path only. Both force-clean and time-expiry park through the same park_limit_order_for_clean path (L15/L16), so per the issue one test is sufficient — not skipping the time-expired path, it's the same code path.

Needs your review/merge on !734, then close. @PlasticDigits

Knocked out #271 — both coverage gaps, tests-only, no wasm change. Branch qa/271-claim-clean-test-coverage off main, commit 6457bac, MR !734. Three new tests in smartcontracts/tests/src/limit_order_tests.rs: - clean_limit_book_emits_no_cw20_transfers (#263 / L15) - claim_expired_limit_order_non_owner_rejected (#264 / L1) - batch_claim_expired_foreign_owner_reverts_whole_tx (L11, the stretch) Acceptance criteria: - [x] CleanLimitBook tx does NOT CW20-transfer to maker while a parked claim row exists -> clean_limit_book_emits_no_cw20_transfers checks it BOTH ways: event scan (no action=transfer to maker) AND maker balance flat across the clean tx. The scanner is self-checked against the real transfer in the claim tx, so the clean assertion can't pass vacuously. - [x] Same test proves maker can claim afterward, refund deferred not lost -> owner claim pays exactly `remaining`. Paying the FULL remaining also proves PENDING_ESCROW was untouched by the clean (no premature decrement) — covers the "premature pending decrement" abuse row without a pending query (none is exposed). - [x] Non-owner ClaimExpiredLimitOrder -> Unauthorized, claim row and balances unchanged -> claim_expired_limit_order_non_owner_rejected: attacker claim reverts Unauthorized; parked row still present with same remaining/owner; maker AND attacker balances unchanged; owner then claims fine (single refund). - [x] make test-contracts / cargo test -p cl8y-dex-tests limit_order green -> make test-contracts 405 passed / 0 failed (was 402, +3). cargo test -p cl8y-dex-tests limit_order: 71 passed / 0 failed. - [x] docs/contracts-security-audit.md rows cite new test fn names -> L1 cites claim_expired_limit_order_non_owner_rejected, L11 cites batch_claim_expired_foreign_owner_reverts_whole_tx, L15 cites clean_limit_book_emits_no_cw20_transfers. Verification criteria: - [x] cargo test -p cl8y-dex-tests clean_limit_book_emits_no_cw20_transfers passes - [x] cargo test -p cl8y-dex-tests claim_expired_limit_order_non_owner_rejected passes - [x] Grep audit doc: new tests listed under L1, L15, and L11 (batch stretch landed) - [x] No wasm code diff — diff is exactly limit_order_tests.rs + contracts-security-audit.md, zero contract src touched. clippy -p cl8y-dex-tests --tests clean. Functional paths covered: force-dust clean (zero CW20 -> claim refunds); non-owner single claim rejected; owner claim after rejected attacker attempt still pays once; (stretch) foreign-owner batch claim reverts whole tx, both rows persist. Attack/abuse vectors covered: permissionless keeper steals escrow via clean -> no CW20 out in clean tx; attacker claims victim's parked dust -> rejected, row remains; attacker claims after force-clean -> same rejection; double refund (clean + claim by different senders) -> clean never pays, only owner claim transfers; premature pending decrement -> claim pays full remaining (proves pending intact); batch claim partial grief -> foreign id reverts all, no partial CW20. Scope note: covered the force-dust clean path only. Both force-clean and time-expiry park through the same park_limit_order_for_clean path (L15/L16), so per the issue one test is sufficient — not skipping the time-expired path, it's the same code path. Needs your review/merge on !734, then close. @PlasticDigits

PlasticDigits commented

2026-06-02 14:21:12 +00:00

(Migrated from gitlab.com)

mentioned in commit 12308a3803

mentioned in commit 12308a3803962cb57308d8564be4f540d1ee9d93

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-06-02 14:21:12 +00:00

Brouie commented

2026-06-02 18:00:12 +00:00

(Migrated from gitlab.com)

mentioned in issue #246

Sign in to join this conversation.