refactor(twap-monitor): port poll loop onto keeper#240
Conversation
75e2e89 to
0633f28
Compare
0a7b417 to
6712cee
Compare
0633f28 to
e0b2392
Compare
6712cee to
e6f49c2
Compare
e0b2392 to
304b14e
Compare
e6f49c2 to
2763a44
Compare
304b14e to
013c78c
Compare
2763a44 to
49caf33
Compare
013c78c to
ed46d40
Compare
49caf33 to
a0f4a80
Compare
ed46d40 to
bad52bd
Compare
a0f4a80 to
8bf30dc
Compare
bad52bd to
1c68b61
Compare
8bf30dc to
a9580f9
Compare
1c68b61 to
c767d21
Compare
a9580f9 to
9dcf0f2
Compare
c767d21 to
8a22007
Compare
9dcf0f2 to
06e7f3d
Compare
8a22007 to
8fec813
Compare
06e7f3d to
47c5181
Compare
8fec813 to
283c938
Compare
47c5181 to
318103a
Compare
283c938 to
d4fbbdf
Compare
318103a to
4c45d7f
Compare
d4fbbdf to
ffa0586
Compare
4c45d7f to
2598743
Compare
ffa0586 to
7ef3f63
Compare
2598743 to
725162b
Compare
7ef3f63 to
c7a609c
Compare
725162b to
745712c
Compare
b4fd03b to
787757a
Compare
15e60c1 to
c0cce78
Compare
787757a to
79335b9
Compare
c0cce78 to
1f9c305
Compare
79335b9 to
e734f8e
Compare
1f9c305 to
b4ff875
Compare
e734f8e to
71e0cc8
Compare
b4ff875 to
61d7c89
Compare
71e0cc8 to
ff15ae2
Compare
61d7c89 to
831dbea
Compare
ff15ae2 to
6566b23
Compare
831dbea to
1395917
Compare
6566b23 to
dd7a7be
Compare
a888dc2 to
b258116
Compare
b75d9ca to
e5da3e1
Compare
b258116 to
e67ba0a
Compare
lgahdl
left a comment
There was a problem hiding this comment.
Clean port — the deletion of BuildError, WatchUpdate, apply_watch_update, apply_submit_retry, and all hand-rolled gate helpers is the right move. Two things worth calling out explicitly:
Behaviour change, not identity: The PR description says the acceptance tests are "byte-identical to base, demonstrating behaviour identity of the port." This is true for the success path only. poll_one now delegates to classify_poll_error, which maps any ChainError::Rpc with 4+ bytes of data that decode_revert does not recognise to DontTryAgain (permanent drop). The old code always fell back to TryNextBlock. Any live TWAP whose handler contract emits an unrecognised custom error will have its watches permanently dropped after this merge. That is the right policy direction, but it should be stated as a deliberate tightening, not subsumed under "byte-identical."
Silent bug fix: The deleted apply_submit_retry wrote Backoff gates as the raw string "next_epoch:{owner_hex}:{hash_hex}" instead of going through Gates::set_next_epoch. If that key format ever differed from what is_ready read, the backoff gate was invisible to the gating check. The new path through RetryLedger::apply -> Gates::set_next_epoch is provably consistent. Worth a sentence in the PR description.
| // fault warrants its own diagnostic here. | ||
| Err(err) => { | ||
| if let ChainError::Fault(fault) = &err { | ||
| tracing::warn!("eth_call failed ({fault}); retrying next block"); |
There was a problem hiding this comment.
Only transport faults are logged here; when classify_poll_error returns DontTryAgain for an unrecognised Rpc revert the watch disappears with zero diagnostic at the poll site (materialise does not log for DontTryAgain either). The old code always emitted a warn for this path — an operator has no visibility into why a TWAP watch dropped.
Consider logging for the Rpc case too after the classify_poll_error call, e.g.: if matches!(outcome, PollOutcome::DontTryAgain) { tracing::warn!("eth_call unrecognised revert for {}; dropping watch", watch.key()); }
There was a problem hiding this comment.
Cross-ref for when this thread gets swept: the diagnostic half of this finding is superseded by the ADR-0013 Verdict seam (#334) — Verdict::Invalid { reason: [u8; 4] } now carries the selector that this path drops silently, and the strategy's drop diagnostic logs it. So on the train tip the watch no longer disappears with zero trace; this branch's transport-only logging is fine as an interim state.
| /// Short strategy name compositions prefix shared log lines with | ||
| /// (for example `"twap"`). Diagnostic only - no behaviour keys | ||
| /// off it. |
There was a problem hiding this comment.
Grammar makes this hard to parse: "compositions" should be "implementations", and "prefix ... with" inverts the normal subject-verb order.
| /// Short strategy name compositions prefix shared log lines with | |
| /// (for example `"twap"`). Diagnostic only - no behaviour keys | |
| /// off it. | |
| /// Short name that implementations return to prefix shared log lines. | |
| /// Diagnostic only - no behaviour keys off this value. | |
| /// Default: `"conditional"`. |
| /// source cannot decode polls again next block rather than tearing | ||
| /// down the sweep. | ||
| struct TwapSource; |
There was a problem hiding this comment.
"Row" is informal and inconsistent with the SDK's terminology (watch, WatchSet). The rest of the PR uses "watch" throughout.
| /// source cannot decode polls again next block rather than tearing | |
| /// down the sweep. | |
| struct TwapSource; | |
| /// and evaluate `getTradeableOrderWithSignature` on chain. A watch | |
| /// whose params cannot be decoded returns `TryNextBlock` rather than | |
| /// tearing down the sweep. |
e5da3e1 to
dc6b70b
Compare
2195569 to
d4db1a0
Compare
dc6b70b to
d03e955
Compare
9f6016a to
dbc8f12
Compare
003de79 to
757f00b
Compare
dbc8f12 to
cf2a241
Compare
The run logged through the LoggingHost seam, which the guest tracing capture cannot observe - module tests proving behaviour identity for keeper ports assert on tracing events. Route the submit-path diagnostics through the tracing macros with the wording the legacy twap poll loop established, and give ConditionalSource a defaulted label so shared log lines attribute the strategy that produced them.
Reduce strategy.rs to decode and evaluate: log decoding persists watches through the keeper watch set, and the getTradeableOrderWithSignature evaluation moves behind ConditionalSource. The hand-rolled gate keys, watch-update lifecycle, submitted: markers, and submit-retry dispatch are deleted in favour of cow::run over the keeper stores and retry ledger, still on the legacy CowApiHost seam. The MockHost dispatch tests are untouched - the behaviour-identity proof for the port.
What
Ports
twap-monitoroff its hand-rolled poll loop onto the shared keeper composition (ConditionalSource+WatchSet), via the run loop inshepherd_sdk::cow::run, reusing the existingCowApiHostseam.crates/nexum-sdk/src/keeper.rs:ConditionalSourcegains a defaultedlabel()hook (default"conditional") so shared log lines can attribute the emitting strategy.crates/shepherd-sdk/src/cow/run.rs: the run loop's submit-path diagnostics ("submit retry-next-block: ...","{label} UID divergence: client=... server=...", submitted markers) now go through the guest tracing facade instead ofhost.log, so they are observable viacapture_tracing.modules/twap-monitor/src/strategy.rs: shrinks from ~1140 to ~830 lines by deleting the hand-rolled poll loop (gate keys, watch-update lifecycle,submitted:markers, submit-retry dispatch,BuildError,build_order_creation,compute_uid_hex,outcome_to_update) in favour of aTwapSource: ConditionalSource<H>implementation (label() -> "twap") driven byshepherd_sdk::cow::run. The strategy's own revert-decode dispatch is also dropped in favour of the sharedclassify_poll_errorpolicy.on_chain_logsnow persists viaWatchSet::put, andon_blockbuilds aTick.crates/shepherd-sdk/tests/run.rs: updated the run-loop integration test's log assertion to capture viacapture_tracinginstead ofhost.logging.crates/shepherd-sdk/Cargo.toml,modules/twap-monitor/Cargo.toml,Cargo.lock: dependency updates to support the above (tracingas a direct dependency,nexum-sdk-testdev-dependency,strum/thiserror/serde_jsondropped fromtwap-monitor's non-dev dependencies).Why
twap-monitorcarried its own gate bookkeeping, watch lifecycle and submit-retry dispatch, duplicating behaviour the keeper composition already provides generically. Porting it ontoConditionalSource+WatchSetremoves that duplication and givestwap-monitorthe same submit-path diagnostics and retry policy as every other strategy running on the keeper, whilelabel()keeps shared log lines attributable once more than one composed strategy is running.Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warnings(host and wasm32-wasip2)cargo test --workspace(twap-monitor, shepherd-sdk, nexum-sdk; wall-clock-dependent tests re-run twice)Acceptance: the
MockHostdispatch tests are byte-identical to base and pass, demonstrating behavioural identity of the port. No.witfiles touched, so no new WIT packages or bindgen smoke required.AI Assistance
Claude (Opus) used for the port.
Closes #147