Skip to content

feat: Add Latest State TransactionValidator implementation#1498

Merged
mattsse merged 16 commits intoparadigmxyz:mainfrom
chirag-bgh:feat/Add_Latest_State_TransactionValidator_implementation
Mar 10, 2023
Merged

feat: Add Latest State TransactionValidator implementation#1498
mattsse merged 16 commits intoparadigmxyz:mainfrom
chirag-bgh:feat/Add_Latest_State_TransactionValidator_implementation

Conversation

@chirag-bgh
Copy link
Contributor

@chirag-bgh chirag-bgh commented Feb 22, 2023

Closes #1289

@chirag-bgh
Copy link
Contributor Author

chirag-bgh commented Feb 22, 2023

@mattsse
I'm not able to implement all the checks for transaction validation.
Moreover, i need to change the return type for the fn validate_transaction in order to use ? operator and I think there's need to introduce more error fields in PoolError

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.

forgot about the error.
let's change the function so it returns a result instead.

@mattsse mattsse added the A-tx-pool Related to the transaction mempool label Feb 22, 2023
@mattsse mattsse mentioned this pull request Mar 2, 2023
@mattsse
Copy link
Collaborator

mattsse commented Mar 3, 2023

@chirag-bgh this will be a blocker shortly, do you have bandwidth to get this over the line?

@chirag-bgh
Copy link
Contributor Author

@chirag-bgh this will be a blocker shortly, do you have bandwidth to get this over the line?

I will get this done asap.

@chirag-bgh
Copy link
Contributor Author

chirag-bgh commented Mar 3, 2023

@mattsse
For chain_id, I suppose we need to use :

pub enum Transaction {
/// Legacy transaction.
Legacy(TxLegacy),
/// Transaction with an [`AccessList`] ([EIP-2930](https://eips.ethereum.org/EIPS/eip-2930)).
Eip2930(TxEip2930),
/// A transaction with a priority fee ([EIP-1559](https://eips.ethereum.org/EIPS/eip-1559)).
Eip1559(TxEip1559),
}

But I'm not able to figure out how this should be done.

@mattsse
Copy link
Collaborator

mattsse commented Mar 3, 2023

right, we'd need to extend the PoolTransaction trait with

fn chain_id() -> Option<u64>

this is an option because it is optional for legacy tx

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2023

Codecov Report

Merging #1498 (7ccaabe) into main (9dfee49) will decrease coverage by 0.22%.
The diff coverage is 5.83%.

📣 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    #1498      +/-   ##
==========================================
- Coverage   74.48%   74.27%   -0.22%     
==========================================
  Files         377      377              
  Lines       45197    45302     +105     
==========================================
- Hits        33667    33649      -18     
- Misses      11530    11653     +123     
Flag Coverage Δ
integration-tests 20.89% <5.00%> (-0.05%) ⬇️
unit-tests 68.89% <5.83%> (-0.20%) ⬇️

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

Impacted Files Coverage Δ
crates/interfaces/src/consensus.rs 40.00% <0.00%> (-60.00%) ⬇️
crates/rpc/rpc/src/eth/error.rs 5.49% <0.00%> (-0.13%) ⬇️
crates/transaction-pool/src/error.rs 0.00% <0.00%> (ø)
crates/transaction-pool/src/pool/mod.rs 45.33% <ø> (ø)
crates/transaction-pool/src/traits.rs 3.06% <0.00%> (-0.10%) ⬇️
crates/transaction-pool/src/validate.rs 21.87% <0.00%> (-28.13%) ⬇️
crates/transaction-pool/src/test_utils/mock.rs 61.01% <25.00%> (-7.14%) ⬇️
crates/transaction-pool/src/lib.rs 36.79% <33.33%> (-0.71%) ⬇️
crates/transaction-pool/src/test_utils/mod.rs 95.45% <100.00%> (ø)
... and 8 more

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

@chirag-bgh chirag-bgh marked this pull request as ready for review March 4, 2023 01:05
@chirag-bgh chirag-bgh requested a review from mattsse March 4, 2023 03:51
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.

thanks for this.

left a few comments,questions and suggestions.
this looks pretty good, just need to get all checks right.

we should have a closer look at how geth handles some of the checks as well.

@chirag-bgh chirag-bgh requested a review from mattsse March 5, 2023 22:25
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.

great progress,

I think we're almost there.
mostly nits.
the Arc change seems redundant?

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.

please don't mark requests as resolved without addressing them if no changes are made.

last change requests I think.

@chirag-bgh
Copy link
Contributor Author

chirag-bgh commented Mar 7, 2023

please don't mark requests as resolved without addressing them if no changes are made.

last change requests I think.

Sorry, I marked them as resolved because I made the changes on my local and didn't pushed so that I could keep track of unresolved comments. Will do it now.

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.

merging to unblock, this PR has been going on for a bit too long.

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.

Add Latest State TransactionValidator implementation

4 participants