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: Add tx_type() and encoded_length() on PoolTransaction trait #1500

Merged

Conversation

leruaa
Copy link
Contributor

@leruaa leruaa commented Feb 22, 2023

Closes #1485

@leruaa leruaa changed the title feat: Add methods on PoolTransaction trait feat: Add tx_type() and encoded_length() on PoolTransaction trait Feb 22, 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.

lgtm

Comment on lines +388 to +391
/// Returns the length of the rlp encoded object
fn encoded_length(&self) -> usize {
self.transaction.length()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is sufficient for now,
perhaps we should think about adding this value to the ValidatedTransaction instead so we don't have to do this all the time when we send this transaction over p2p

Copy link
Contributor Author

@leruaa leruaa Feb 22, 2023

Choose a reason for hiding this comment

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

Yeah, I was just writing a comment about this, I wasn't able to access a ValidPoolTransaction while impl encoded_length() on PooledTransaction, so it must be something I don't understand.

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 think I get it. I'll wait this get merged and will do another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah let's use the same pattern as in Transaction ECRECOVER where we don't want to recalculate the signer all the time because it's expensive so we put it in TransactionEcRecovered or whatever.

@mattsse mattsse added C-enhancement New feature or request A-tx-pool Related to the transaction mempool labels Feb 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2023

Codecov Report

Merging #1500 (e9f9a08) into main (0fc9f67) will increase coverage by 0.11%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1500      +/-   ##
==========================================
+ Coverage   76.00%   76.11%   +0.11%     
==========================================
  Files         358      358              
  Lines       42660    42723      +63     
==========================================
+ Hits        32423    32519      +96     
+ Misses      10237    10204      -33     
Flag Coverage Δ
integration-tests 21.81% <0.00%> (-0.05%) ⬇️
unit-tests 70.58% <0.00%> (+0.13%) ⬆️

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

Impacted Files Coverage Δ
crates/transaction-pool/src/test_utils/mock.rs 66.89% <0.00%> (+4.83%) ⬆️
crates/transaction-pool/src/traits.rs 3.19% <0.00%> (-0.22%) ⬇️
crates/net/eth-wire/src/ethstream.rs 83.97% <0.00%> (-3.07%) ⬇️
crates/net/network/src/session/mod.rs 76.59% <0.00%> (-1.36%) ⬇️
crates/net/eth-wire/src/disconnect.rs 82.55% <0.00%> (-1.03%) ⬇️
crates/net/eth-wire/src/capability.rs 74.26% <0.00%> (-0.74%) ⬇️
crates/net/eth-wire/src/p2pstream.rs 79.41% <0.00%> (-0.42%) ⬇️
crates/stages/src/stages/hashing_storage.rs 95.38% <0.00%> (-0.28%) ⬇️
bin/reth/src/args/rpc_server_args.rs 71.64% <0.00%> (ø)
crates/net/discv4/src/lib.rs 66.23% <0.00%> (+0.14%) ⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mattsse mattsse merged commit 2e73463 into paradigmxyz:main Feb 22, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend PoolTransaction trait with tx_type and length
4 participants