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

feature: abi-json crate #78

Merged
merged 7 commits into from
Jun 8, 2023
Merged

feature: abi-json crate #78

merged 7 commits into from
Jun 8, 2023

Conversation

prestwich
Copy link
Member

@prestwich prestwich commented Jun 8, 2023 •
edited
Loading

Initial re-implementation of ethabi crate's JSON file format.

@prestwich prestwich added the enhancement New feature or request label Jun 8, 2023
@prestwich prestwich requested a review from DaniPopes June 8, 2023 01:22
@prestwich prestwich self-assigned this Jun 8, 2023
@DaniPopes
Copy link
Member

failures unrelated, fixed in #79

DaniPopes
DaniPopes requested changes Jun 8, 2023
View reviewed changes
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

  • What about -json-abi as the name? Matches -dyn-abi

  • As discussed, Item should be an interally tagged enum #[serde(tag = "type")], this removes the need to manually implement deserialize.
    I would also say it makes it collapsible into one file.

crates/abi-json/src/event.rs Outdated

/// JSON representation of a complex event parameter.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct ComplexEventParam {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not one struct, with components as #[serde(default)]/Option<Vec<_>>?

Copy link
Member Author

Choose a reason for hiding this comment

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

I went back and forth on this. in the end, I decided to prevent users from ever accessing properties that don't exist. It is legitimately a different type too. E.g. all ComplexParam.type fields will include "tuple" while no SimpleParam.type fields will

crates/abi-json/src/functions.rs Outdated Show resolved Hide resolved
crates/abi-json/src/param.rs Outdated Show resolved Hide resolved
DaniPopes
DaniPopes requested changes Jun 8, 2023
View reviewed changes
crates/abi-json/Cargo.toml Outdated Show resolved Hide resolved
crates/abi-json/src/error.rs Outdated Show resolved Hide resolved
crates/abi-json/src/functions.rs Outdated Show resolved Hide resolved
crates/abi-json/src/param.rs Outdated Show resolved Hide resolved
@prestwich prestwich enabled auto-merge (squash) June 8, 2023 20:02
@prestwich prestwich merged commit 0a00f01 into main Jun 8, 2023
@DaniPopes DaniPopes deleted the prestwich/abi-json branch June 8, 2023 20:04
@DaniPopes DaniPopes mentioned this pull request Jun 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaniPopes DaniPopes DaniPopes approved these changes

Assignees

@prestwich prestwich

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

Successfully merging this pull request may close these issues.
2 participants
@prestwich @DaniPopes