cow: build the cow venue adapter component with bounded request timeouts - #467
Conversation
lgahdl
left a comment
There was a problem hiding this comment.
Thorough review given the stakes here (real funds/orders flowing through this eventually) — the vast majority of this checks out clean: EIP-1271 signature attachment is correctly wired to the same decoded body throughout, the pre-sign flow's requires-signing carries the real contract address/UID/calldata with no path that marks an order locally 'live' before the host actually sends the tx, TimedFetch genuinely clamps every adapter request with no bypass found, errorType classification correctly preserves retry hints through the collapse and reuses the already-ratified table from #464 unchanged, the assembly slice move is a pure relocation with the legacy keeper and new adapter now genuinely sharing one function rather than two copies, wasi:http is scoped to exactly api.cow.fi, and no unsafe unwraps exist outside test code. A few things worth a look:
| .map_err(|e| VenueError::InvalidBody(e.to_string()))?; | ||
| let uid = match post_order(fetch, config, &creation)? { | ||
| Posted::Accepted(uid) => uid, | ||
| Posted::AlreadyHeld => assembly::order_uid(config.chain, &order, owner), |
There was a problem hiding this comment.
On Posted::AlreadyHeld, the returned UID is computed purely client-side (assembly::order_uid) with nothing from the server to cross-check it against — the CoW API's 409/already-held response carries no UID in its body by design, so this is the only lever available. This is inherent to the "already-held is success" design, not something this diff introduced a bug into, but it's worth having status_with (called right after, in the same flow) hard-fail loudly if the returned order's sell/buy token or validTo don't match the just-submitted body, rather than trusting the locally-derived UID alone all the way through. Same consideration applies to the Accepted branch just above (line 171) — nothing asserts the orderbook's actual stored order matches what was locally derived, which would catch an orderbook/library UID-derivation drift immediately instead of silently trusting the server.
There was a problem hiding this comment.
Confirmed still valid at the end-of-train tip, and agreed it is a trust gap rather than a bug this diff introduced. Filed as #559.
One correction to the suggested fix, which changed the shape of the tracker. status_with cannot do this comparison as written: it receives only the 56-byte receipt, and it deliberately deserializes a single field (struct OrderStatusView { status: OrderStatus }, commented "the one server field the lifecycle projection reads"). So there is nothing to compare the tokens or validTo against without threading the expected order into the status path, which is an interface change rather than a guard added to the existing read.
#559 therefore carries three options: verify at submit time with one extra GET, verify at status time by widening both the view and the signature, or the cheap one that I think should happen regardless, which is asserting the server's returned UID against assembly::order_uid(...) on the Accepted branch where both values are already in hand. That last one catches exactly the derivation-drift case you are worried about at zero request cost and with no signature change, though it cannot cover AlreadyHeld, where no server UID exists at all.
| impl Refusal { | ||
| /// Collapse for call sites where already-held is not a success | ||
| /// shape (reads); the orderbook only emits it on submission. | ||
| fn into_error(self) -> VenueError { |
There was a problem hiding this comment.
already_held is correct as success for submit, but this into_error() exists specifically because status/quote reads must NOT treat it as success — nothing at the type level forces a future call site to pick the right interpretation if someone adds a new read-path call to refusal() and forgets .into_error(). Consider splitting into refusal_for_submit/refusal_for_read (or renaming) to make the caller's obligation explicit rather than relying on every future caller remembering which context they're in.
There was a problem hiding this comment.
Confirmed still valid at the end-of-train tip. Filed as #560.
Worth recording that all three production call sites are currently correct: quote_with and status_with both take refusal(&response).into_error(), and submit_with is the single site that matches on the variant and treats AlreadyHeld as success. So it is a foot-gun for the next read-path caller rather than a live defect.
#560 takes your first suggestion as the recommendation, splitting into refusal_for_submit returning Refusal and refusal_for_read returning VenueError directly, so a read path can never hold an unresolved already-held meaning. I have not folded it into this car: #473 also edits cow-venue/src/adapter.rs, so doing it here would just conflict when that car is rippled. It should land after #473, or inside #473's own pass.
dbadb56 to
f82f4a0
Compare
f82f4a0 to
fb5d1a8
Compare
39f178a to
a12853a
Compare
a12853a to
6bc2bce
Compare
What
Adds the
adapterfeature slice tocow-venue:CowAdapterunder#[videre_sdk::venue]decodesCowIntentBody, derives the value-flow header, and speaks the orderbook REST API over scopedwasi:http. A signed order posts EIP-1271 (receipt is the canonical 56-byte UID); an unsigned order posts pre-sign and returnsrequires-signingcarrying thesetPreSignaturecall. An already-held rejection succeeds with the client-derived UID;errorTyperejections project ontovenue-errorthrough the classification table so the retry hint survives the collapse.Chain-edge order assembly (
gpv2_to_order_data,order_data_to_body,order_uid_hex,build_order_creation) moves into a newassemblyslice the adapter owns;shepherd-sdkre-exports it for the legacy keeperrun(), otherwise unchanged.videre-sdkgainsBoundedFetch, which caps the wasi:http connect, first-byte and between-bytes timeouts of every adapter request; the adapter's default bound isnexum_sdk::http::DEFAULT_TIMEOUT, overridable per instance via thehttp-timeout-msconfig key. Codec vectors and header goldens ship undertests/vectorswith a conformance suite. The built component bundles into the shepherd distribution: engine adapters stanza, Docker/CI wasm build, justfile target.Closes #324.
Why
Routes outbound orderbook calls through typed transport wrappers carrying a per-request timeout, resolving #41 in the adapter rather than patching
cow_orderbook.rs. The bound is applied by clamping wasi:http's ownrequest-optionsphase timeouts, not by any guest-side timing. The adapter is thevidere:adapter/venue-adaptercomponent the CoW-on-videre split plan calls for; the legacy keeper path stays onCowApiHostbut now shares the assembly slice. Pool-router wiring is a later port.Testing
cargo nextest run -p cow-venue -p videre-sdk -p shepherd-sdkcargo test --doccargo build --release --target wasm32-wasip2 -p cow-venue --features adaptercargo fmt --all --check,cargo clippy --workspace --all-targets --all-features -- -D warningsAI Assistance
Implemented with Claude Code.