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

Use custom derive ser/de instead of std::io #533

Merged
merged 75 commits into from
Sep 3, 2023

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Jul 27, 2023

Closes #443. Initial work migrated from FuelLabs/fuel-tx#190, and the improved and adapted to the current system.

This PR replaces hand-written type serialization code with a macro-derived ones. It removes a lot of error-prone boilerplate code and makes it easier to add new types. The old implementation using io::{Read, Write} was not no_std compatible, but the new generated code is. The new implmentation also allows computing many sizes at compile time.

The new serialization framework is called canonical. It's used similarly to serde:

use fuel_types::canonical::{Serialize, Deserialize};

#[derive(Serialize, Deserialize)]
pub struct ExampleStruct {
    inner: InnerStruct,
    dynamically_sized: Vec<u8>,
    primitive: u64,
    #[canonical(ignore)]
    primitive: u64,
}

Several datatype-level attributes are also accepted:, expecially for enums.

structs

/// The struct is prefixed with the given word.
/// Useful with`#[canonical(inner_discriminant)]`.
pub prefix: Option<TokenStream>,

enums

/// Use a custom type as a discriminant. Mapped using `TryFromPrimitive` and `Into`.
pub discriminant: Option<TokenStream>,
/// Same as `discriminant`, but each field is prefixed with the discriminant,
/// so it's not removed when the enum itself is deserialized.
pub inner_discriminant: Option<TokenStream>,
/// Replaces calculation of the serialized static size with a custom function.
pub serialized_size_static_with: Option<TokenStream>,
/// Replaces calculation of the serialized dynamic size with a custom function.
pub serialized_size_dynamic_with: Option<TokenStream>,
/// Replaces serialization with a custom function.
pub serialize_with: Option<TokenStream>,
/// Replaces deserialization with a custom function.
pub deserialize_with: Option<TokenStream>,
/// Determines whether the enum has a dynamic size when `serialize_with` is used.
pub SIZE_NO_DYNAMIC: Option<TokenStream>,

@Dentosal Dentosal added breaking A breaking api change tech-debt fuel-tx Related to the `fuel-tx` crate. fuel-types Related to the `fuel-types` crate. labels Jul 27, 2023
@Dentosal Dentosal self-assigned this Jul 27, 2023
@xgreenx xgreenx added no changelog Skips the CI changelog check and removed no changelog Skips the CI changelog check labels Jul 31, 2023
Copy link
Contributor

@bvrooman bvrooman left a comment

Choose a reason for hiding this comment

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

Can we now remove the mem_layout macro and related code in this PR?

@bvrooman bvrooman requested a review from a team August 31, 2023 22:58
@bvrooman
Copy link
Contributor

I've reviewed 66 files at this point - I will continue with the last stretch tomorrow.

@Voxelot
Copy link
Member

Voxelot commented Sep 1, 2023

What happens when an enum uses #[canonical(inner_discriminant = Type)], but the corresponding structs of each variant don't use #[canonical(prefix = Type::P1)]? If we always need to specify the prefix along with discriminant, should we have some kind of check at compile time to ensure it's setup correctly?

edit: disregard - after looking at the serialization helpers for inputs, it seems like there's not an easy way to verify this at compile time atm since the prefix may be set by custom logic.

@Voxelot
Copy link
Member

Voxelot commented Sep 2, 2023

Good call on removing discriminant, it was a little confusing how it was suppose to be used compared to inner_descriminant.

Copy link
Member

@Voxelot Voxelot left a comment

Choose a reason for hiding this comment

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

Wrote some local fuzz tests to check the ability of roundtrip serialization between this and the original encoding, and didn't find any issues. I think we're probably good to ship this.

@Dentosal Dentosal added this pull request to the merge queue Sep 3, 2023
Merged via the queue into master with commit 736ff24 Sep 3, 2023
34 checks passed
@Dentosal Dentosal deleted the dento/nostd-serialization branch September 3, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking api change fuel-tx Related to the `fuel-tx` crate. fuel-types Related to the `fuel-types` crate. tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add no_std canonical serialization and deserialization
4 participants