Skip to content

test: handle commit event properly by pool maintaining #16125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 9, 2025

Conversation

int88
Copy link
Contributor

@int88 int88 commented May 8, 2025

No description provided.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome,

but I think we could simplify this if we only mock the state and state subscription?

this is more like a real e2e test, we could therefor also move this to reth-ethereum-node, but I'd prefer to use state/notification mocking here

Comment on lines 67 to 72
reth-ethereum = { workspace = true, features = [
"test-utils",
"network",
"pool",
"provider",
] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this we can't really do because this is a cyclic dep graph, that only works because this is a dev-dep, so we should flatten these.

or move this test into reth-ethereum-node entreily

Comment on lines 89 to 92
node.inner.provider.clone(),
txpool.clone(),
node.inner.provider.clone().canonical_state_stream(),
executor.clone(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, for this test, we likely just need a way to mock the subscription?

sow e can ditch the entire node setup?

@github-project-automation github-project-automation bot moved this from Backlog to In Progress in Reth Tracker May 8, 2025
@int88 int88 requested a review from Rjected as a code owner May 8, 2025 13:33
@int88
Copy link
Contributor Author

int88 commented May 8, 2025

@mattsse updated!

@@ -56,6 +56,7 @@ reth-payload-primitives.workspace = true
reth-e2e-test-utils.workspace = true
reth-rpc-eth-api.workspace = true
reth-tasks.workspace = true
reth-ethereum = { workspace = true, features = ["pool"] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't import this here either, because this is now cyclic,

we need to import the crates we need, which is the pool crate

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, we def want to test this e2e

@mattsse mattsse added A-tx-pool Related to the transaction mempool C-test A change that impacts how or what we test labels May 9, 2025
@mattsse mattsse added this pull request to the merge queue May 9, 2025
@mattsse
Copy link
Collaborator

mattsse commented May 9, 2025

fyi @Rimeeeeee once this is merged we can also reuse the OkValidator for this

Merged via the queue into paradigmxyz:main with commit db885cc May 9, 2025
45 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Reth Tracker May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tx-pool Related to the transaction mempool C-test A change that impacts how or what we test
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants