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

Follow-up for canonical se/de after an additional round of review #588

Merged
merged 4 commits into from
Sep 25, 2023

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Sep 22, 2023

  • Removed all enum attributes. The manual implementation of the traits can do the same.
  • Removed clone for each Input required to call into_full. It is possible because of a new Empty<Type> type that implements canonical se/de with minimal implementation.
  • Removed SIZE_NO_DYNAMIC because it is not used in the codebase.
  • Removed SerializedSizeFixed because we don't have any specific logic that uses it.
  • Moved all methods from SerializedSize to Serialize. It looks unnatural that #[derive(Serialize)] implements 3 traits. Instead, it implements on trait now.
  • Removed SIZE_STATIC because it is impossible to calculate in the case of enums(or if the struct has an enum field). Supporting something like this requires another way of doing that, plus it doesn't provide a lot of wins for us right now because #[inline(always] handle that well enough.
  • Removed panics during size calculation. Instead we use saturating_add. If the size is incorrect we will fail in another place.
  • Removed usage of num_enum and replaced with canonical se/de.
  • Removed calculation of the discriminant on macro level. Replaced with native rust calculation that covers all cases.

- Removed all enum attributes. The manual implementation of the traits can do the same.
- Removed `clone` for each `Input` required to call `into_full`. It is possible because of a new `Empty<Type>` type that implements canonical se/de with minimal implementation.
- Removed `SIZE_NO_DYNAMIC` because it is not used in the codebase.
- Removed `SerializedSizeFixed` because we don't have any specific logic that uses it.
- Moved all methods from `SerializedSize` to `Serialize`. It looks unnatural that `#[derive(Serialize)]` implements 3 traits. Instead, it implements on trait now.
- Removed `SIZE_STATIC` because it is impossible to calculate in the case of enums(or if the `struct` has an enum field). Supporting something like this requires another way of doing that, plus it doesn't provide a lot of wins for us right now because `#[inline(always]` handle that well enough.
- Removed panics during size calculation. Instead we use `saturating_add`. If the size is incorrect we will fail in another place.
- Removed usage of `num_enum` and replaced with canonical se/de.
- Removed calculation of the discriminant on macro level. Replaced with native rust calculation that covers all cases.
@xgreenx xgreenx requested review from Dentosal and a team September 22, 2023 15:05
@xgreenx xgreenx self-assigned this Sep 22, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -451,6 +453,81 @@ impl From<Mint> for Transaction {
}
}

impl Serialize for Transaction {
Copy link
Member

@Voxelot Voxelot Sep 22, 2023

Choose a reason for hiding this comment

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

What's the benefit of implementing this kind of boilerplate logic manually? Does it just add unnecessary complexity to the macros by trying to automate this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can use the peek function, which removes the Clone requirement from the trait Input: Clone. Skipping of the discriminant is a very "magic" thing and I prefer to do that manually because we need it only in one place for Transaction.

@Dentosal
Copy link
Member

Removed all enum attributes. The manual implementation of the traits can do the same.

I dislike the added copy-paste, as that makes it possible to accidentally apply a change to only a part of the branches.

Removed clone for each Input required to call into_full. It is possible because of a new Empty type that implements canonical se/de with minimal implementation.

The empty type changes I really like, and this is a nice additional benefit.

Removed SIZE_NO_DYNAMIC because it is not used in the codebase.
Removed SerializedSizeFixed because we don't have any specific logic that uses it.
Moved all methods from SerializedSize to Serialize. It looks unnatural that #[derive(Serialize)] implements 3 traits. Instead, it implements on trait now.

Sure.

Removed SIZE_STATIC because it is impossible to calculate in the case of enums(or if the struct has an enum field). Supporting something like this requires another way of doing that, plus it doesn't provide a lot of wins for us right now because #[inline(always] handle that well enough.

I though there was an attempt to get const-time size calculations. If this is no longer a goal, then removing the const-time size access is a good thing.

Removed panics during size calculation. Instead we use saturating_add. If the size is incorrect we will fail in another place.

I dislike this, as it has a possibility to silently fail, and could hide implementation bugs. Shouldn't matter much, but if we change the serialization itself at some point, this makes it a bit harder.

Removed usage of num_enum and replaced with canonical se/de.

Sure.

Removed calculation of the discriminant on macro level. Replaced with native rust calculation that covers all cases.

The new code seems good enough.

Copy link
Member

@Dentosal Dentosal left a comment

Choose a reason for hiding this comment

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

I disagree with some design decisions here, as mentioned above.
Still, I don't think it matters enough to keep discussing this, so lets just go ahead and merge.

@xgreenx xgreenx added this pull request to the merge queue Sep 25, 2023
Merged via the queue into master with commit 5e2f432 Sep 25, 2023
36 checks passed
@xgreenx xgreenx deleted the feature/canonical-follow-up-green-review branch September 25, 2023 14:10
@xgreenx xgreenx mentioned this pull request Sep 27, 2023
Dentosal added a commit to FuelLabs/fuel-core that referenced this pull request Sep 27, 2023
Addresses breaking fuel-vm changes from
FuelLabs/fuel-vm#582,
FuelLabs/fuel-vm#578,
FuelLabs/fuel-vm#588 and
FuelLabs/fuel-vm#587.

Waiting for a new fuel-vm release.

---------

Co-authored-by: Green Baneling <[email protected]>
crypto523 added a commit to crypto523/fuel-core that referenced this pull request Oct 7, 2024
Addresses breaking fuel-vm changes from
FuelLabs/fuel-vm#582,
FuelLabs/fuel-vm#578,
FuelLabs/fuel-vm#588 and
FuelLabs/fuel-vm#587.

Waiting for a new fuel-vm release.

---------

Co-authored-by: Green Baneling <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants