cow: retire the legacy cow-api host cone - #471
Conversation
lgahdl
left a comment
There was a problem hiding this comment.
This is the culmination of the whole venue-platform split tracked across this entire review session, so a few notes at that scale before the line-level findings:
- Process concern: bundling total cone deletion (4242 deletions) with the stop-loss migration and a new enforcement gate into one PR removes the ability to bisect a regression to one of those independently-risky changes. This session found real defects in nearly every predecessor PR in this train — a single atomic diff here is higher-risk for review surface, even if it's the right call for runtime cutover safety. Worth landing this shape as separate PRs next time (migration first, proven stable, then delete).
- Scope clarity: this closes the ADR-0010 (venue-as-adapter-component) half of the original design from #132 cleanly — but grep confirms zero references to
EgressGuard/AllowAllGuardanywhere in this diff. ADR-0012's egress guard pipeline remains the advisory no-op flagged back in #433's review, entirely untouched. Worth being explicit that this PR closes the adapter-boundary half only; the egress-policy half is still open, not implied as done by "cone retirement."
On the code itself: deletion completeness checked out clean (workspace members, README, extensions.toml, engine configs, justfile all consistently updated, no dangling load-time reference found), stop-loss's migration avoids the signature-inclusive dedup bug from #468/#469 (it builds an unsigned order with no re-sign path, so that specific defect doesn't apply here), the journal check-then-act race is inherited unchanged from nexum-sdk::keeper::Journal (untouched by this diff, not newly introduced), and the new wit_layering.rs gate is actually correctly scoped this time — unlike the similar zero-leak scripts in #432/#449/#450/#462, it doesn't have a stale-pattern blind spot (though it's WIT-text-only, so a future Rust-level import of a CoW-specific type from a generic crate with no WIT reference would slip past it — worth pairing with a dependency-graph check eventually).
One real regression in test rigor worth a look:
| ConditionalOrderCreated::SIGNATURE_HASH, | ||
| events::CONDITIONAL_ORDER_CREATED.topic0, | ||
| "sol! topic-0 must match the shepherd:cow/cow-events pin", | ||
| fn topic0_matches_the_cow_events_package_of_record() { |
There was a problem hiding this comment.
The old test compared two independently-sourced values for equality (ConditionalOrderCreated::SIGNATURE_HASH from the sol! macro vs. events::CONDITIONAL_ORDER_CREATED.topic0 from the WIT-derived constant) — a real cross-check that would catch drift between the two sources. This rewrite computes expected from the same SIGNATURE_HASH source and only checks that hex string appears as a substring somewhere in the raw WIT file text — not a parsed/typed comparison against an independent second value. If the WIT-derived constant this used to check against drifted from SIGNATURE_HASH while the WIT file's text still happened to contain the old hex string anywhere (even in a stale comment), this test would still pass. This may be a necessary consequence of the events module being deleted along with the cow client cone rather than a deliberate weakening, but worth restoring an independent second source for the comparison if one is available (e.g. parsing the WIT file's actual const/comment value rather than a raw substring match).
There was a problem hiding this comment.
Agreed, and fixed in this car. You are right that it was a real loss of rigour rather than a cosmetic rewrite: deriving expected from SIGNATURE_HASH and then substring-matching the whole file means the assertion can only fail if the hash is absent from the text entirely, which a stale comment elsewhere would mask.
The independent second source is still available, just no longer in Rust: the cow-events WIT pins both a signature: and a topic0: per variant. The test now parses those out of the doc block belonging to that specific variant and compares typed values, rather than scanning raw text.
let (signature, topic0) = wit_event_pin(wit, "conditional-order-created");
assert_eq!(signature, ConditionalOrderCreated::SIGNATURE, ...);
assert_eq!(topic0, ConditionalOrderCreated::SIGNATURE_HASH, ...);wit_event_pin walks the file, keeps the most recent signature:/topic0: pins, and returns them when it reaches the named variant, so a hash sitting anywhere else in the file cannot satisfy it. Parsing the topic0 into a B256 also means a truncated or malformed pin fails loudly instead of silently not matching.
Verified in both directions rather than just observing a green tick. It passes as written, which incidentally proves ConditionalOrderCreated::SIGNATURE is byte-identical to the WIT's declared signature, so that second assertion carries weight too. Then I mutated the WIT's topic-0 by a single hex digit and confirmed the test fails with a typed left/right mismatch, which is exactly the drift the old substring form would have waved through.
There was a problem hiding this comment.
Following up on my own fix, because reviewing it properly showed I had put the rigour in the wrong place.
The WIT parity assertion is weaker than either of us framed it. The topic0: pins in cow-events.wit are /// doc comments consumed by nothing, and SIGNATURE_HASH is derived by sol! as keccak of SIGNATURE. So a typo in the sol! declaration produces a self-consistent wrong hash, this test fails, and the natural fix is to paste the new hash into the WIT, which silences the alarm while shipping the bug. The WIT is a hand-maintained mirror, not an independent oracle, so the test cannot detect the failure mode its own doc comment claimed.
Meanwhile the assertion that does have runtime teeth was carrying exactly the substring weakness you flagged, and neither of us looked at it: manifest.contains(&expected) over module.toml. That one is load-bearing, because nexum-runtime::supervisor feeds event_signature to build_alloy_filter, so a drift there subscribes to one topic and decodes another and the module silently sees nothing in production.
So I have rebalanced rather than piling more onto the weaker test. The manifest assertion now parses module.toml, finds the chain-log subscription, reads its actual event_signature and compares it as a typed B256, so a stale hash in a comment cannot satisfy it. Verified by mutating the pin by one hex digit and confirming failure. The WIT assertion stays as cheap documentation parity, with its doc comment corrected to say what it actually guarantees and to point at the subscription pin as the one with teeth.
For completeness on the "independent second source" question: there is not one available in-repo today. The cowprotocol dependency does not expose ConditionalOrderCreated or the ComposableCoW ABI, and the committed vectors are header and intent-body fixtures rather than on-chain logs. A genuine oracle would need a committed ComposableCoW ABI or a captured log fixture decoded end to end. Happy to file that if you think it is worth the machinery.
There was a problem hiding this comment.
Final disposition: mfw78 called it, and the WIT parity assertion is deleted rather than strengthened.
The reasoning is the one in my previous reply: cow-events.wit's topic0: values are doc comments consumed by nothing, and SIGNATURE_HASH is derived from SIGNATURE, so the WIT can only ever mirror the decoder rather than independently check it. A test that fails and is then "fixed" by pasting the new hash into the mirror is worse than no test, because it reads as coverage.
What remains is the assertion with runtime teeth, hardened along the lines you asked for: the manifest pin now parses module.toml, selects the chain-log subscription, and compares its event_signature to the decoder topic-0 as a typed B256. That one maps to a real production failure, since nexum-runtime::supervisor feeds the value to build_alloy_filter, so a drift subscribes to one topic while decoding another. Verified by mutating the pin one hex digit and confirming failure.
Two follow-on tidies so nothing overclaims: module.toml's comment no longer says the topic-0 is "pinned in cow-events.wit and parity-tested", since only the decoder parity is tested now, and the surviving test's doc no longer refers to the deleted sibling.
Net effect on your original finding: the weakened test is gone rather than restored, and the rigour moved to the assertion that was silently carrying the same substring weakness all along.
|
Pre-verified ahead of this car reaching the frontier, so it does not need re-deriving at review time. This car deletes Checked that the deletion re-points consumers at the generated constants rather than relocating the literals. On #463 the call sites read So this is the correct end state and no follow-up is needed. Noting for completeness that between #463 and this car the tree carries hash literals whose sole guard is |
fd57ecb to
21c14f0
Compare
77276fa to
f2b8407
Compare
a68de42 to
208cb20
Compare
|
On the two review-scale notes. Scope clarity. Agreed and folded into the PR body as a Process. Fair, and I am not going to pretend otherwise: bundling a 4242-line cone deletion with the stop-loss migration and a new enforcement gate does remove the ability to bisect a regression to one of the three. Splitting it now would mean rebuilding the ripple for a car that is already green, so the cost outweighs the benefit at this point in the train, but the shape you describe (migrate, prove stable, then delete) is the right default and I will use it for the remaining cars where the same pattern comes up. Worth noting the deletion half is at least self-verifying here: the On your WIT-text-only caveat for that gate, that is a real limitation and I agree a dependency-graph check is the eventual pairing: a Rust-level import of a CoW-specific type from a generic crate with no WIT reference would slip past it today. Not filing it as an issue yet since the crate layout is about to move under the M5 carve, which is where a graph check would have to be re-homed anyway. |
208cb20 to
e95d9a0
Compare
Delete shepherd-cow-host, the shepherd:cow legacy worlds (cow-api, cow-ext, shepherd; cow-events stays as the event-ABI package of record), the shepherd-sdk cow surface, and the shepherd-sdk-test mocks. The shepherd binary composes the core lattice with the videre platform alone. The keeper sweep moves to composable-cow behind the sweep slice; stop-loss migrates onto #[videre_sdk::keeper] and pool submit through the typed CowClient, keyed on the venue-and-body intent-id. twap gates topic-0 on the sol decoder pinned against cow-events; the WIT layering gate moves to cow-venue. ADR-0005/0006 marked superseded.
e95d9a0 to
5476510
Compare
Delete shepherd-cow-host, the shepherd:cow legacy worlds (cow-api, cow-ext, shepherd; cow-events stays as the event-ABI package of record), the shepherd-sdk cow surface, and the shepherd-sdk-test mocks. The shepherd binary composes the core lattice with the videre platform alone. The keeper sweep moves to composable-cow behind the sweep slice; stop-loss migrates onto #[videre_sdk::keeper] and pool submit through the typed CowClient, keyed on the venue-and-body intent-id. twap gates topic-0 on the sol decoder pinned against cow-events; the WIT layering gate moves to cow-venue. ADR-0005/0006 marked superseded.
What
Deletes the legacy
shepherd-cow-hostextension, theshepherd:cowlegacy WIT worlds (cow-api,cow-ext,shepherd;cow-eventsstays as the event-ABI package of record), theshepherd-sdkcow client cone, andshepherd-sdk-testmocks. Adds asweepslice tocomposable-cow(poll-loop composition over the typedcow-venueclient onvidere:venue/client) and moves stop-loss onto#[videre_sdk::keeper]+ venue-registry submit, keyed on the venue-and-body intent-id. twap gates topic-0 on the sol decoder pinned againstcow-events. Adds a WIT layering gate incow-venueasserting no generic package referencesshepherd:cow. Marks ADR-0005/0006 superseded.Why
Closes out the CoW-on-videre split for M4: the submit/status path is now served entirely by the
cow-venueadapter over the venue registry, so the interim host-sidecow-apishim is dead weight per the videre-split-plan §4 (the R2 category error resolution and the clean adapter/keeper re-split).Scope
This closes the ADR-0010 half of the split, venue-as-adapter-component: the adapter boundary is the only submit/status path and the legacy host cone is gone.
It does not touch the ADR-0012 egress-guard pipeline, which stays the advisory no-op it is today. There are zero references to
EgressGuardorAllowAllGuardin this diff, and cone retirement should not be read as closing the egress-policy half.Testing
cargo fmt --all --checkcargo check --workspace --all-featurescargo clippy --workspace --all-targets --all-features -- -D warningscargo nextest run --workspace --all-features --no-fail-fastcargo test --docAI Assistance
Implemented with Claude Code assistance; reviewed and gated by mfw78.
Closes #293