twap-monitor: index ComposableCoW v2 ConditionalOrderRemoved - #472
Conversation
lgahdl
left a comment
There was a problem hiding this comment.
Verified thoroughly, including actually building and running the test suite against this commit and cross-checking ConditionalOrderRemoved's topic-0 against upstream cow-rs's own keccak256 self-test — both match. The chain-position comparison is correct (derived Ord on {block, index} gives block-first lexicographic ordering, and < is the right comparison since a create and its own removal can never share a log index). All four named test cases genuinely test what their names claim: the stale-removal-after-re-register case creates then removes at an older position and asserts the watch survives; the drop-and-spare case seeds a sibling watch and confirms it's untouched while the target's gates are gone; the redelivered-older-create case confirms prior.max(indexed_at) isn't regressed; and the no-mined-position case uses the SDK's real pending-log representation (block_number/log_index both None) and confirms no panic. The row codec (versioned tag + two LE u64s ahead of the ABI payload) is a clean, minimally-coupled encoding with its own round-trip and malformed-input tests.
One minor test-coverage nit, not a correctness defect: no test covers same-block, different-log-index ordering (e.g. create at (7,5), a genuine removal at (7,6)) — worth adding alongside the existing cross-block cases, but the underlying comparison logic is already verified correct by inspection. Approving.
e95d9a0 to
5476510
Compare
b2c6c4d to
c0e34e0
Compare
Subscribe the twap-monitor to the ComposableCoW v2 ConditionalOrderRemoved topic-0 and pin it in the shepherd:cow/cow-events package of record. The created and removed subscription streams merge in arrival order, not chain order, so each watch row carries the create log's (block, log-index) stamp and a removal drops the watch (with its gates) only when it postdates the stamp: a stale removal for a re-registered order is ignored, and an unprovable ordering keeps the watch for the poll path's self-healing drop.
c0e34e0 to
2e89336
Compare
|
Thanks for the depth here, particularly cross-checking the removal topic-0 against upstream The coverage nit is real and is now closed in this car. The existing cases bracketed the ordering boundary on only one side: Added Two other changes landed in this car while rippling it onto the retired cone, both consequences of a decision taken on #471 rather than anything from your review. The WIT parity assertion this branch carried is gone rather than extended. mfw78's call, and the reasoning is that The manifest assertion took its place and is stronger than the form on either side of the rebase. It parses The PR body previously said the topic-0s were "pinned in cow-events.wit and parity-tested against module.toml"; it now says the WIT describes them and names what is actually asserted. |
twap: index the v2 ConditionalOrderRemoved event and drop the watch Subscribe the twap-monitor to the ComposableCoW v2 ConditionalOrderRemoved topic-0 and pin it in the shepherd:cow/cow-events package of record. The created and removed subscription streams merge in arrival order, not chain order, so each watch row carries the create log's (block, log-index) stamp and a removal drops the watch (with its gates) only when it postdates the stamp: a stale removal for a re-registered order is ignored, and an unprovable ordering keeps the watch for the poll path's self-healing drop.
What
Indexes the ComposableCoW v2
ConditionalOrderRemovedevent intwap-monitorand drops the corresponding watch (with its gates) when the removal provably postdates the watch's indexed create. Watch rows now carry a chain-position stamp (block,log-index) alongside the ABI-encoded params;created/removedtopic-0s are described inwit/shepherd-cow/cow-events.witand parity-tested by comparingmodule.toml's chain-log pins against thesol!decoder topic-0s as a set.Why
The two subscription streams (create, remove) merge in arrival order, not chain order, so an unordered removal could otherwise drop a re-registered watch or apply a stale remove ahead of its create. Stamping the indexed position makes the merge order irrelevant: a removal lands only when it is provably later than the watch's create.
Closes #54
Testing
nextest: strategy.rs indexer/poll suite, incl. new cases - stale removal after re-register is ignored, later removal in its own dispatch drops the watch + gates while sparing siblings, redelivered older create keeps the later stamp, and a removal without a mined position is a no-op.
AI Assistance
Implemented with Claude Code.