You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Summary: This PR refactors add_dummy_tx by extracting create_dummy_tx (returns the raw signed tx) and adds add_external_tx (adds a pre-built tx to the order builder). These are used by a new test_bundle_allow_tx_skip test that validates the allow_tx_skip behavior when two bundles share a reverting transaction.
Findings:
Incomplete doc comment on create_dummy_tx + (setup.rs:123): The doc comment /// create_dummy_tx + is truncated/unclear. It should describe what add_dummy_tx does beyond create_dummy_tx (i.e., adds the tx to the order builder and returns the hash).
Unnecessary .into() on tx_hash (setup.rs:134): *tx.hash() already returns B256 which is TxHash. The old code returned Ok(tx_hash) directly. The new code uses Ok(tx_hash.into()) — this is a no-op conversion. Not a bug, but unnecessary.
Test doesn't verify allow_tx_skip=true is actually the cause: The test comments say the tx is skipped "because allow_tx_skip=true", but TestSetup always constructs PartialBlock::new(true) (i.e., discard_txs=true). A stronger test would also verify that with discard_txs=false, Bundle 2 would fail — confirming the behavior is actually gated on allow_tx_skip. Without that, the test could pass even if the skip logic were broken, as long as something else handles the stale nonce gracefully.
No negative test for non-reverting tx with stale nonce: The test only covers skipping a tx that is in reverting_tx_hashes. It would be valuable to also test that a non-reverting tx with a stale nonce (i.e., TxRevertBehavior::NotAllowed) causes the bundle to fail, confirming the skip is conditional on revert behavior.
Overall the code is clean and the refactoring is reasonable. The main suggestion is to strengthen the test to actually validate the allow_tx_skip gate rather than only testing the happy path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Summary
New test case.
✅ I have completed the following steps:
make lintmake test