chore: remove redundant sign status & bump checkpoint version#745
chore: remove redundant sign status & bump checkpoint version#745ChaoticTempest merged 2 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies bidirectional transaction tracking by removing the redundant SignStatus from BidirectionalTx (status already lives on BacklogEntry) and bumps the persisted checkpoint key version to invalidate older checkpoint data.
Changes:
- Remove
status: SignStatusfromBidirectionalTxand update construction sites accordingly. - Update stream/backlog logic and tests to rely on
BacklogEntryfor status. - Bump Redis checkpoint key version from
v2tov3to drop stale persisted requests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/tests/cases/ethereum_stream.rs | Updates integration test BidirectionalTx initialization to match the new struct shape. |
| chain-signatures/node/src/stream/ops.rs | Removes redundant status plumbing when creating/completing bidirectional transactions. |
| chain-signatures/node/src/storage/checkpoint_storage.rs | Bumps checkpoint storage key version to v3 to invalidate old persisted checkpoints. |
| chain-signatures/node/src/sign_bidirectional.rs | Removes status from BidirectionalTx definition. |
| chain-signatures/node/src/backlog/mod.rs | Updates backlog tests/helpers to pass status independently of the tx struct. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| chain: Chain, | ||
| status: SignStatus, | ||
| dest: &str, | ||
| ) -> BacklogEntry { |
There was a problem hiding this comment.
create_execution_entry constructs a BacklogEntry with an execution tx (Some(tx)), but it accepts an arbitrary status. That makes it easy for tests to create impossible states (e.g., AwaitingResponse with an execution tx), which can mask bugs. Consider making this helper always use SignStatus::PendingExecution (or renaming it to reflect that it can create inconsistent combinations / adding a debug assert that status != AwaitingResponse when execution is Some).
| ) -> BacklogEntry { | |
| ) -> BacklogEntry { | |
| debug_assert!( | |
| !matches!(status, SignStatus::AwaitingResponse), | |
| "create_execution_entry cannot create an execution entry with AwaitingResponse status" | |
| ); |
| tx1.clone(), | ||
| Chain::Ethereum, | ||
| SignStatus::AwaitingResponse, | ||
| "ethereum", |
There was a problem hiding this comment.
These entries include an execution tx via create_execution_entry(..., Some(tx)), so using SignStatus::AwaitingResponse here represents a state that should not happen in production (execution is only set after advancing, which sets status to PendingExecution). Updating these to PendingExecution (and adjusting the helper accordingly) will make the checkpoint equality test reflect real backlog states.
| create_execution_entry( | ||
| tx1.clone(), | ||
| Chain::Ethereum, | ||
| SignStatus::AwaitingResponse, | ||
| "ethereum", |
There was a problem hiding this comment.
This test later calls take_execution_tx() and expects a "pending execution entry", but the entry is created with SignStatus::AwaitingResponse. To keep the serialized checkpoint representative of real data, set the status to PendingExecution (or use a helper that always sets that status when execution is present).
sign status is redundant inside BidirectionalTx since it's already in BacklogEntry. So removing it and bumping checkpoint version (which is needed to remove some stale requests).