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.

Sign up for GitHub

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

Jump to bottom

refactor: chain-specific provider #603

Merged
merged 13 commits into from
Aug 20, 2024
Merged

refactor: chain-specific provider #603

merged 13 commits into from
Aug 20, 2024

Conversation

Wodann
Copy link
Member

@Wodann Wodann commented Aug 16, 2024 •
edited
Loading

This PR makes the Provider and ProviderData types chain-agnostic as well as remaining supporting types.

At this point only the L1ChainSpec is supported for providers. The OptimismChainSpec will follow in follow-up PR.

@Wodann Wodann self-assigned this Aug 16, 2024
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 16, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: b608356

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Wodann Wodann added the no changeset needed This PR doesn't require a changeset label Aug 16, 2024
@Wodann Wodann temporarily deployed to github-action-benchmark August 16, 2024 20:20 — with GitHub Actions Inactive
@Wodann Wodann had a problem deploying to github-action-benchmark August 16, 2024 23:31 — with GitHub Actions Error
@Wodann Wodann temporarily deployed to github-action-benchmark August 16, 2024 23:45 — with GitHub Actions Inactive
Wodann
Wodann commented Aug 17, 2024
View reviewed changes
crates/edr_evm/src/runtime.rs Outdated Show resolved Hide resolved
@Wodann Wodann temporarily deployed to github-action-benchmark August 19, 2024 15:39 — with GitHub Actions Inactive
@Wodann Wodann temporarily deployed to github-action-benchmark August 19, 2024 16:15 — with GitHub Actions Inactive
@Wodann Wodann marked this pull request as ready for review August 19, 2024 17:15
@Wodann Wodann requested a review from agostbiro August 19, 2024 17:15
agostbiro
agostbiro approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@agostbiro agostbiro left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM with some questions/nits.

There were too many changes to review in crates/edr_provider/src/data.rs due to reorganizing the methods, so I just reviewed there what looked new.

crates/edr_eth/src/transaction/signed/eip155.rs Outdated
}
}

pub fn chain_id(transaction: &Eip155) -> u64 {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this could be private and potentially just implemented in the method that seems to be the only usage.

Copy link
Member Author

@Wodann Wodann Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well spotted! Addressed in 7b56c44

crates/edr_eth/src/transaction/signed/eip4844.rs Outdated
@@ -153,6 +208,11 @@ impl From<&Decodable> for transaction::request::Eip4844 {
}
}

/// Total blob gas used by the transaction.
pub fn total_blob_gas(transaction: &Eip4844) -> u64 {
GAS_PER_BLOB * transaction.blob_hashes.len() as u64
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd add parenthesis around transaction.blob_hashes.len() as u64 to make it clear that the cast refers to the usize from the length.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 7b56c44

crates/edr_evm/src/block/builder.rs Outdated
@@ -91,23 +92,25 @@ pub struct BuildBlockResult<ChainSpecT: ChainSpec> {
}

/// A builder for constructing Ethereum blocks.
pub struct BlockBuilder<ChainSpecT: ChainSpec> {
pub struct BlockBuilder<ChainSpecT: ChainSpec, BlockchainErrorT, StateErrorT> {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering what's the reason for adding BlockchainErrorT, StateErrorT to the generics of the struct which necessitates a PhantomData? Declaring them as generics on the methods that need them seems simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! It was a leftover from when one of the trait bounds needed to know about the error type. In a second pass I was able to avoid that but never returned to the old implementation. Thanks for spotting that!

Addressed in b608356

crates/edr_optimism/src/rpc.rs
/// Hash that uniquely identifies the source of the deposit.
#[serde(skip_serializing_if = "Option::is_none")]
#[serde(default, skip_serializing_if = "Option::is_none")]
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering what's the reason for adding default here? Was it an oversight that it wasn't there before?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was an oversight. We don't expect an explicit nil value in the JSON. If it's absent during deserialisation, we use the default value.

crates/edr_optimism/src/rpc/transaction.rs
transaction::Type::Eip4844 => {
Self::Eip4844(transaction_with_signature.try_into()?)
}
transaction::Type::Deposit => unreachable!("already handled"),
Copy link
Member

Choose a reason for hiding this comment

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

How is this handled?

Copy link
Member Author

Choose a reason for hiding this comment

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

At the top of this match statement: https://github.com/NomicFoundation/edr/pull/603/files/5383bede41bd34ec2c368ce583498868b1041a10#diff-1137d5f7224dd6c50cd7b24ce4121a1a0e4ddbc1f2cdaeb93c1694b0ecf5a143R90

I was honestly surprised that rustc doesn't detect that the code path has already been handled.

@Wodann Wodann had a problem deploying to github-action-benchmark August 20, 2024 15:41 — with GitHub Actions Error
@Wodann Wodann temporarily deployed to github-action-benchmark August 20, 2024 15:56 — with GitHub Actions Inactive
@Wodann Wodann merged commit dd9c9e7 into feat/multichain Aug 20, 2024
37 checks passed
@Wodann Wodann deleted the refactor/optimism-provider branch August 20, 2024 22:58
@Wodann Wodann linked an issue Aug 30, 2024 that may be closed by this pull request
Can we query an optimism-specific receipt? #379
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fvictorio fvictorio fvictorio left review comments

@agostbiro agostbiro agostbiro approved these changes

Assignees

@Wodann Wodann

Labels
no changeset needed This PR doesn't require a changeset
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can we query an optimism-specific receipt?
3 participants
@Wodann @fvictorio @agostbiro