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

Derive forward attribute #102

Merged
merged 11 commits into from
Jul 14, 2023
Merged

Derive forward attribute #102

merged 11 commits into from
Jul 14, 2023

Conversation

limemloh
Copy link
Contributor

@limemloh limemloh commented Jul 10, 2023

Purpose

Related to Concordium/concordium-rust-smart-contracts#307.
Support forward attribute in derive macros for Serial, Deserial, DeserialWithState and SchemaType.
Should be merged before Concordium/concordium-rust-smart-contracts#314

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

@limemloh limemloh marked this pull request as ready for review July 10, 2023 14:02
@limemloh limemloh requested review from abizjak and DOBEN July 10, 2023 14:02
concordium-contracts-common-derive/CHANGELOG.md Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@abizjak abizjak left a comment

Choose a reason for hiding this comment

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

Looks solid overall, but I have some questions.

  • How much is the module size increase when manually inlining nested enums and deriving serialization, vs. when using forward?

  • I am not sure how it is ensured that the forward with cis2 for example ensures that repr is u8.

  • It would be good to have examples of how an error is manifested when generating the schema using forward, but when the inner type is not an enum. So we can document this error case that is not caught by the compiler.

concordium-contracts-common-derive/CHANGELOG.md Outdated Show resolved Hide resolved
concordium-contracts-common/src/types.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/lib.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/lib.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/lib.rs Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
concordium-contracts-common-derive/src/derive.rs Outdated Show resolved Hide resolved
@limemloh limemloh requested a review from abizjak July 14, 2023 11:49
Copy link
Contributor

@abizjak abizjak left a comment

Choose a reason for hiding this comment

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

Ok, but if you can provide some data on the size difference (or lack thereof) that would be great.

@limemloh
Copy link
Contributor Author

For the wCCD example: the schema is unchanged in size when using forward attribute, but it should produce the exact same thing, so this is expected.
The smart contract module itself went from 131.487 kB (on current main) to 130.862 kB using rustc 1.69.0 and command:

cargo concordium build --schema-embed

With no_std the same example is unchanged between main and this branch

@limemloh limemloh merged commit b65a456 into main Jul 14, 2023
26 checks passed
@limemloh limemloh deleted the derive-forward-attribute branch July 14, 2023 13:52
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.

3 participants