refactor(order): resolve order caller via UserAccount

[mbjorkqvist]

Purpose

Follows up on @gregorydemay's review of #226. The order entry points resolved the caller through the legacy bare-Principal effective_account helper and then re-derived "is this a delegated key" with a (caller != owner) comparison — a pattern predating the funding/trading account distinction. This PR moves caller resolution into the State order methods so both add_limit_order and cancel_limit_order take the raw caller and resolve owner and acting-key attribution from the richer UserAccount type.

This is a behavior-preserving refactor: no functional change, no spec change, no candid/CBOR change. Owner and attribution (placed_by/canceled_by) are identical for all three cases (funding, trading, unknown caller).

Approach

• add_limit_order and cancel_limit_order each look the caller's UserAccount up once and pass it into their validate method. The validate methods act directly on that Option<UserAccount>, deriving the owner via effective_principal(); the entry points read placed_by/canceled_by from the same account via order_actor(). The two paths are symmetric, and no separate caller-resolution helper is needed.
• Attribution is driven by the UserAccount variants (funding → self / None, trading → funding principal / Some(acting key), unknown → self / None), not by a caller != owner comparison.
• lib.rs stays thin: it keeps the caller-allowed check on the raw caller and the candid→domain parsing, then makes a single state call.
• The record_* methods keep their owner-Principal signatures. They are shared with the post-upgrade replay path, which reconstructs them from the persisted event's already-resolved owner and has no caller/UserAccount to resolve — resolution is a live-only decision that the audit log freezes at submission time.

📚 PR stack

• feat(order): resolve order cancellation to the funding account (DEFI-2911 PR 6/6) #226 — feat: resolution on cancel (DEFI-2911 PR 6) — ✅ merged
• this PR — refactor order caller resolution via UserAccount ← you are here

Retargeted to main now that #226 has merged.

[github-actions]

canbench 🏋 (dir: canister) e83d139 2026-07-31 08:38:44 UTC

✅ canister/canbench_results.yml is up to date
📦 canbench_results_benchmark.csv available in artifacts

---------------------------------------------------

Summary:
  instructions:
    status:   No significant changes 👍
    counts:   [total 16 | regressed 0 | improved 0 | new 0 | unchanged 16]
    change:   [max +2.45M | p75 +10 | median 0 | p25 -12.12K | min -141.08M]
    change %: [max +0.20% | p75 0.00% | median 0.00% | p25 -0.18% | min -0.39%]

  heap_increase:
    status:   No significant changes 👍
    counts:   [total 16 | regressed 0 | improved 0 | new 0 | unchanged 16]
    change:   [max 0 | p75 0 | median 0 | p25 0 | min 0]
    change %: [max 0.00% | p75 0.00% | median 0.00% | p25 0.00% | min 0.00%]

  stable_memory_increase:
    status:   No significant changes 👍
    counts:   [total 16 | regressed 0 | improved 0 | new 0 | unchanged 16]
    change:   [max 0 | p75 0 | median 0 | p25 0 | min 0]
    change %: [max 0.00% | p75 0.00% | median 0.00% | p25 0.00% | min 0.00%]

---------------------------------------------------
CSV results saved to canbench_results.csv

[mbjorkqvist]

🧐 VERDICT: READY — 0 blockers, 0 mediums, 0 nits; CI green.

Review details

Behavior-preserving refactor, confirmed by equivalence + mutation:

• Funding: order_actor → (principal, None); since the funding account is keyed by caller, principal == caller, so old (effective_account, (caller != owner).then_some(caller)) → (caller, None). Identical.
• Trading: order_actor → (grant.funding, Some(principal)); a trading key is never its own funding (SelfGrant prevented), so old code's caller != owner is always true → Some(caller). Identical.
• Unknown: unwrap_or((caller, None)) matches old effective_account's unwrap_or(caller) + then_some(None). Identical.

Mutation of the Trading branch to drop attribution failed resolution_on_placement::should_place_a_trading_account_order_on_the_funding_account and resolution_on_cancel::should_cancel_the_funding_account_order_and_attribute_the_acting_caller — both sides of the axis are guarded end-to-end through the productive API. Reverted; tree clean.

Scope: only add_limit_order / cancel_limit_order changed. effective_account is correctly retained (5 read-path sites in lib.rs). No candid/CBOR/spec change.

Maintainability:

• Duplication: none material. resolve_order_caller mirrors effective_account and order_actor mirrors effective_principal, but each is 3 lines returning semantically distinct data (read-resolution principal vs. owner+attribution) — intentional, sub-threshold sibling parallelism, not copy-paste worth unifying.
• Unused derives: N/A (no new types).
• Primitive-obsession: the (Principal, Option<Principal>) return is a two-Principal tuple, mildly swap-prone, but consistent with the codebase's bare-Principal owner/placed_by convention and immediately destructured at both call sites — cleared.
• Divergent invariant handling: none; unknown caller degrades to (caller, None) exactly as effective_account degrades to caller.
• Silent fallbacks: none; the unwrap_or covers the expected unregistered-caller case (downstream ownership check rejects foreign callers, per should_reject_cancel_by_a_foreign_caller), not an invariant breach.

Tests: no redundant tests added; existing resolution_on_placement / resolution_on_cancel table-driven tests and lifecycle coverage carry the safety net. Docs added on both new methods are accurate; no requirement-ID tags.

[mbjorkqvist]

✅ Ready for your review. Behavior-preserving refactor — add_limit_order + cancel_limit_order now resolve the caller via UserAccount (resolve_order_caller / UserAccount::order_actor) instead of the bare-Principal effective_account + (caller != owner).then_some(caller) idiom, per @gregorydemay's #226 review.

• Reviewer verdict: READY (0 findings; behavior-equivalence mutation-checked across funding/trading/unknown callers).
• CI fully green; mergeable: MERGEABLE.
• Scope: only the two order paths; read paths keep effective_account; no candid/CBOR/spec change. Existing resolution_on_placement/resolution_on_cancel + attribution + lifecycle tests pass unchanged.

📚 Stacked on #226 (base = mathias/DEFI-2911-resolution-on-cancel). When #226 merges I'll retarget this to main. Left as a draft for your review/merge.

[mbjorkqvist]

🧐 🔵 Nit (non-gating): order_actor().0 is always equal to effective_principal(), and State::resolve_order_caller mirrors effective_account line-for-line — the order-path pair is a superset of the read-path pair. Not worth unifying here (they're 4–5 line accessors and coupling the read-path helper to order attribution semantics would arguably read worse), but flagging the parallelism in case a future change makes it cheap to collapse (e.g. effective_principal = self.order_actor().0). Also: order_actor is pub but currently only used within the crate; that matches effective_principal's visibility, so no change needed.

[mbjorkqvist]

🧐 VERDICT: READY — 0 blockers, 0 mediums, 1 nit; CI green.

Review details

Behavior-preserving refactor that moves order caller resolution into the State
methods, resolving (owner, placed_by/canceled_by) via the private
resolve_order_caller (UserAccount-driven). Confirmed against the four verify points:

• Behavior-preserving. order_actor().0 == effective_principal() for both variants,
  and the acting-key mapping (funding→None, trading→Some(trading), unknown→None)
  reproduces the old (caller != owner).then_some(caller) exactly. Mutation check:
  breaking order_actor's trading acting-key fails both resolution_on_placement and
  resolution_on_cancel — the moved logic is covered by evidence.
• R12 preserved. assert_caller_is_allowed still runs on the raw caller, before any
  resolution, in both lib.rs entry points.
• Addresses the request. State methods now resolve via UserAccount; lib.rs passes
  only the raw caller; the legacy bare-Principal pre-resolution is gone from the order
  paths.
• Scope. Only the two sync order paths changed; deposit/withdraw untouched; no
  candid/CBOR/spec change; add_limit_order/cancel_limit_order are symmetric.

Maintainability rundown:

• duplication (in-diff): none substantial. resolve_order_caller/effective_account and
  order_actor/effective_principal are near-parallel 4–5 line pairs, but they express
  distinct concepts (order path vs read path) and stay below the substantial bar — see nit.
• unused derives: none (no new types).
• primitive-obsession params: none — caller: Principal, pair: TradingPair,
  pending: PendingOrder are all domain types.
• divergent invariant handling: none — unknown-caller fallback (caller, None) matches
  effective_account's unwrap_or(caller).
• silent fallbacks: none — the unwrap_or((caller, None)) is an expected case (unknown
  principal acts as itself), not an invariant breach; mirrors prior behavior.
• test-only code in production: none.
• redundant/derivable params: improved — the old cancel_limit_order(owner, canceled_by, …)
  threaded two derivable args; both are now derived inside. This is the decision-ownership
  fix the refactor set out to make.

Tests: no redundant additions; the changed-signature call sites and the table-driven
resolution_on_* tests cover both axes (funding/trading) end-to-end for owner, status,
attribution, and the emitted event.

[mbjorkqvist]

✅ Ready for your review. Order-path caller resolution now happens inside the State methods via UserAccount — add_limit_order and cancel_limit_order take the raw caller and resolve through UserAccount::order_actor (a match on the funding/trading variants); lib.rs passes only caller and the legacy bare-Principal effective_account pre-resolution is gone from the order paths. Addresses @gregorydemay's #226 note.

• Reviewer verdict: READY — behavior-preserving (equivalence + mutation-checked across funding/trading/unknown callers), R12 preserved (raw-caller assert_caller_is_allowed first).
• CI fully green on fb32f8d; mergeable: MERGEABLE.
• State::add_limit_order is symmetric with the existing State::cancel_limit_order; deposit/withdraw untouched; no candid/CBOR/spec change; existing resolution/attribution/lifecycle tests pass unchanged.
• Accepted non-gating nit: the read paths (get_balances/get_my_orders/get_my_trades) still use effective_account, so it's retained — leaving parallel resolution helpers. Unifying the reads onto UserAccount (which would let us delete effective_account entirely) is deferred as a possible separate follow-up.

📚 Stacked on #226. When #226 merges I'll retarget this to main. Left as a draft for your review/merge.

[Copilot]

Pull request overview

This PR refactors order placement/cancellation so the State layer resolves the raw caller into (owner, acting_key) using UserAccount, removing the legacy effective_account + (caller != owner) attribution logic from the canister entrypoints while preserving behavior.

Changes:

• Add UserAccount::order_actor() to derive (owner, placed_by/canceled_by) directly from account classification.
• Introduce State::add_limit_order(caller, ...) and update State::cancel_limit_order(caller, ...) to resolve caller/attribution internally via a shared helper.
• Simplify canister/src/lib.rs order entrypoints to parse inputs and delegate to State with the raw caller; update affected state tests for the new signature.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
canister/src/user/mod.rs	Adds order_actor() to derive order owner + attribution from UserAccount variants.
canister/src/state/mod.rs	Moves caller resolution into state order methods; adds add_limit_order orchestration and a shared resolve_order_caller helper.
canister/src/lib.rs	Thins entrypoints by delegating order placement/cancel logic to State using the raw caller.
canister/src/state/tests.rs	Updates call sites to the new cancel_limit_order(caller, ...) signature.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[zeropath-ai]

✅ No security or compliance issues detected. Reviewed everything up to ab6870b.

Security Overview

• 🔎 Scanned files: 9 changed file(s)
• 🔗 Scan Link: https://zeropath.com/app/repositories/a2241de7-cf71-4864-8711-25a8fbfe66c5?scanId=92a163ed-ec2d-44df-8e5d-2b7a00d69a5e&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Enhancement	► canister/src/state/mod.rs     Add new add_limit_order workflow with auditing integration ► canister/src/state/mod.rs     Refactor validate_limit_order to use optional account and funding path ► canister/src/state/mod.rs     Update cancel_limit_order to pass caller-based account and remove owner/canceled_by parameters
Enhancement	► canister/src/lib.rs     Integrate new add_limit_order path and adjust cancel_limit_order to use new parameter set
Enhancement	► canister/src/state/audit/tests.rs     Adjust tests to use account lookups for validating limit orders
Enhancement	► canister/src/dashboard/tests.rs     Update test to resolve account via lookup_account before validate_limit_order
Enhancement	► canister/src/state/snapshot/tests.rs     Update should_roundtrip tests to use account lookup for validate_limit_order
Enhancement	► canister/src/state/tests.rs     Update tests to pass account reference from lookup_account into validate_limit_order
Enhancement	► canister/src/state/audit/tests.rs     Modify test setup to use account lookup for owner/funding relationships in limit order validation
Enhancement	► canister/src/user/mod.rs     Add order_actor to determine ownership attribution for placements/cancels

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[gregorydemay]

One understanding question

[gregorydemay]

nit: here for a lack of a better place: PR description formatting is weird (probalby claude used git commit message width)

[mbjorkqvist]

Good catch — reflowed the description into normal paragraphs (dropped the ~72-col hard wrapping that came from a commit-message-width editor) and refreshed it for the current shape of the PR, since the resolve_order_caller indirection you asked about below is now gone. Thanks!

[gregorydemay]

I don't get this new indirection via resolve_order_caller. Why not just have the validate method act directly on UserAccount (result of `UserRegistry::lookup)?

[mbjorkqvist]

Good call — dropped resolve_order_caller entirely. Both validate_limit_order and validate_cancel_limit_order now act directly on the UserAccount from lookup: each takes the Option<&UserAccount> and derives the owner via effective_principal(), and the add/cancel entry points read placed_by/canceled_by from the same account via order_actor(). The two paths are symmetric now.

No caller fallback is threaded into validate either — an unregistered caller resolves to None, which has no funding balance and owns no orders, so None already determines the outcome (InsufficientBalance / NotOrderOwner) without needing the principal.

The one place that stays owner-Principal-based is record_limit_order / record_cancel_limit_order: they're shared with the post-upgrade replay path, which reconstructs them from the persisted event's already-resolved owner and has no caller/UserAccount to resolve. Resolution is a live-only decision the audit log freezes at submission time (re-resolving at replay against mutable grant state could diverge from the original owner), so those keep taking the resolved principal.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

[mbjorkqvist]

This is behavior-preserving on every reachable state, because an order's owner is always a registered funding principal. Orders are only ever created through record_limit_order, which enforces exactly that: lookup(user).and_then(funding_id).expect("order owner not registered — deposit registers every user") (mod.rs:299). So if caller == record.owner, the caller is registered, lookup returns Some, and effective_principal() == caller == record.owner → the check passes. The only way to hit the None branch is an unregistered caller, which by that invariant can never equal record.owner — so None != Some(record.owner) → NotOrderOwner is the correct "not the owner" answer, never a false rejection of a genuine owner.

The old effective_account self-fallback only produced owner == caller in that same unregistered case, so it too would have rejected here — the paths agree. The invariant also holds across upgrades: order_history and user_registry are both stable and replayed from the same event log, where a Deposit registers the user before any of their AddLimitOrder events.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

canister/src/state/mod.rs:382

• validate_cancel_limit_order treats account: None as "not owner" unconditionally. Previously, unknown callers were treated as acting on their own principal via effective_account(caller) -> caller, so an (unregistered) principal could still cancel an order it owns. This is also a mismatch with the PR description’s "unknown → self / None" behavior. To preserve prior semantics, the owner comparison needs a fallback to the raw caller when no UserAccount exists (e.g., by passing caller into validate_cancel_limit_order, or computing an effective_owner = account.map(...).unwrap_or(caller) before validating).

    fn validate_cancel_limit_order(
        &self,
        account: Option<&UserAccount>,
        order_id: &OrderId,
    ) -> Result<(), CancelLimitOrderError> {
        let record = self
            .order_history
            .get(order_id)
            .ok_or(CancelLimitOrderError::OrderNotFound)?;
        if account.map(UserAccount::effective_principal) != Some(record.owner) {
            return Err(CancelLimitOrderError::NotOrderOwner);
        }

canister/src/state/mod.rs:247

• validate_limit_order re-looks up the funding account even when account is already a UserAccount::Funding (and thus already contains the UserId). This adds an unnecessary registry lookup and makes the flow harder to follow. Consider extracting the funding UserId directly from account and only doing a registry lookup for the trading-account case.

        let free = account
            .map(UserAccount::effective_principal)
            .and_then(|owner| self.user_registry.lookup(owner))
            .and_then(|funding_account| funding_account.funding_id())
            .and_then(|u| self.balances.get_balance(u, &token))
            .map(|b| *b.free())
            .unwrap_or(Quantity::ZERO);