Skip to content
This repository has been archived by the owner on Jan 4, 2023. It is now read-only.

Introduced two new traits for spec's serialization and deserialization #190

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

xgreenx
Copy link
Contributor

@xgreenx xgreenx commented Sep 23, 2022

Overview

Introduced two new traits - Serialize and Deserialize to implement spec's rules.

Each primitive type is padded to have 64 bits alignment.
Vec<u8> is padded with zeroes.
Also implemented derive procedure macro with following rules:

Struct:

  • Iterate over each field and call encode_static/decode_static. It encodes all information required to create an instance of the type into bytes.
  • After, iterate again and call encode_dynamic/decode_dynamic. It encodes the dynamic part of the type. Those methods are implemented only for Vec<T> and [T; N].

So each not-dynamic type is simply encoded/decoded into/from bytes. For all dynamic types(aka Vec<T>), the first iteration leaves sizes, and the second iteration leaves the bytes of inner types.

#[derive(crate::canonical::Serialize, crate:: canonical::Deserialize)]
pub struct Witness {
    dynamic1: Vec<u8>,
    not_dynamic1: u8,
    dynamic2: Vec<u8>,
    not_dynamic2: u16,
    dynamic3: Vec<u8>,
    not_dynamic3: u32,
}

// The serialized layout of `Witness` will look like:
pub struct Witness {
    dynamic1_size: u64,
    not_dynamic1: u64,
    dynamic2_size: u64,
    not_dynamic2: u64,
    dynamic3_size: u64,
    not_dynamic3: u64,
    dynamic1_data: [u8; dynamic1_size + padding],
    dynamic2_data: [u8; dynamic2_size + padding],
    dynamic3_data: [u8; dynamic3_size + padding],
}

The same rule is applied for each variant of the enum. The discriminant of the enum is u64.

Reason

This PR aims to automate the implementation of serialization/deserialization instead of manual implementation.
It reduces the chance of an error(if we test auto-generation properly) in the layout and reduces the amount of code.

Another reason for this PR is to create explicit traits/rules of spec's serialization and deserialization(to avoid FuelLabs/fuel-types#51 (comment)). We have several serialization formats for each project area, and the fuel-core database format is mixed with fuel-tx. For example, all types in the fuel-types have const LEN, which describes the number of bytes. In the fuel-tx, const LEN describes the aligned number of bytes.

State of the change

The PR entirely replaced io::Read and io::Write implementations. But it doesn't have new tests. All old tests are passed, so serialization is the same as before. I will fill it with tests if we are okay with the change.

I cleaned up io::Read and io::Write, but not bytes::SizedBytes. It can be done in follow-up PR.

Also, if we believe in auto-generation(I believe with proper testing), we can remove all manual consts files and replace them with auto-calculated sizes.

Also, maybe we want to move new traits into fuel-types. Because it already contains bytes module with padding.

Adaption of fuel-vm for this change, all tests are passed: FuelLabs/fuel-vm#223

Implemented it for all primitives and used types.
Added derive macro to simplify implementation.
@xgreenx xgreenx self-assigned this Sep 23, 2022
@@ -488,6 +482,7 @@ impl Input {
.chain(recipient)
.chain(nonce.to_be_bytes())
.chain(amount.to_be_bytes())
// TODO: Seems it is a bug and we need to pad `data` with zeros
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the spec, it says "and all other value types are serialized according to the standard transaction serialization".

And seems we need to pad it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@xgreenx xgreenx Sep 29, 2022

Choose a reason for hiding this comment

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

Hmm, but seems spec says that we need to do it with alignment
image

src/io.rs Outdated
}
}

// TODO: Move trait definition to `fuel-types` and derive this implementation for `fuel-asm`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mentioned it in the PR's description=)

Added more comments.
Implemented traits for `[T; N]` arrays. It has many usage of `unsafe`, but I did the same change in this PR: paritytech/parity-scale-codec#299
src/io.rs Outdated
}

impl<const N: usize, T: Deserialize> Deserialize for [T; N] {
fn decode_static<I: Input + ?Sized>(buffer: &mut I) -> Result<Self, Error> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It contains a lot of unsafe, but I did the same change here: paritytech/parity-scale-codec#299

So it should be safe=)

@xgreenx xgreenx changed the title Introduced two new traits for VM serialization and deserialization Introduced two new traits for spec's serialization and deserialization Sep 25, 2022
xgreenx and others added 5 commits September 26, 2022 21:18
…sed in the offset calculation.

Added unit tests for fuel types, primitives, vector encode and decode.
Fixed typo in the error name.
@xgreenx xgreenx marked this pull request as draft November 1, 2022 03:13
@xgreenx
Copy link
Contributor Author

xgreenx commented Nov 1, 2022

It is breaking change and we froze the code for testnet. So this change is paused until the code unfreezes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant