refactor(cow): structured Verdict poll seam + LegacyRevertAdapter (Wave 1, ADR-0013)#334
Conversation
077d69c to
a7495ff
Compare
a7495ff to
d86e052
Compare
56b3790 to
806fc83
Compare
806fc83 to
fa31ad5
Compare
fa31ad5 to
1894f9d
Compare
lgahdl
left a comment
There was a problem hiding this comment.
Reviewed across technical/comment/architecture dimensions, with the behavior-identity claim verified variant-by-variant against the base: it holds — every legacy selector maps to the identical store effect (gate keys, seconds-vs-ms, keep/drop all unchanged), the #75 unknown-selector drop policy is preserved with its regression tests migrated intact, proptests weren't weakened, and the submit path (incl. the #320 retry-classification area) is byte-identical. Wave-1-now sequencing is right, the quarantine is clean (one production importer; run.rs knows nothing about legacy decode), and the variant names match the fork's GeneratorResultCode vocabulary 1:1. Three findings inline; three notes here:
- Stale references the rename missed (ironic on a sweep branch):
shepherd_sdk::cow::decode_revertstill named incrates/nexum-runtime/src/host/provider_pool.rs:431(a rustdoc link to a now-nonexistent path),crates/nexum-runtime/src/host/impls/chain.rs:127,crates/nexum-sdk/src/proptests.rs:13;PollOutcome/decode_revert(_hex)still described as the shipped API indocs/sdk.md:72,docs/00-overview.md:280,docs/05-sdk-design.md:79,docs/08-platform-generalisation.md:990,docs/qa-signoff.md:62; andsubmit_ready's doc inrun.rsstill says "freshly-polledReadyorder". - Deletion path: ADR-0013 schedules
composable.rsfor deletion at the wire swap, but this PR houses the permanentVerdictin that same file, docs threaded with adapter references. SplittingVerdictintocow/verdict.rs(leaving composable.rs = sol! mirror + adapter) would make wave 2 a file deletion instead of surgery on the surviving type. Postshape vs the fork wire: the ADR'sGeneratorResultcarries nosignaturefield (it arrives at thegetTradeableOrderWithSignaturelevel) and suppliesreasonCodeuniformly whilePostis the one variant withoutreason. Worth a doc note on signature provenance before wave 2 is called mechanical.
| watches.remove(watch)?; | ||
| } | ||
| Verdict::NeedsInput { .. } => { | ||
| tracing::info!("watch {} parked awaiting input", watch.key()); |
There was a problem hiding this comment.
"Parked" overstates this arm on both sides of the doc/code line: no gate is written and no parked state exists, so the watch stays gate-ready and is re-polled (one eth_call) every tick, emitting this info line each time — semantically TryNextBlock plus log spam. Unreachable today, but the moment the fork makes NEEDS_INPUT reachable this becomes a busy-poll masquerading as quiescence, and ADR-0013's stated handling ("hands to the offchainInput layer or parks") has neither half implemented. Suggest: either document it honestly as interim re-poll (debug-level, one-shot log) or write a real gate with a defined unpark path — and add the missing dispatch test (needs_input_keeps_watch_and_writes_no_gate); it's the only variant without store-effect coverage, so a later "fix" could silently change semantics.
| /// Advisory Unix timestamp (seconds) the fork's generator hints | ||
| /// the next poll at. `0` when synthetic - the legacy adapter | ||
| /// has no such hint, so the submit path ignores it. | ||
| next_poll_timestamp: u64, |
There was a problem hiding this comment.
The 0 sentinel collides with the fork's wire semantics before wave 2 even starts: per ADR-0013 the fork's nextPollTimestamp uses 0 = "poll at validTo + 1" and uint256::MAX = "stop", while wave 1 defines 0 = "synthetic, ignored". Same field, same type, incompatible meanings — and since run() destructures the field away, wave 2 must reinterpret the sentinel and add gate behaviour simultaneously, exactly the coupled change this pre-migration exists to avoid. Option<u64> (or a small NextPoll enum: NoHint / At(u64) / AfterValidTo / Stop) now, while it's provably a no-op, makes the swap actually mechanical.
| /// selector the outcome was derived from, for logging only - no | ||
| /// behaviour keys off it. It is `[0; 4]` when the outcome is synthetic | ||
| /// (no selector available, e.g. a transport fault). `Post` is the only | ||
| /// variant [`LegacyRevertAdapter`] never produces; it comes from the |
There was a problem hiding this comment.
Self-contradicting: NeedsInput's own doc three lines down says the adapter never produces it either. While correcting this, one archaeology line would serve readers of old logs/tests: the adapter only produces TryNextBlock/WaitBlock/WaitTimestamp/Invalid; Post comes from the strategy's success path, NeedsInput only from the fork's generator — renamed from PollOutcome (ADR-0013 wave 1): Ready→Post, TryOnBlock→WaitBlock, TryAtEpoch→WaitTimestamp, DontTryAgain→Invalid.
1894f9d to
d1cefb8
Compare
What
Migrates the CoW poll seam from the ad-hoc reverting-poll enum to a structured
Verdict, mirroring the composable-cow fork's structured generator. Wave 1 of the ccow-monitor migration (ADR-0013, #333). Rust-only, no contract dependency: the deployed 1.x reverting poll wire is unchanged, its decode is renamed into a quarantinedLegacyRevertAdapter, and the module already dispatches on the newVerdict.crates/shepherd-sdk/src/cow/composable.rs: the old poll enum becomesVerdict { Post{order,signature,next_poll_timestamp}, WaitTimestamp, WaitBlock, TryNextBlock, Invalid, NeedsInput }; the legacy revert-decode helpers becomeLegacyRevertAdapter::{decode,classify}, mapping the five deployed-1.x selectors ontoVerdict.crates/shepherd-sdk/src/cow/run.rs: the keeper run loop dispatches onVerdict(ConditionalSource<Outcome = Verdict>).modules/twap-monitor/src/strategy.rs:TwapSourceproducesVerdictvia the adapter.crates/shepherd-sdk/src/cow/mod.rs,src/lib.rs,src/proptests.rs,README.md,crates/shepherd-sdk/tests/run.rs,crates/shepherd-sdk-test/tests/mock_venue.rs: re-exports, docs and test suites updated to the new names.Why
The composable-cow fork returns a structured, non-reverting poll verdict plus scheduling hints, so the off-chain revert-decode is obsolete. Migrating the Rust seam now, before the fork deploys, makes the eventual wire-swap a struct-read rather than a re-port. The merge gate holds: the eth_call target and the deployed 1.x contract are untouched, and the adapter is deleted when the fork deploys (ADR-0013). Behaviour is identical: every
Verdictvariant drives the exact store effect its predecessor did.NeedsInputis new (it parks the watch) and is unreachable through the legacy wire.Testing
cargo nextest run -p shepherd-sdk -p twap-monitor --all-features(62 passed, 0 failed) andcargo clippy -p shepherd-sdk -p twap-monitor --all-targets --all-features -- -D warnings(clean), under the pinned 1.94 toolchain vianix develop. The whole-workspace battery passed on the assembled head during review (443 passed). Existing MockHost tests are the behaviour-identity proof and now assert theVerdictequivalents.AI Assistance
Claude (Opus) used for the implementation, red-team, and gate pipeline that produced and validated the seam.