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(anvil): add support for EIP4844 types #7202

Merged
merged 16 commits into from
Mar 12, 2024
Merged

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Feb 21, 2024

Motivation

This does not include any functionality regarding 4844 yet other than support for 4844 variants in the corresponding types.

First of a series of PRs that will add Cancun support to foundry. Tracking here: #5574

Solution

Add support for 4844 transaction types.

@Evalir Evalir mentioned this pull request Feb 21, 2024
10 tasks
@Evalir Evalir changed the title [wip] feat(anvil): add support for EIP4844 types feat(anvil): add support for EIP4844 types Feb 21, 2024
@Evalir Evalir marked this pull request as ready for review February 21, 2024 21:18
@@ -259,6 +259,12 @@ impl FeeHistoryService {
.min(U256::from(t.max_fee_per_gas).saturating_sub(base_fee))
.to::<u64>()
}
// TODO: This probably needs to be extended to extract 4844 info.
Copy link
Member Author

Choose a reason for hiding this comment

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

Will be done in a subsequent PR which modifies FeeHistory to accomodate 4844 changes

Copy link
Member

Choose a reason for hiding this comment

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

we should add a follow up issue for this to track

Copy link
Member Author

@Evalir Evalir Feb 27, 2024

Choose a reason for hiding this comment

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

This should go on #7242

Copy link
Member

Choose a reason for hiding this comment

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

saw it there

Copy link
Member

@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.

this does not support the envoloped format for 4844 which includes the sidecar

I think we want a helper type here that decodes with the sidecar, then extract the sidecar (+ validate), and convert into TypedTx

https://github.com/foundry-rs/foundry/blob/master/crates/anvil/src/eth/api.rs#L942-L942

crates/anvil/core/src/eth/transaction/mod.rs Outdated Show resolved Hide resolved
@Evalir Evalir marked this pull request as draft February 22, 2024 23:50
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

looks good to me, want @mattsse to take a look

@@ -259,6 +259,12 @@ impl FeeHistoryService {
.min(U256::from(t.max_fee_per_gas).saturating_sub(base_fee))
.to::<u64>()
}
// TODO: This probably needs to be extended to extract 4844 info.
Copy link
Member

Choose a reason for hiding this comment

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

we should add a follow up issue for this to track

@Evalir Evalir marked this pull request as ready for review February 28, 2024 06:33
Copy link
Member

@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

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

good w me @mattsse @Evalir lmk wyt on the point about the super enum

@@ -39,11 +42,14 @@ pub fn transaction_request_to_typed(tx: TransactionRequest) -> Option<TypedTrans
gas_price,
max_fee_per_gas,
max_priority_fee_per_gas,
max_fee_per_blob_gas,
Copy link
Member

Choose a reason for hiding this comment

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

I forget, why don't we do this with a From impl / reuse any alloy types code? Is it due to the Deposit tx I guess?

Copy link
Member

Choose a reason for hiding this comment

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

nit there's a Some(126) below i'd like to replace it with a const that indicates the semantic value of the number from op specs

Comment on lines +621 to 624
/// EIP-4844 transaction
EIP4844(Signed<TxEip4844Variant>),
/// op-stack deposit transaction
Deposit(DepositTransaction),
Copy link
Member

Choose a reason for hiding this comment

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

same here i take that we redefine the enum vs reusing from alloy bc of the deposit tx right?

@mattsse do u have any ideas for doing this with a super-enum?

something like enum TypedTransaction { #[flatten] Inner(alloy::TypedTransaction), Deposit(...)?

@@ -259,6 +259,12 @@ impl FeeHistoryService {
.min(U256::from(t.max_fee_per_gas).saturating_sub(base_fee))
.to::<u64>()
}
// TODO: This probably needs to be extended to extract 4844 info.
Copy link
Member

Choose a reason for hiding this comment

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

saw it there

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@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, but this will likely break with new alloy rlp API

but since this most likely also breaks existing variants we can do this

@mattsse mattsse merged commit eef87de into master Mar 12, 2024
19 of 20 checks passed
@mattsse mattsse deleted the evalir/add-4844-types branch March 12, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants