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

Add Serde Feature #22

Merged

Conversation

d-sonuga
Copy link
Contributor

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 f09be04 to e34731a Compare November 26, 2024 12:18
@Neotamandua
Copy link
Member

It should be noted that the decision was made in this PR to consistently use base58 encoding for all keys and key-related types as the way serde handles these datatypes.

This includes secret keys and signatures. This also means that we aim to apply the same encoding rule to serde features of phoenix keys and their respective types, unless they are already defined to be encoded in a different format.

Copy link
Member

@Neotamandua Neotamandua left a comment

Choose a reason for hiding this comment

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

It looks good in general, but having tests for all serde implementations is desirable.

@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch from 9b8d533 to ed52ff1 Compare November 27, 2024 16:07
@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch 2 times, most recently from 0b2cd29 to 393ade1 Compare December 4, 2024 13:03
Copy link
Member

@Neotamandua Neotamandua left a comment

Choose a reason for hiding this comment

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

It would be nice if the tests would also run by default or in the CI. Currently they are feature gated, so cargo test won't let them run by default

Can you add the test with the serde feature to the CI as well?

Cargo.toml Outdated Show resolved Hide resolved
@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch 6 times, most recently from 028d3cd to a28a10f Compare December 10, 2024 13:13
@Neotamandua Neotamandua self-requested a review December 10, 2024 14:34
Copy link
Member

@Neotamandua Neotamandua left a comment

Choose a reason for hiding this comment

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

LGTM from my side

@d-sonuga d-sonuga force-pushed the add-serde-feature-execution-core branch from a28a10f to 6362a7d Compare December 11, 2024 15:18
@d-sonuga d-sonuga requested a review from Neotamandua December 11, 2024 15:19
@Neotamandua Neotamandua linked an issue Dec 12, 2024 that may be closed by this pull request
@d-sonuga d-sonuga changed the title Added serde feature to enable serde Serialize and Deserialize for execution-core Add Serde Feature Dec 17, 2024
@d-sonuga d-sonuga merged commit bc92c34 into dusk-network:main Dec 17, 2024
6 checks passed
@d-sonuga d-sonuga deleted the add-serde-feature-execution-core branch December 17, 2024 15:40
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.

Add serde support
2 participants