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

Bump alloy to 0.8.0 #13268

Merged
merged 9 commits into from
Dec 10, 2024
Merged

Bump alloy to 0.8.0 #13268

merged 9 commits into from
Dec 10, 2024

Conversation

moricho
Copy link
Contributor

@moricho moricho commented Dec 10, 2024 •
edited
Loading

Bumped alloy related dependencies

  • alloy: 0.8.0
  • core: 0.8.15
  • op-alloy: 0.8.0
  • revm-inspector: 0.13.0

The main changes in this PR are to support Typed2718 trait introduced in alloy-rs/alloy#1746

@moricho moricho requested a review from rkrasiuk as a code owner December 10, 2024 13:49
@mattsse
Copy link
Collaborator

mattsse commented Dec 10, 2024

supersedes #13077

@mattsse mattsse added the A-dependencies Pull requests or issues that are about dependencies label Dec 10, 2024
moricho
moricho commented Dec 10, 2024
View reviewed changes
crates/optimism/primitives/src/transaction/signed.rs
@@ -228,7 +229,7 @@ impl alloy_rlp::Encodable for OpTransactionSigned {

fn length(&self) -> usize {
let mut payload_length = self.encode_2718_len();
if !self.is_legacy() {
if !Encodable2718::is_legacy(self) {
Copy link
Contributor Author

@moricho moricho Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In some parts, we have to specify Encodable2718 or Typed2718 specifically to use is_legacy, but the result will be the same.
Should we consolidate the is_legacy method to one or the other on alloy side?

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, I think Encodable2718 should just have Typed2718 as supertrait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks for the suggestion. I'll address this later

@mattsse mattsse mentioned this pull request Dec 10, 2024
Merged
@moricho moricho requested a review from joshieDo as a code owner December 10, 2024 14:09
moricho
moricho commented Dec 10, 2024
View reviewed changes
crates/rpc/rpc-server-types/src/result.rs
@@ -162,7 +162,6 @@ pub fn block_id_to_str(id: BlockId) -> String {
format!("hash {}", h.block_hash)
}
}
BlockId::Number(n) if n.is_number() => format!("number {n}"),
BlockId::Number(n) => format!("{n}"),
Copy link
Contributor Author

@moricho moricho Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Due to alloy-rs/alloy#1765, we can remove the branching on whether BlockNumberOrTag is a number or not

klkvr
klkvr previously requested changes Dec 10, 2024
View reviewed changes
crates/net/eth-wire-types/src/broadcast.rs Outdated Show resolved Hide resolved
crates/optimism/primitives/src/transaction/signed.rs
@@ -228,7 +229,7 @@ impl alloy_rlp::Encodable for OpTransactionSigned {

fn length(&self) -> usize {
let mut payload_length = self.encode_2718_len();
if !self.is_legacy() {
if !Encodable2718::is_legacy(self) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

right, I think Encodable2718 should just have Typed2718 as supertrait

crates/primitives-traits/src/transaction/mod.rs Show resolved Hide resolved
mattsse
mattsse approved these changes Dec 10, 2024
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.

lgtm

@klkvr making Typed2718 a super trait of Encodeable is perhaps not worth it but would be nicer

@moricho moricho requested a review from klkvr December 10, 2024 17:39
@moricho
Copy link
Contributor Author

moricho commented Dec 10, 2024

Arbitrary tests for OpTransactionSigned generated by maybe_generate_test fails with stack overflow error. Perhaps Arbitrary implementation for OpTransactionSigned causes this...?

@mattsse mattsse added this pull request to the merge queue Dec 10, 2024
Merged via the queue into paradigmxyz:main with commit 73f1583 Dec 10, 2024
42 checks passed
@moricho moricho deleted the bump-alloy branch December 11, 2024 02:09
lean-apple pushed a commit to lean-apple/reth that referenced this pull request Dec 12, 2024
@moricho @mattsse @lean-apple
Bump alloy to 0.8.0 (paradigmxyz#13268)
f47a205 
Co-authored-by: Matthias Seitz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@emhane emhane Awaiting requested review from emhane

@fgimenez fgimenez Awaiting requested review from fgimenez fgimenez is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk rkrasiuk is a code owner

@joshieDo joshieDo Awaiting requested review from joshieDo joshieDo is a code owner

@klkvr klkvr Awaiting requested review from klkvr klkvr is a code owner

Assignees
No one assigned
Labels
A-dependencies Pull requests or issues that are about dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@moricho @mattsse @klkvr