Skip to content

Serialize Vec<u8> in RPC struct fields to 0x-prefixed-hex-string #3355

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

Merged
merged 4 commits into from
Apr 10, 2025

Conversation

Kailai-Wang
Copy link
Collaborator

Context

As topic - it uses ethers::types::Bytes to do this.

Alternatively we could write a manual serializer/deserializer, but I think that's too verbose.

Extra struct is defined to keep the original struct SCALE-codecable as we still use it internally. E.g. AesOutput vs SerdeAesOutput

@Kailai-Wang Kailai-Wang requested a review from a team April 10, 2025 00:20
@Kailai-Wang Kailai-Wang self-assigned this Apr 10, 2025
Copy link

linear bot commented Apr 10, 2025

pub n: Vec<u8>,
#[serde_as(as = "serde_with::hex::Hex")]
pub e: Vec<u8>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we make Rsa3072PubKey to use fixed-size arrays to guarantee 3072-bit key length as below?:

pub struct Rsa3072PubKey {
    pub n: [u8; 384], 
    pub e: [u8; 3], 
}

pub e: Vec<u8>,
}

#[derive(Serialize, Deserialize)]
pub struct SerdeRsa3072PubKey {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to rename this to SerdeRsaPubKey because the struct can handle RSA public keys of any length, not just 3072-bit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean this struct itself is a counterpart of Rsa3072PubKey - so if we want to rename it, we have to rename both.

But Rsa3072PubKey is serialised from public key of ShieldingKey - which is a 3072 bit rsa key pair. So actually I think it's fine

@Kailai-Wang Kailai-Wang requested a review from a team April 10, 2025 07:06
Copy link
Member

@felixfaisal felixfaisal left a comment

Choose a reason for hiding this comment

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

Bytes seems like a quick solution, I do have one question other than that LGTM


impl From<Rsa3072PubKey> for SerdeRsa3072PubKey {
fn from(k: Rsa3072PubKey) -> Self {
Self { n: k.n.into(), e: k.e.into() }
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the wrapper, if the Bytes support conversion to Vec, this might get cumbersome for other types we want to ensure 0x, if only here then i guess it's fine.

@Kailai-Wang Kailai-Wang enabled auto-merge (squash) April 10, 2025 08:41
@Kailai-Wang Kailai-Wang merged commit 9bc3ba2 into dev Apr 10, 2025
21 checks passed
@Kailai-Wang Kailai-Wang deleted the p-1451-use-consistent-0x-prefix-for-hex-string branch April 10, 2025 11:30
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