refactor: separate transaction builders for tx types #1259
Merged
Conversation
This file contains 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
klkvr
requested review from
DaniPopes,
gakonst,
mattsse,
onbjerg,
prestwich and
emhane
as code owners
klkvr
commented
Sep 9, 2024
crates/provider/src/fillers/gas.rs
Outdated
| @@ -210,8 +157,6 @@ impl<N: Network> TxFiller<N> for GasFiller { | |||
| { | |||
| if tx.gas_price().is_some() || tx.access_list().is_some() { | |||
There was a problem hiding this comment.
Our GasFiller automatically treats transaction as legacy if it has an accessList set which is not really correct. We also have a test enforcing this, wondering if this should be changed?
There was a problem hiding this comment.
I believe this is a bug, should be changed
mattsse
approved these changes
Sep 9, 2024
Comment on lines
+313
to
+315
| pub trait RecommendedFillers { | ||
| /// Recommended fillers for this network. | ||
| type RecomendedFillters: TxFiller; |
There was a problem hiding this comment.
I think this makes a ton of sense, so we can network specific defaults
|
should we merge this now or hold off until next breaking release? |
|
I'd like to hold off and see if we can do a smaller patch soon because @tcoratger is flagging a few more missing features |
lwedge99
pushed a commit
to sentioxyz/alloy
that referenced
this pull request
Oct 8, 2024
* refactor: txtype-specific transaction builders * enforce legacy if access list * ignore access list
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
Not all netwoks support 7702 and 4844 transactions, however, current network abstraction requires
TransactionRequestto contain setters for both of those tx types. This results in custom network implementations either having no-op setters/getters on builders, or introducing additional fallible steps during transaction conversion pipeline to reject attempts to construct unsupported transactions.Solution
Extracts part of
TransactionBuildermethods intoTransactionBuilder4844andTransactionBuilder7702. Extracts blob fee filling fromGasFillerintoBlobGasFillerwhich enforcesN::TxRequest: TransactionBuilder4844bound.Given that filling logic now depends on transaction request properties, I've extracted recomended fillers into
RecommendedFillerstrait which can be implemented on custom network implementations, adding support for filling custom transaction types. Right now it is implemented onEthereumandAnyNetwork.This makes it a bit harder to use transaction builder due to need to import 2 additional traits, so I'm wondering if we should consider adding some of the methods directly to it?
PR Checklist