refactor: unify transaction validation logic between pool and evm#3463
Merged
refactor: unify transaction validation logic between pool and evm#3463
Conversation
klkvr
changed the title
klkvr
force-pushed
the
klkvr/unify-pool-evm
branch
from
4e3a62a to
394a230
Compare
klkvr
requested review from
0xrusowsky,
fgimenez,
howydev and
legion2002
as code owners
mattsse
approved these changes
Apr 7, 2026
Contributor
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
all of this makes sense
pedantic nit
crates/revm/src/handler.rs
Outdated
Comment on lines
+1579
to
+1590
| /// Context returned by [`TempoEvmHandler::validate_transaction`] with resolved | ||
| /// fee token and key expiry information for use by the transaction pool. | ||
| #[derive(Debug, Clone)] | ||
| pub struct ValidationContext { | ||
| /// The resolved fee token address used to pay for this transaction. | ||
| pub fee_token: Address, | ||
| /// The expiry timestamp of the access key used by this transaction. | ||
| /// Populated for keychain-signed transactions or transactions carrying a KeyAuthorization. | ||
| pub key_expiry: Option<u64>, | ||
| } | ||
|
|
||
| impl<DB, I> TempoEvmHandler<DB, I> |
klkvr
force-pushed
the
klkvr/unify-pool-evm
branch
from
4d5be66 to
09919d8
Compare
klkvr
force-pushed
the
klkvr/unify-pool-evm
branch
from
09919d8 to
ceaad9d
Compare
0xKitsune
approved these changes
Apr 7, 2026
Contributor
0xKitsune
left a comment
0xKitsune
left a comment
There was a problem hiding this comment.
Nice, lgtm. Flagging that it looks like there were a few tests removed that are not covered in the evm crate.
Specifically, these have no EVM side equivalents:
- Spending limits: rejecting txs that exceed the limit, zero remaining balance, or no limit set for the fee token being used
- Access key expiry: rejecting expired keys, edge case where expiry equals current time
- Key expiry getting cached so the pool can evict expired-key txs later
- Rejecting txs where the fee payer is blacklisted
- Key authorization chain_id matching the current chain
- Recipient-scoped call rules rejecting undeployed targets
- Duplicate token limits in key authorizations getting rejected
decofe
added a commit
that referenced
this pull request
Apr 8, 2026
The unify-validation refactor (#3463) moved validate_against_keychain, call_scope_allows_call, and validate_keychain_version out of the pool validator. These tests now belong in the EVM validation layer. Co-Authored-By: grandizzy <38490174+grandizzy@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on #3461
Changes
TempoTransactionValidatorto fully delegate validation of transactions toTempoEvm.This removes a lot of tests (mostly the ones that were covering
validate_against_keychain). My assumption is that validator would now fully inherit EVM test coverage and if some scenarios are not covered there we should just migrate the tests