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 serialize/deserialize to ElGamalPubkey and AeCiphertext #33760

Closed
wants to merge 2 commits into from
Closed

Add serialize/deserialize to ElGamalPubkey and AeCiphertext #33760

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 19, 2023

Problem

Allow serialisation derivatives for AeCiphertextVisitor , ElGamalCiphertext and ElGamalPubkey in order to support token-22 extensions serialisation. See solana-labs/solana-program-library#5437

@buffalojoec
Copy link
Contributor

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

There is some duplication of work, but if it's done at a lower level, then we can eventually remove the ones in token-2022. @samkim-crypto do you think it's ok to add these directly into the zk-token-sdk?

Comment on lines +3 to +6
use serde::{
de::{self, SeqAccess, Visitor},
Deserialize, Deserializer,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Since serde is only imported when the target_os is not solana, this PR is creating downstream build errors when trying to build on-chain programs: https://github.com/solana-labs/solana/actions/runs/6567978881/job/17850122613?pr=33760

Can you wrap all of these in #[cfg(not(target_os = "solana"))] or #[cfg_attr(not(target_os = "solana"), derive(Serialize, Deserialize))]?

Comment on lines +57 to +68
fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: SeqAccess<'de>,
{
let mut arr = [0u8; AE_CIPHERTEXT_LEN];
for i in 0..AE_CIPHERTEXT_LEN {
arr[i] = seq
.next_element()?
.ok_or(de::Error::invalid_length(i, &self))?;
}
Ok(AeCiphertext(arr))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to serialize / deserialize with byte arrays? The serializers pointed out by Joe all use string representations https://github.com/solana-labs/solana-program-library/blob/acabef3680e972baf8e9a47531c35eaa45405740/token/program-2022/src/serialization.rs#L112-L147, and I imagine we'll want to stick with that

@samkim-crypto
Copy link
Contributor

Yes, I think it makes more sense to add serialization in zk-token-sdk. We can eventually remove the ones in token-2022.

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 6, 2023
@github-actions github-actions bot closed this Nov 13, 2023
@samkim-crypto samkim-crypto reopened this Nov 13, 2023
@github-actions github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 14, 2023
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 28, 2023
@github-actions github-actions bot closed this Dec 5, 2023
@samkim-crypto samkim-crypto reopened this Dec 5, 2023
@github-actions github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 6, 2023
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 21, 2023
@github-actions github-actions bot closed this Dec 28, 2023
@samkim-crypto samkim-crypto reopened this Dec 28, 2023
@github-actions github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 29, 2023
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 12, 2024
@ghost
Copy link
Author

ghost commented Jan 18, 2024

Sorry for the delay. Got busy with a lot of other things. Planning to work on this, this weekend

Repository owner closed this by deleting the head repository Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution need:merge-assist stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants