Skip to content

cow: settle the idempotency seam on the venue-and-body intent-id - #466

Merged
mfw78 merged 1 commit into
dev/m1from
feat/m4-idempotency-seam
Jul 23, 2026
Merged

cow: settle the idempotency seam on the venue-and-body intent-id#466
mfw78 merged 1 commit into
dev/m1from
feat/m4-idempotency-seam

Conversation

@mfw78

@mfw78 mfw78 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

What

Keys the CoW submitted: journal on the deterministic venue-and-body intent_id (cow-venue::intent_id, wrapping the generic sweep's submission_key over the encoded CowIntentBody) instead of the client-computed order UID. The id is derived pre-submit from the same body bytes a venue submit carries, before OrderCreation is assembled. The OrderUid type lands here with its conversions and round-trip tests; the adapter wires it into SubmitOutcome's accepted receipt in #467. intent_id's rustdoc states what the key scopes to: a signed payload keys on its signature, so it covers that exact payload rather than the economic order. CowIntent gains a Signed variant for a conditional-order keeper's owner-signed order. videre-sdk::keeper::submission_key is now public so a keeper journalling outside Keeper::sweep writes the same key. twap-monitor and shepherd-sdk tests and docs follow the rename.

Why

Once order assembly moves into the venue adapter, the keeper can no longer derive a UID pre-submit, opening a double-post window across the keeper-to-adapter boundary. The venue-and-body intent-id is derivable before any network work and survives that move.

Testing

  • New unit tests: intent_id determinism and body-scoping, SignedOrder borsh round-trip, OrderUid conversions and Display.
  • Regression test: resubmit after restart asserts a single orderbook POST.
  • cargo fmt --all --check, cargo check --workspace --all-features, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo nextest run --workspace --all-features, cargo test --doc.

AI Assistance

Implemented with Claude Code.

Closes #396
Closes #558

@lgahdl lgahdl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent_id derivation itself is sound (deterministic, body-scoped, verified different Signed payloads produce different ids), and the restart-regression test genuinely simulates a fresh process (independent MockHost::default(), only the local-store snapshot carried across) rather than just single-process dedup. Three things worth addressing:

}
};
let journal = Journal::submitted(host);
if journal.contains(&intent_id)? {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the real production journal site (submit_ready), and it has the same non-atomic check-then-act shape as the gap I flagged on #454 (videre-sdk::keeper.rs's journal.contains/journal.record): contains(&intent_id) here, then record(&intent_id) later after the actual network submit — two separate host calls with no atomicity between them. Two overlapping sweeps on the same host could both pass contains before either records. This PR's framing ("closes the double-post window") is about the pre-submit-derivability problem (the keeper can't derive a UID once order assembly moves into the adapter) — it doesn't touch this concurrency/atomicity race, which is a different problem and predates this PR (it was already split on the old UID key). Worth flagging as still open rather than implicitly closed by this PR's title.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed still valid at the end-of-train tip, and you are right that this PR does not close it. It is already tracked as #538, filed off the same shape you flagged on #454.

The site moves. shepherd-sdk/src/cow/run.rs is deleted by #471, and this logic lands in composable-cow/src/sweep.rs, where the shape is intact at the tip: journal.contains(&intent_id)? at :133, the network submit at :138, journal.record(&intent_id) at :157. I have added that pointer to #538 so the tracker does not go stale against the carve.

One correction on the concurrency framing, which #538 already records in full. Two overlapping sweeps cannot interleave contains and record within an engine process, and it is not lock-based: the runtime moves one owned Supervisor into a single tokio task, the event loop selects one event and awaits dispatch_* to completion before the next, every dispatch and restart path takes &mut self, a timed-out guest call is dropped before dispatch_to returns, and restart replaces the wasmtime Store in place. Two engine processes over one redb state_dir is the only genuine race, and redb is single-process anyway.

Severity is also lower on this L3 CoW path than on the L2 keeper site #538 was filed against, because the venue backstops it. If the record write faults here the sweep logs and carries on by design (:153-159), the next tick re-posts, the orderbook answers with a duplicate error type, classification::is_already_submitted maps it to Refusal::AlreadyHeld (cow-venue/src/adapter.rs:391), post_order returns Posted::AlreadyHeld (:325), and the adapter answers SubmitOutcome::Accepted with assembly::order_uid(chain, order, owner) (:172). The journal write then succeeds on that pass, so the gap self-heals at a cost of one redundant POST rather than a duplicate order.

The L2 videre-sdk keeper site is the one that still bites, since it propagates Err after a submitted-but-unrecorded body and has no venue-side backstop. That remains #538's target, with the decided fix being reserve/commit/release over the existing verbs and no WIT change.

/// An owner-signed order ready for the orderbook: what a
/// conditional-order keeper emits after a poll.
#[derive(BorshSerialize, BorshDeserialize, Clone, Debug, PartialEq, Eq)]
pub struct SignedOrder {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SignedOrder derives BorshSerialize over the whole struct including signature: Vec<u8>, and intent_id hashes the full encoded body — so the signature bytes are part of the dedup key. That's fine for collision-safety, but it means a legitimate re-sign of the same underlying order (same order+owner, fresh EIP-1271 signature after e.g. a keeper restart mid-flight, before the first attempt's journal write landed) produces a different intent_id and defeats dedup rather than falsely triggering it — the opposite failure mode from what this PR is guarding against. Concrete scenario: keeper signs, crashes before journaling, restarts, re-signs the same order, and if the first submit actually reached the orderbook, the second (differently-keyed) attempt isn't recognized as a duplicate. Worth either hashing (venue, owner, order) and excluding signature, or documenting explicitly that this key scopes to "this exact signed payload," not "this economic order."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact is confirmed at the end-of-train tip: the sweep builds CowIntentBody::V1(CowIntent::Signed(SignedOrder { order, owner, signature })) (composable-cow/src/sweep.rs:120) and intent_id hashes the whole encoded body, so the signature bytes really are in the dedup key. Filed as #558.

The concrete scenario does not arise on this path though, for two independent reasons, both checked rather than assumed.

The keeper never signs. submit_ready receives the signature from source.poll's Verdict::Post { order, signature, .. } (composable-cow/src/sweep.rs:60), so the bytes come from the conditional-order source rather than being generated per attempt. For ComposableCoW that is the on-chain proof, and a restart re-reads the same deterministic bytes for the same order and params. There is no "re-sign" step in the loop to produce fresh bytes.

Even a genuinely different signature costs only one redundant POST. The orderbook's identity for an order is signature-independent: assembly::order_uid is order.uid(&chain.settlement_domain(), owner). So a differently-keyed second attempt at the same economic order still lands on the duplicate path, classification::is_already_submitted maps it to Refusal::AlreadyHeld (cow-venue/src/adapter.rs:391), and the adapter answers SubmitOutcome::Accepted with the derived canonical uid (:172). It comes back accepted, not double-posted.

So the failure mode is milder than "defeats dedup": dedup degrades to the venue's own idempotency rather than breaking. What genuinely remains is your second option, documenting that the key scopes to this exact signed payload and not to the economic order, plus the latent hazard that the argument above leans on a venue whose duplicate response is idempotent. #558 carries both, with documenting as the recommendation and excluding the signature from the key as the alternative.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revisited this and folded it into the PR rather than leaving it to the tracker, taking your documentation option.

intent_id's rustdoc now says the key covers the encoded body, so a signed payload keys on its signature and scopes to that exact payload rather than the economic order, which dedups only through the venue's duplicate response.

Folding was safe on this car specifically: cow-venue/src/client.rs is touched by no other open car in the train, and the only later change is #479's grouping move, a pure R100 rename with a zero-byte content delta.

The analysis above still stands on why the runtime behaviour is sound today, so this is a precision fix to the contract rather than a behaviour change. #558 stays open only to record the alternative (keying on (venue, owner, order)) should a venue ever be added whose duplicate response is not idempotent, and #466 now carries Closes #558.

/// The CoW venue marker: every [`CowClient`] call routes to
/// [`Venue::ID`] and encodes a [`CowIntentBody`].
/// [`Venue::ID`] and encodes a [`CowIntentBody`]. An accepted submit's
/// receipt is the canonical [`OrderUid`](crate::OrderUid) in wire form.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment (and the PR body) states "SubmitOutcome's accepted receipt is now the canonical 56-byte OrderUid," but nothing in this diff actually wires OrderUid into submit_order's return type or SubmitOutcome — neither symbol is touched here. OrderUid is added with solid conversions and round-trip tests, but it's constructed only in this PR's own unit tests; no production call site builds one from a real submit response yet. Worth either wiring it into the real return path in this PR, or scoping this comment/the PR description to "the type is added, integration follows" so it doesn't read as already-shipped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct as written, and the diff confirms it: this PR adds exactly that one doc line to client.rs and touches neither submit_order nor SubmitOutcome. OrderUid is built only in this PR's own unit tests.

The wiring lands in the very next car, #467 (feat/m4-cow-adapter-cdylib), which adds cow-venue/src/adapter.rs. There post_order decodes the orderbook's uid from the success response and submit returns SubmitOutcome::Accepted(uid.as_slice().to_vec()) (:174), the already-held path falls back to the derived assembly::order_uid rather than inventing a receipt (:172), the status path rejects anything that is not a 56-byte order uid (:203-204), and the tests assert receipt == uid.as_slice(). So the claim is accurate one car later, and byte-identical at the end-of-train tip where it holds.

I have left the code comment alone rather than spend a re-push and a full CI run on a line that is true from the next car onward, and scoped the PR body instead, which is where it read as already shipped. If you would rather the comment itself carry the "type added here, integration in #467" hedge for the duration of one car, say so and I will fold it into the next ripple.

@mfw78
mfw78 force-pushed the feat/m4-grant-divergence branch from 09ad5b8 to 801b294 Compare July 23, 2026 04:35
Base automatically changed from feat/m4-grant-divergence to dev/m1 July 23, 2026 04:37
@mfw78
mfw78 force-pushed the feat/m4-idempotency-seam branch 2 times, most recently from dbadb56 to f82f4a0 Compare July 23, 2026 04:47
The submitted: journal now keys on the deterministic intent-id (the
generic sweep's venue-and-body submission key over the encoded
CowIntentBody), derived pre-submit without assembling OrderCreation.
SubmitOutcome's accepted receipt is fixed as the canonical 56-byte
order UID (OrderUid), and the CowIntent schema gains the Signed kind
a conditional-order keeper emits. A regression test covers resubmit
after restart with a single orderbook POST.
@mfw78
mfw78 force-pushed the feat/m4-idempotency-seam branch from f82f4a0 to fb5d1a8 Compare July 23, 2026 05:11
@mfw78
mfw78 merged commit 93994bc into dev/m1 Jul 23, 2026
7 checks passed
@mfw78
mfw78 deleted the feat/m4-idempotency-seam branch July 23, 2026 05:20
mfw78 added a commit that referenced this pull request Jul 27, 2026
The submitted: journal now keys on the deterministic intent-id (the
generic sweep's venue-and-body submission key over the encoded
CowIntentBody), derived pre-submit without assembling OrderCreation.
SubmitOutcome's accepted receipt is fixed as the canonical 56-byte
order UID (OrderUid), and the CowIntent schema gains the Signed kind
a conditional-order keeper emits. A regression test covers resubmit
after restart with a single orderbook POST.
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