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): Core types migration #6808

Merged
merged 35 commits into from
Jan 25, 2024
Merged

feat(anvil): Core types migration #6808

merged 35 commits into from
Jan 25, 2024

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Jan 15, 2024

Motivation

Migrates Anvil to use alloy. This brings over types from Alloy Primitives, along with custom types which Anvil needs, mainly to support Optimism deposit transactions, and its internal execution logic.

The big diff is mainly concerned with the deletion of a lot of unneeded files.

Only test utilities which are needed on anvil remain on ethers (Related to providers). Followup PRs will remove ethers from anvil tests completely and subsequently remove the ethers dep.

Solution

Move Anvil to use Alloy. A couple of things to note:

  • The impersonated signature is not { v: 0, r: 0, s: 0} anymore, as it's not possible to construct such signature with our new Signature type. It's changed to { v: 1, r: 1, s: 1}.
  • We are using the new alloy signers as well.

* feat: remove most ethers and old anvil core types

* chore: replace handles for providers constructed on actual tests

* finish moving test providers

* chore: switch to decode_revert

* chore: replace with maybe_decode_revert

* u256::decode

* chore: move all of anvil but tests and block subscriptions off ethers

* re-enable opt

* solve nits

* chore: remove more println

* chore: rename to gen
@Evalir Evalir marked this pull request as ready for review January 19, 2024 18:58
@Evalir Evalir requested a review from gakonst January 19, 2024 19:01
crates/anvil/src/eth/backend/executor.rs Outdated Show resolved Hide resolved
crates/anvil/tests/it/sign.rs Outdated Show resolved Hide resolved
crates/anvil/tests/it/sign.rs Outdated Show resolved Hide resolved
crates/anvil/src/eth/sign.rs Outdated Show resolved Hide resolved
crates/anvil/src/eth/sign.rs Outdated Show resolved Hide resolved
crates/anvil/core/src/eth/transaction/optimism.rs Outdated Show resolved Hide resolved
crates/anvil/src/eth/backend/mem/mod.rs Outdated Show resolved Hide resolved
crates/anvil/src/eth/backend/mem/mod.rs Outdated Show resolved Hide resolved
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.

just minor nits to add

crates/anvil/src/eth/api.rs Outdated Show resolved Hide resolved
crates/anvil/src/eth/backend/mem/mod.rs Outdated Show resolved Hide resolved
crates/anvil/tests/it/anvil.rs Outdated Show resolved Hide resolved
@Evalir
Copy link
Member Author

Evalir commented Jan 22, 2024

deny check comes from mdbook's shlex. ignored it for now, although i expect mdbook to release a version soon that patches it.

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 when we figure out why the signed data is diff

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.

what regressions do we expect?

there a few reported rlp/signing issues on alloy

looks like everything's passing here though

crates/anvil/core/src/eth/utils.rs Show resolved Hide resolved
@Evalir
Copy link
Member Author

Evalir commented Jan 23, 2024

@mattsse I expect no regressions for this PR as the test suite is saying everything's okay, but the rlp/signing issues on alloy make me feel a bit uneasy. I think we should put our attention there and fix upstream before we merge this, just to be careful.

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.

last nits

crates/anvil/src/eth/api.rs Show resolved Hide resolved
crates/anvil/tests/it/sign.rs Outdated Show resolved Hide resolved
crates/anvil/tests/it/sign.rs Outdated Show resolved Hide resolved
crates/anvil/tests/it/traces.rs Outdated Show resolved Hide resolved
@Evalir
Copy link
Member Author

Evalir commented Jan 25, 2024

Blocked on alloy-rs/core#499, needs an alloy-rs/alloy followup afterwards with the functionality in the aforementioned PR so we can get the same hashes on typed data as before.

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.

gg

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.

Send and onto the next!

@Evalir Evalir merged commit 4a1ad36 into master Jan 25, 2024
20 checks passed
@Evalir Evalir deleted the evalir/migrate-base-anvil branch January 25, 2024 19:59
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.

5 participants