Conversation
|
📝 WalkthroughWalkthroughTwo targeted fixes: nonce-tracking cleanup is now conditional on the old nonce being linked to the current tx UUID, and built transactions are persisted before being sent to the inclusion stage. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as BuildingStage
participant State
participant Inclusion as InclusionStage
rect rgb(200,220,240)
Note over Builder: Tx built
Builder->>State: store_tx(&tx) %% NEW: persist before send
State-->>Builder: Ok
end
rect rgb(220,240,220)
Builder->>Inclusion: send_tx_to_inclusion_stage(tx)
Inclusion-->>Builder: Ok / Err
end
rect rgb(255,240,200)
Note over Builder: If send fails, tx already persisted
end
sequenceDiagram
participant Caller as NonceAssigner
participant DB as NonceStateDB
Caller->>DB: assign_next_nonce(tx_uuid, old_nonce?)
alt old_nonce is Some(n)
DB->>DB: get_tracked_tx_uuid(n)
alt tracked_tx_uuid == tx_uuid
DB->>DB: clear_tracked_tx_uuid(n)
DB->>DB: clear_tracked_tx_nonce()
else tracked to other tx
Note right of DB: leave linkage intact
end
else old_nonce is None
Note right of DB: nothing to clear
end
DB-->>Caller: new_nonce
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign.rs(1 hunks)rust/main/lander/src/dispatcher/stages/building_stage/building.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
rust/main/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run cargo clippy for Rust code linting
Files:
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign.rsrust/main/lander/src/dispatcher/stages/building_stage/building.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: yarn-install
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: lint-rs
- GitHub Check: test-rs
- GitHub Check: lander-coverage
🔇 Additional comments (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign.rs (1)
17-29: Good defensive check, mate.This conditional cleanup is the right move - it prevents clearing nonce associations that don't actually belong to the transaction in question. Before, any old_nonce would trigger cleanup unconditionally, which could mess up other transactions' nonce linkages. Now you're verifying ownership first.
The error propagation from
get_tracked_tx_uuidlooks fine, and the warning message on lines 22-25 is still appropriate for when the condition is actually met.rust/main/lander/src/dispatcher/stages/building_stage/building.rs (1)
102-107: Store-before-send pattern looks good.Moving the persistence before the send is the right approach. Even if the send fails or gets retried, the transaction is already safely stored. This aligns well with the PR objective of ensuring no transaction tracking is lost.
The retry logic with
call_until_success_or_nonretryable_errorensures eventual delivery to the inclusion stage, and the store operation won't be repeated on retries since it happens before the retry loop.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rust/main/lander/src/dispatcher/stages/building_stage/building.rs (1)
99-99: Error handling forstore_txremains unaddressed.This concern was previously raised and is still valid. The call to
store_txdoesn't surface errors to the caller - if database persistence fails, the transaction will still proceed to the inclusion stage on line 102, defeating the purpose of storing it first.Without knowing whether storage succeeded, you can't guarantee the transaction won't be lost if the send operation fails later. The whole swamp... er, the whole point of this change is to persist before sending, but silent failures undermine that goal.
🧹 Nitpick comments (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign.rs (1)
18-28: Consider logging when nonce linkage check fails.The new conditional properly prevents clearing unrelated nonce associations, which is good defensive coding. However, when
old_nonceexists but doesn't match the currenttx_uuid, the function silently continues without any indication. This could make debugging difficult if callers expect the old nonce to be cleared.Consider adding a debug or info log when the check fails, explaining why the cleanup was skipped. Something like:
if *tx_uuid == self.get_tracked_tx_uuid(nonce).await? { // existing logic... } else { debug!( ?nonce, ?tx_uuid, "Skipping nonce cleanup - old nonce not associated with this transaction" ); }This would help track down issues where nonces aren't being reassigned as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign.rs(1 hunks)rust/main/lander/src/dispatcher/stages/building_stage/building.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
rust/main/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run cargo clippy for Rust code linting
Files:
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign.rsrust/main/lander/src/dispatcher/stages/building_stage/building.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-matrix (sealevel)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign/tests/tests_assign.rs (1)
18-50: Solid test for preserving nonce linkage across different transactions.This test nails down an important edge case, mate. When a new transaction grabs a nonce and references an old one that belongs to a different transaction, the old linkage stays put. That's exactly what we want to prevent linkage from getting trampled on.
The test setup is clean and the assertions are spot-on. Nicely done keeping it consistent with the other tests in the file.
A couple optional thoughts if you're feeling fancy:
You might consider storing an actual transaction for
other_tx_uuid(likecreate_tx(other_tx_uuid, TransactionStatus::PendingInclusion)followed bystate.tx_db.store_transaction_by_uuid(&tx)), similar to what the other tests do. Makes the scenario a tad more realistic, though it's not strictly necessary for what you're testing here.Could add an assertion that
new_nonceis actually tracked totx_uuid, though other tests probably cover that ground already.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign/tests/tests_assign.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
rust/main/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run cargo clippy for Rust code linting
Files:
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign/tests/tests_assign.rs
🧬 Code graph analysis (1)
rust/main/lander/src/adapter/chains/ethereum/nonce/state/assign/tests/tests_assign.rs (1)
rust/main/lander/src/tests/test_utils.rs (1)
tmp_dbs(41-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: infra-test
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
- GitHub Check: test-rs
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7217 +/- ##
=====================================
Coverage 0.00% 0
=====================================
Files 1 0 -1
Lines 14 0 -14
=====================================
+ Misses 14 0 -14
🚀 New features to boost your workflow:
|
3 participants
Description
This PR makes sure that no linkage between tx & nonce is lost. It also ensures that TX are stored in DB in the build stage before sending them to the inclusion stage.
Drive-by changes
Related issues
Linear Issue 2034
Backward compatibility
Testing
Summary by CodeRabbit
Bug Fixes
Tests