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

Implement transparent new-types in ssz_derive #3644

Closed
wants to merge 15 commits into from

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Oct 17, 2022

Issue Addressed

Related to #3625

Proposed Changes

Allows transparent encoding for new-type structs, just like serde does.

/// The encoded/decoded bytes will be identical to a `Vec<u8>`
#[derive(PartialEq, Debug, Encode, Decode)]
#[ssz(struct_behaviour = "transparent")]
struct TransparentStruct {
    inner: Vec<u8>,
}

Additional Info

I added some top-level docs to ssz_derive to make sure we're keeping track of all the attributes somewhere.

I've also moved the tests for the derive macro from eth2_ssz to eth2_ssz_derive. I did this primarily because cargo-udeps can't detect when a dev-dependency is only used in doc-tests and raises a false positive. Moving the tests caused eth2_ssz to be used in tests and therefore udeps was happy again (there's an ignore method for udeps but it was too blunt IMO). It also kinda makes sense to have those tests in the derive crate rather than where they were.

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Oct 17, 2022
@paulhauner paulhauner self-assigned this Oct 17, 2022
@paulhauner paulhauner changed the base branch from stable to unstable October 17, 2022 01:48
@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Oct 18, 2022
@paulhauner
Copy link
Member Author

paulhauner commented Oct 18, 2022

FYI @realbigsean here's that SSZ impl we discussed last week ☺️ Does it have all the features you need?

IIRC, you need something like this (which is supported):

/// The encoded/decoded bytes will be identical to a `Vec<u8>`
#[derive(PartialEq, Debug, Encode, Decode)]
#[ssz(struct_behaviour = "transparent")]
struct TransparentStruct {
    serialized_thing:: Vec<u8>,
    #[ssz(skip_serializing, skip_deserializing)]
    local_thing: u8,
}

@paulhauner paulhauner marked this pull request as ready for review October 18, 2022 22:27
@paulhauner paulhauner changed the title Allow ssz(transparent) for new-type structs Implement transparent new-types in ssz_derive Oct 18, 2022
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

Looks awesome! The new docs are great. I took a stab at also supporting unnamed fields here:
realbigsean@697e190

I included the suggested changes below in that commit ^

consensus/ssz_derive/src/lib.rs Outdated Show resolved Hide resolved
consensus/ssz_derive/src/lib.rs Outdated Show resolved Hide resolved
);
}

let mut field_names = vec![];
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these field_names are used

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch!

@realbigsean realbigsean added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 19, 2022
paulhauner and others added 2 commits November 14, 2022 17:00
Co-authored-by: realbigsean <seananderson33@GMAIL.com>
@paulhauner paulhauner added ready-for-review The code is ready for review work-in-progress PR is a work-in-progress and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. ready-for-review The code is ready for review labels Nov 14, 2022
@paulhauner
Copy link
Member Author

I took a stab at also supporting unnamed fields

That was great, I included it in the PR! I made a change in 27a5a36 to support the scenario where there are skipped tuple fields before the non-skipped field.

This is ready for review again ☺️

@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 14, 2022
@paulhauner
Copy link
Member Author

Closed in favor of #3861

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants