Skip to content
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

feat(tx-pool): Add new TransactionPool trait for getting the best transactions with blob fee #4834

Merged
merged 3 commits into from
Sep 29, 2023

Conversation

PanGan21
Copy link
Contributor

Closes: #4822

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #4834 (18aa88a) into main (66fc196) will decrease coverage by 0.53%.
Report is 27 commits behind head on main.
The diff coverage is 0.00%.

Impacted file tree graph

Files Coverage Δ
crates/transaction-pool/src/lib.rs 37.25% <0.00%> (-1.13%) ⬇️
crates/transaction-pool/src/noop.rs 9.94% <0.00%> (-0.35%) ⬇️
crates/transaction-pool/src/pool/blob.rs 14.28% <0.00%> (-1.21%) ⬇️
crates/transaction-pool/src/pool/mod.rs 48.38% <0.00%> (-0.62%) ⬇️
crates/transaction-pool/src/traits.rs 11.96% <0.00%> (-0.54%) ⬇️
crates/transaction-pool/src/pool/txpool.rs 72.54% <0.00%> (-1.32%) ⬇️

... and 143 files with indirect coverage changes

Flag Coverage Δ
integration-tests 15.53% <0.00%> (-0.65%) ⬇️
unit-tests 62.68% <0.00%> (-0.58%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 32.02% <ø> (-0.10%) ⬇️
blockchain tree 83.76% <ø> (+0.06%) ⬆️
pipeline 88.45% <ø> (ø)
storage (db) 73.31% <ø> (-0.03%) ⬇️
trie 94.48% <ø> (-0.04%) ⬇️
txpool 49.03% <0.00%> (-0.48%) ⬇️
networking 76.42% <ø> (-0.05%) ⬇️
rpc 57.80% <ø> (+0.08%) ⬆️
consensus 61.06% <ø> (-1.53%) ⬇️
revm 28.54% <ø> (+0.21%) ⬆️
payload builder 8.16% <ø> (-0.05%) ⬇️
primitives 85.28% <ø> (-1.10%) ⬇️

@@ -245,6 +245,39 @@ impl<T: TransactionOrdering> TxPool<T> {
}
}

/// Returns an iterator that yields transactions that are ready to be included in the block with
/// the given base fee and optional blob fee.
pub(crate) fn best_transactions_with_attributes(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mattsse please let me know your opinion about this function and then I can also write the related unit tests

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.

this is great, ty.

one suggestion to turn the arguments into a new struct type

Comment on lines 424 to 425
base_fee: u64,
blob_fee: Option<u64>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want a new struct type for this, this way it's easier to call and it won't break if we need more attributes in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added BestTransactionsAttributes struct as the argument

basefee: u64,
blob_fee: Option<u64>,
) -> Vec<Arc<ValidPoolTransaction<T>>> {
unreachable!()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
unreachable!()
Vec::new()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mattsse mattsse marked this pull request as ready for review September 28, 2023 14:15
@mattsse mattsse added C-enhancement New feature or request A-tx-pool Related to the transaction mempool labels Sep 28, 2023
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.

this is great, tysm!

@mattsse mattsse added the M-changelog This change should be included in the changelog label Sep 29, 2023
@mattsse mattsse added this pull request to the merge queue Sep 29, 2023
Merged via the queue into paradigmxyz:main with commit 25aed7b Sep 29, 2023
23 checks passed
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-enhancement New feature or request M-changelog This change should be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new TransactionPool trait for getting the best transactions with blob fee
2 participants