twap: grant one next-block retry before dropping on InvalidEip1271Signature - #473
Conversation
lgahdl
left a comment
There was a problem hiding this comment.
The marker lifecycle, classification table wiring, and string-prefix matching are all solid where they were actually applied: Retrier::apply's DropOnRepeat arm correctly distinguishes "later block" from "same block" (verified: tick.block > block -> drop, tick.block <= block -> no-op), clearing on accept is idempotent and WatchSet::remove also deletes the marker so nothing orphans, classification.toml's row moved cleanly to drop-on-repeat with no dangling reference to the old drop behavior, and classify_denied's prefix match is anchored (split_once(':') then exact table lookup, not a substring/contains search) so there's no false-positive risk from an unrelated error's text.
But the PR's central claim — that this logic is applied on "both the videre-sdk keeper sweep and the legacy composable-cow sweep" — is false as shipped. I checked crates/videre-sdk/src/keeper.rs directly at this commit: retry_action's Denied arm is completely unchanged by this diff and still reads:
VenueFault::UnknownVenue
| VenueFault::InvalidBody(_)
| VenueFault::Unsupported
| VenueFault::Denied(_) => RetryAction::Drop,(crates/videre-sdk/src/keeper.rs:194-197). Only crates/composable-cow/src/sweep.rs:64-67 was actually edited to special-case VenueFault::Denied(detail) => classify_denied(detail). That means twap-monitor — which per #469 runs on the generic videre_sdk::Keeper::sweep path, and which is the exact scenario this PR's "Why" section describes ("a first-time TWAP user's Safe wiring... lands in the same block") — still drops the watch permanently on the FIRST InvalidEip1271Signature refusal. Only stop-loss (composable-cow's legacy path, per #471) actually gets the one-block grace this PR adds. The bug this PR closes #320 for is still live for its primary named beneficiary.
Fix: add the same VenueFault::Denied(detail) => classify_denied(detail) branch inside videre_sdk::keeper::retry_action's Denied arm (or fold the re-derivation into retry_action itself so both sweeps call through one place, closing the duplication risk at the same time).
c0e34e0 to
2e89336
Compare
…nature A first-time user's Safe wiring and ComposableCoW create() land in one block, so the orderbook rejects the first submission against its own head and the keeper dropped a valid watch. The classification table gains a drop-on-repeat action, the retry ledger records the refused block and gates one next-block retry, and a denied refusal re-enters the table by its errorType prefix so the grace survives the coarse venue-error collapse. A repeat on a later block still drops.
428527b to
c5b52f3
Compare
|
The finding is real and the PR body was overclaiming, so thank you. Two corrections though, because the module attribution is inverted and the suggested fix cannot be implemented as written. Which module is on which path. It is the other way round. Why the suggested fix is not available. Adding Severity today. stop-loss builds What I changed. The PR body no longer claims the re-derivation applies on both sweeps. It now says where it lives, why the generic path cannot carry it, and which module is affected. Filed nullislabs/videre-nexum-module#11 for the seam: a venue-supplied denial classifier so both paths derive the same Separately, this car arrived with |
…nature (#473) A first-time user's Safe wiring and ComposableCoW create() land in one block, so the orderbook rejects the first submission against its own head and the keeper dropped a valid watch. The classification table gains a drop-on-repeat action, the retry ledger records the refused block and gates one next-block retry, and a denied refusal re-enters the table by its errorType prefix so the grace survives the coarse venue-error collapse. A repeat on a later block still drops.
What
Adds
RetryAction::DropOnRepeat: the firstInvalidEip1271Signaturerefusal for a watch retries on the next block instead of dropping; a repeat refusal on a later block drops as before. Tracked via arefused:block marker on the watch, cleared on an accepted submit. Reclassifies theInvalidEip1271Signaturerow inclassification.toml/classification_data.rsfromdroptodrop-on-repeat. Thedeniedwire error stays coarse, socow_venue::classify_deniedre-derives the action from theerrorTypeprefix in the detail string. That re-derivation lives on thecomposable-cowsweep, which is the path twap-monitor runs (composable_cow::run) and therefore the path this issue is about. The genericvidere_sdk::keeper::retry_actionstill mapsDeniedstraight toDrop: it cannot callclassify_deniedwithoutvidere-sdkdepending oncow-venue, which inverts the layering the venue-agnostic gate enforces. The only module on that generic path today is stop-loss, which submits unsigned pre-sign orders and so cannot receiveInvalidEip1271Signature. Tracked for a venue-injectable classifier rather than papered over.Why
A first-time TWAP user's Safe wiring and
ComposableCoW.create()can land in the same block as the indexed event. Shepherd polls and submits inside that block, faster than an orderbook node can verify against its own head, so a signature that is valid one block later is rejected and the watch is dropped permanently. Treating only a repeated refusal as permanent absorbs the one-block indexing race while still dropping genuinely broken signatures.Closes #320
Testing
nexum-sdkandvidere-sdkkeeper unit tests cover the marker set/clear/repeat-drop lifecycle;cow-venueadapter and classification tests cover thedrop-on-repeatrow andclassify_deniedprefix recovery;composable-cowsweep tests cover the marker clear on acceptance.AI Assistance
Implemented with AI assistance under human direction and review.