Skip to content

Add price bump config#3967

Merged
mattsse merged 2 commits intoparadigmxyz:mainfrom
altugbakan:fee-bump-config
Jul 28, 2023
Merged

Add price bump config#3967
mattsse merged 2 commits intoparadigmxyz:mainfrom
altugbakan:fee-bump-config

Conversation

@altugbakan
Copy link
Contributor

Fixes #3945

This pull request adds a PriceBumpConfigstruct for making fee bump rules configurable.

@altugbakan altugbakan requested a review from mattsse as a code owner July 27, 2023 16:04
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #3967 (c77e52f) into main (f577e14) will decrease coverage by 0.12%.
Report is 5 commits behind head on main.
The diff coverage is 69.23%.

Impacted file tree graph

Files Changed Coverage Δ
crates/transaction-pool/src/lib.rs 32.31% <ø> (ø)
bin/reth/src/args/txpool_args.rs 20.93% <25.00%> (+0.41%) ⬆️
crates/transaction-pool/src/config.rs 96.42% <87.50%> (-3.58%) ⬇️
crates/transaction-pool/src/pool/txpool.rs 59.33% <100.00%> (ø)

... and 17 files with indirect coverage changes

Flag Coverage Δ
integration-tests 15.53% <53.84%> (-0.04%) ⬇️
unit-tests 64.46% <69.23%> (-0.10%) ⬇️

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

Components Coverage Δ
reth binary 27.16% <25.00%> (-0.03%) ⬇️
blockchain tree 83.04% <ø> (ø)
pipeline 89.82% <ø> (ø)
storage (db) 74.30% <ø> (ø)
trie 94.70% <ø> (ø)
txpool 45.27% <88.88%> (-1.32%) ⬇️
networking 77.63% <ø> (-0.09%) ⬇️
rpc 58.71% <ø> (-0.05%) ⬇️
consensus 64.46% <ø> (ø)
revm 33.68% <ø> (ø)
payload builder 6.61% <ø> (ø)
primitives 87.82% <ø> (ø)

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,

almost there already

Comment on lines 85 to 86
#[derive(Debug, Clone)]
pub struct PriceBump(u128);
Copy link
Collaborator

Choose a reason for hiding this comment

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

while I like having default::default,

I think this adds more complexity than it is useful,

I'm okay with just using u128 as fields ins PriceBumpConfig

transaction.as_ref(),
entry.get().transaction.as_ref(),
PRICE_BUMP,
PriceBumpConfig::default().default_price_bump.into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

we want this as a field in PoolConfig

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 made some changes but I'm not sure if it is the intended solution.

@mattsse mattsse added the A-tx-pool Related to the transaction mempool label Jul 27, 2023
@altugbakan altugbakan requested a review from onbjerg as a code owner July 27, 2023 17:07
@altugbakan
Copy link
Contributor Author

@mattsse was

if Self::is_underpriced(
    transaction.as_ref(),
    entry.get().transaction.as_ref(),
    PoolConfig::default().price_bump,
) {

what you meant?

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!

this is great,
once we support blob tx we can easily integrate this as well

Comment on lines +36 to +39

/// Price bump (in %) for the transaction pool underpriced check.
#[arg(long = "txpool.price_bump", help_heading = "TxPool", default_value_t = DEFAULT_PRICE_BUMP)]
pub price_bump: u128,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

@mattsse mattsse added this pull request to the merge queue Jul 28, 2023
Merged via the queue into paradigmxyz:main with commit d2cdd10 Jul 28, 2023
@altugbakan altugbakan deleted the fee-bump-config branch July 28, 2023 10:12
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make fee bump rules configurable in txpool

2 participants