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

dusk-core: add serde support #3046

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

d-sonuga
Copy link
Collaborator

@d-sonuga d-sonuga commented Nov 22, 2024

This is to add the feature described in #2773.
I've only added serde support for a single event struct - MoonlightTransactionEvent.
I need some thoughts on whether what I have now is okay.

I've added serde support for the event structs.
I also addedPartialEq derive impls for SlashEvent and StakeEvent so that they could be compared in tests.
Changed dusk-jubjub and bls12_381-bls versions to the ones that now have serde support.

@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch from 8b9d6ae to 458f951 Compare November 22, 2024 16:50
@d-sonuga d-sonuga requested a review from moCello November 22, 2024 16:52
@HDauven HDauven requested a review from Neotamandua November 22, 2024 16:57
@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch from 458f951 to 9114601 Compare November 25, 2024 09:27
execution-core/Cargo.toml Outdated Show resolved Hide resolved
execution-core/src/transfer.rs Outdated Show resolved Hide resolved
@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch 3 times, most recently from e329820 to 645e9bc Compare November 25, 2024 11:10
@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch from 645e9bc to fc895f6 Compare December 23, 2024 12:29
@d-sonuga d-sonuga changed the title execution-core: added serde support for MoonlightTransactionEvent dusk-core: add serde support Dec 23, 2024
@d-sonuga d-sonuga requested a review from Neotamandua December 23, 2024 12:33
@d-sonuga d-sonuga marked this pull request as ready for review December 23, 2024 12:33
core/tests/serde.rs Outdated Show resolved Hide resolved
core/tests/serde.rs Outdated Show resolved Hide resolved
core/Cargo.toml Show resolved Hide resolved
core/Cargo.toml Outdated Show resolved Hide resolved
core/tests/serde.rs Outdated Show resolved Hide resolved
core/src/serde_support.rs Outdated Show resolved Hide resolved
@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch 2 times, most recently from 0b0c5c4 to ca73df8 Compare December 23, 2024 17:49
@d-sonuga d-sonuga requested a review from Neotamandua December 23, 2024 18:07
@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch from ca73df8 to 4951ee5 Compare December 24, 2024 14:23
@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch from 4951ee5 to 21e5150 Compare December 27, 2024 10:35
core/src/serde_support.rs Outdated Show resolved Hide resolved
core/src/serde_support.rs Outdated Show resolved Hide resolved
core/src/serde_support.rs Outdated Show resolved Hide resolved
core/Cargo.toml Outdated Show resolved Hide resolved
@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch 2 times, most recently from be97d5f to dec5bcd Compare December 30, 2024 11:29
@d-sonuga d-sonuga requested a review from moCello December 30, 2024 11:30
@d-sonuga d-sonuga requested a review from Neotamandua December 30, 2024 11:30
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

Is there a reason why the traits are re-implemented for each type?
You might also be able to simply derive types that are composed of other types that implement Serialize and Deserialize already:
For example for StakeFundOwner adding this on top of its declaration will be enough:

#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub enum StakeFundOwner {
...
}

This is probably true for many other types as well.

core/tests/serde.rs Show resolved Hide resolved
core/src/serde_support.rs Show resolved Hide resolved
@d-sonuga
Copy link
Collaborator Author

d-sonuga commented Dec 30, 2024

Is there a reason why the traits are re-implemented for each type? You might also be able to simply derive types that are composed of other types that implement Serialize and Deserialize already: For example for StakeFundOwner adding this on top of its declaration will be enough:

#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub enum StakeFundOwner {
...
}

This is probably true for many other types as well.

The reason is to keep all implementations in one place, for ease of auditing. I think @Neotamandua mentioned something about this at some point before?

Edit: the re-implementations aren't needed.

@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch 2 times, most recently from ff81362 to 21089d6 Compare December 30, 2024 13:47
moCello
moCello previously approved these changes Dec 31, 2024
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

Looks good from my side.
But please wait for a go from @ZER0 and @Neotamandua as well as they are the ones that can judge the serialization approach.

@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch from 21089d6 to 2589132 Compare January 3, 2025 11:41
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