chore: testing updates & repo restructure #347
+1,916
−2,819
Conversation
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
- Remove file structure sections from READMEs - Move build_node function body into LocalNode::new - Replace local TestHarness::signer methods with Account::signer_b256()
Collaborator
🟡 Heimdall Review Status
|
danyalprout
requested review from
haardikk21,
niran and
refcell
and removed request for
haardikk21 and
refcell
Contributor
|
Makes sense to me. I still want the rest of #346's metering tests cleanup, but it shouldn't matter too much which order they're merged in since there will be pain regardless. |
danyalprout
pushed a commit
that referenced
this pull request
Jan 11, 2026
…ransactions (#347) * feat(tests): add BuilderTxValidation utility for validating builder transactions Adds `BuilderTxValidation` trait to test framework that provides: - `find_builder_txs()`: Returns info about builder transactions in a block - `has_builder_tx()`: Quick check if block contains builder transactions - `assert_builder_tx_count()`: Assert expected number of builder transactions Also adds `block_includes_builder_transaction` test demonstrating the utility. Closes #88 * test: add BuilderTxValidation assertions to existing tests Updates smoke, flashblocks, and revert tests to use the BuilderTxValidation utility for validating builder transactions in blocks: - smoke.rs: Added builder tx count assertions to chain_produces_blocks, chain_produces_big_tx_with_gas_limit, and chain_produces_big_tx_without_gas_limit - flashblocks.rs: Added builder tx count assertions to smoke_dynamic_base, smoke_dynamic_unichain, smoke_classic_unichain, and smoke_classic_base - revert.rs: Added builder tx validation to monitor_transaction_gc, disabled, and allow_reverted_transactions_without_bundle tests * fix: move builder tx validation before transactions move Fixes borrow of partially moved value error by calling assert_builder_tx_count before block.transactions is moved.
refcell
reviewed
Jan 11, 2026
Comment on lines
40
to
44
| // Feature extensions (FlashblocksExtension must be last - uses replace_configured) | ||
| runner.install_ext(Box::new(TxPoolExtension::new(tracing_config, sequencer_rpc))); | ||
| runner.install_ext(Box::new(MeteringExtension::new(metering_enabled))); | ||
| runner | ||
| .install_ext(Box::new(FlashblocksExtension::new(flashblocks_cell, flashblocks_config))); |
Contributor
There was a problem hiding this comment.
I think we could have kept the old api by doing:
pub fn install_ext::<T: BaseNodeExtension>(config: T::Config)I don't hold a super strong opinion here, either way works, but I think the old type-based api is a bit easier to read personally. Perhaps worth writing a small ticket once this is merged to address later as a low-hanging-fruit.
refcell
reviewed
Jan 11, 2026
bin/node/src/main.rs
Outdated
| let sequencer_rpc = args.rollup_args.sequencer.clone(); | ||
| let tracing_config = args.tracing_config(); | ||
| let metering_enabled = args.enable_metering; | ||
| let flashblocks_config = args.flashblocks_config(); |
Contributor
There was a problem hiding this comment.
Instead of having to do this, we could implement From<Args> for FlashblocksConfig for example and then in the method, call below just do:
runner.install_ext(Box::new(FlashblocksExtension::new(flashblocks_cell, args.into())));This way we keep the binary file a bit more lean.
(Same goes for the other configs)
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
refcell
reviewed
Jan 11, 2026
Contributor
refcell
left a comment
refcell
left a comment
There was a problem hiding this comment.
A couple of nits, but this is very nice work!
danyalprout
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR is refactors the repo to make the crate dependency graph simpler and leverage test-utils features.
Shared Crates
base-primitivesbase-flashtypesbase-bundlesbase-access-listsbase-primitives(test-utils)base-cli-utilsbase-reth-rpc-typesClient Crates
base-client-nodebase-primitives(test-utils)base-meteringbase-bundles,base-client-nodebase-flashblocksbase-flashtypes,base-client-nodebase-txpoolbase-client-nodeBinaries
base-reth-nodebase-cli-utils,base-client-node,base-flashblocks,base-metering,base-txpoolPR Changes
sharedcrates don't depend onclientFollow ups