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

feat: Alloy type migration #4674

Merged
merged 21 commits into from
Sep 23, 2023
Merged

feat: Alloy type migration #4674

merged 21 commits into from
Sep 23, 2023

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented Sep 19, 2023 •
edited
Loading

First commit is 90% automated with regexes

TODO:

@DaniPopes DaniPopes marked this pull request as draft September 19, 2023 22:49
@codecov
Copy link

codecov bot commented Sep 19, 2023 •
edited
Loading

Codecov Report

Merging #4674 (68c5aaf) into alloy (a792841) will decrease coverage by 0.17%.
Report is 6 commits behind head on alloy.
The diff coverage is 83.85%.

Impacted file tree graph

Files Changed Coverage Δ
bin/reth/src/args/secret_key.rs 0.00% <ø> (ø)
bin/reth/src/args/utils.rs 75.34% <ø> (ø)
bin/reth/src/chain/import.rs 6.89% <0.00%> (ø)
bin/reth/src/cli/config.rs 0.00% <0.00%> (ø)
bin/reth/src/db/list.rs 1.47% <ø> (ø)
bin/reth/src/debug_cmd/execution.rs 1.12% <0.00%> (ø)
bin/reth/src/node/mod.rs 26.11% <0.00%> (ø)
crates/consensus/auto-seal/src/lib.rs 0.00% <0.00%> (ø)
crates/net/dns/src/tree.rs 86.04% <ø> (ø)
crates/net/eth-wire/src/disconnect.rs 82.66% <ø> (ø)
... and 142 more

... and 11 files with indirect coverage changes

Flag Coverage Δ
integration-tests 16.55% <9.47%> (+<0.01%) ⬆️
unit-tests 63.42% <82.69%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 32.11% <45.45%> (-0.07%) ⬇️
blockchain tree 83.80% <100.00%> (+0.06%) ⬆️
pipeline 88.53% <92.10%> (ø)
storage (db) 73.76% <98.93%> (+0.06%) ⬆️
trie 94.76% <100.00%> (+0.03%) ⬆️
txpool 50.29% <100.00%> (+<0.01%) ⬆️
networking 77.13% <100.00%> (-0.02%) ⬇️
rpc 57.55% <65.83%> (-0.35%) ⬇️
consensus 63.15% <84.00%> (+0.05%) ⬆️
revm 28.40% <54.54%> (+0.06%) ⬆️
payload builder 8.32% <0.00%> (ø)
primitives 86.35% <91.20%> (-0.21%) ⬇️

@mattsse mattsse mentioned this pull request Sep 20, 2023
Open
@DaniPopes DaniPopes marked this pull request as ready for review September 20, 2023 15:59
mattsse
mattsse approved these changes Sep 20, 2023
View reviewed changes
Copy link
Collaborator

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

amazing, only 1 q re Bytes type

this PR is blocked by revm, right?

crates/primitives/src/transaction/mod.rs
@@ -836,10 +837,10 @@ impl TransactionSigned {
/// Returns the enveloped encoded transactions.
///
/// See also [TransactionSigned::encode_enveloped]
pub fn envelope_encoded(&self) -> bytes::Bytes {
pub fn envelope_encoded(&self) -> Bytes {
Copy link
Collaborator

Choose a reason for hiding this comment

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

which bytes type is this here?

I'd like to keep bytes::Bytes and not a hex wrapper

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 is alloy Bytes, I don't see why have multiple types in a public interface

@mattsse mattsse added S-blocked This cannot more forward until something else changes C-debt Refactor of code section that is hard to understand or maintain labels Sep 20, 2023
@DaniPopes
Copy link
Member Author

Blocked by Revm migration, and Alloy release

@mattsse mattsse mentioned this pull request Sep 21, 2023
Closed
1 task
This was referenced Sep 21, 2023
Closed
Closed
@DaniPopes DaniPopes mentioned this pull request Sep 22, 2023
Closed
@gakonst
Copy link
Member

gakonst commented Sep 22, 2023 •
edited
Loading

As discussed, let's no longer block this PR on the releases / merging of the deps, let's drive it to the end / remove ethers from as many places as we can, and merge on Monday after we release alpha.9.

@DaniPopes DaniPopes changed the base branch from main to alloy September 23, 2023 00:02
@DaniPopes
Copy link
Member Author

DaniPopes commented Sep 23, 2023 •
edited
Loading

Merging into alloy branch and continuing there (#4737)

@DaniPopes DaniPopes merged commit ccae4eb into paradigmxyz:alloy Sep 23, 2023
18 checks passed
@DaniPopes DaniPopes deleted the alloy branch September 23, 2023 00:31
@DaniPopes DaniPopes self-assigned this Sep 23, 2023
@DaniPopes DaniPopes added C-enhancement New feature or request and removed S-blocked This cannot more forward until something else changes C-debt Refactor of code section that is hard to understand or maintain labels Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gakonst gakonst gakonst left review comments

@mattsse mattsse mattsse approved these changes

@shekhirin shekhirin Awaiting requested review from shekhirin

@joshieDo joshieDo Awaiting requested review from joshieDo

@rakita rakita Awaiting requested review from rakita

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk

@Rjected Rjected Awaiting requested review from Rjected

@onbjerg onbjerg Awaiting requested review from onbjerg

Assignees

@DaniPopes DaniPopes

Labels
C-enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DaniPopes @gakonst @mattsse