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 #141

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

d-sonuga
Copy link

@d-sonuga d-sonuga commented Dec 5, 2024

For dusk-network/rusk/issues/2773 and #143.

To serialize AffineNielsPoint and ExtendedNielsPoint, I implemented dusk_bytes::Serializable for them.
But there is currently some uncertainty about the correctness of the approach used to serialize because I don't understand AffinePoint's serialization and I'm unsure about whether or not it will be relevant to AffineNielsPoint and ExtendedNielsPoint.

Edit: AffineNielsPoint and ExtendedNielsPoint have not being changed.

@d-sonuga d-sonuga force-pushed the add-serde-feature branch 3 times, most recently from 85991d8 to c575083 Compare December 6, 2024 09:51
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, but I'm waiting for a second approval from a maintainer of this repository

@d-sonuga d-sonuga force-pushed the add-serde-feature branch 2 times, most recently from 4f06708 to c40b897 Compare December 9, 2024 08:22
Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/dusk/serde_support.rs Outdated Show resolved Hide resolved
@d-sonuga d-sonuga force-pushed the add-serde-feature branch 3 times, most recently from 8eb2269 to 82aa88f Compare December 10, 2024 13:36
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.

Generally, looks good to me. But based on the other two PRs (dusk-network/bls12_381-bls#22 & dusk-network/jubjub-schnorr#30), and for consistency, I would suggest having the tests as integration tests rather than module tests. Also because the current module tests are testing the implementation for Fr, which is in another module.

Edit: After checking back, we can leave it as it is except for the tests for Fr. Can you move them into the respective Dusk.rs file of the Fr module?

@d-sonuga d-sonuga force-pushed the add-serde-feature branch 2 times, most recently from 6ac3bb3 to a8d99dc Compare December 10, 2024 15:24
src/dusk/serde_support.rs Outdated Show resolved Hide resolved
src/dusk/serde_support.rs Outdated Show resolved Hide resolved
@d-sonuga d-sonuga force-pushed the add-serde-feature branch 2 times, most recently from 6dc2c2a to d10ff66 Compare December 11, 2024 12:47
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.

Code looks good to me, but please also update the issue and pr description and add an entry to the changelog

src/dusk/serde_support.rs Outdated Show resolved Hide resolved
@d-sonuga d-sonuga requested a review from moCello December 11, 2024 16:01
@Neotamandua Neotamandua linked an issue Dec 12, 2024 that may be closed by this pull request
src/dusk/serde_support.rs Outdated Show resolved Hide resolved
src/fr/dusk.rs Outdated Show resolved Hide resolved
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.

We can now aim for a 0.14.2 release of this crate tomorrow, so that we can proceed directly with the phoenix-core crate that depends on it.

@d-sonuga d-sonuga merged commit 3662c82 into dusk-network:master Dec 13, 2024
3 checks passed
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 Feature
3 participants