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: add AnyNetwork #383

Merged
merged 9 commits into from
Mar 29, 2024
Merged

feat: add AnyNetwork #383

merged 9 commits into from
Mar 29, 2024

Conversation

leruaa
Copy link
Contributor

@leruaa leruaa commented Mar 24, 2024

Closes #81

crates/rpc-types/src/eth/transaction/request.rs Outdated Show resolved Hide resolved
crates/rpc-types/src/extra.rs Outdated Show resolved Hide resolved
crates/network/src/any/mod.rs 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.

please also remove the catch all fields currently present on some of the rpc-types, e.g.

#[serde(flatten)]
pub extra_info: BTreeMap<String, serde_json::Value>,

/// Support for arbitrary additional fields.
#[serde(flatten)]
pub other: OtherFields,

/// Arbitrary extra fields.
#[serde(flatten)]
pub other: OtherFields,

@onbjerg onbjerg added the enhancement New feature or request label Mar 24, 2024
@leruaa
Copy link
Contributor Author

leruaa commented Mar 24, 2024

Not sure if this should be removed:

#[serde(flatten)]
pub extra_info: BTreeMap<String, serde_json::Value>,

as Block is not part of Network types.

Edit: Looks like Block should be added as a Network type, as mentioned in #267. Maybe I can take that issue as a follow up, and take this into account at that moment?

@onbjerg
Copy link
Member

onbjerg commented Mar 25, 2024

@leruaa Makes sense, pls flag in that issue that we need to make the proper adjustments it for AnyNetwork

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

crates/network/src/any/builder.rs Outdated Show resolved Hide resolved
crates/network/src/any/mod.rs Show resolved Hide resolved
crates/rpc-types/src/eth/transaction/request.rs Outdated Show resolved Hide resolved
crates/rpc-types/src/with_other.rs Outdated Show resolved Hide resolved
crates/rpc-types/src/with_other.rs Outdated Show resolved Hide resolved
Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

Approving, with a followup issue to add tests with various networks' receipt types in #424

@leruaa
Copy link
Contributor Author

leruaa commented Mar 29, 2024

The PR shows a requested change from @mattsse, but I marked all conversations as resolved. Is this a bug?

@prestwich
Copy link
Member

prestwich commented Mar 29, 2024

not a bug, change requests live until the reviewer or an admin dismisses it. cc @mattsse Im cool with merge when you are

@prestwich prestwich requested a review from mattsse March 29, 2024 23:08
@mattsse mattsse merged commit d3a7ff0 into alloy-rs:main Mar 29, 2024
17 checks passed
@leruaa leruaa deleted the any_network branch October 10, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] AnyNetwork
5 participants