-
Notifications
You must be signed in to change notification settings - Fork 74
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
bug: invalid type: string \"FROST-secp256k1-SHA256-v1\", expected a borrowed string #588
Comments
I think the bug is in /// Deserialize a placeholder ciphersuite field, checking if it's the ciphersuite ID string.
#[cfg(feature = "serde")]
pub(crate) fn ciphersuite_deserialize<'de, D, C>(deserializer: D) -> Result<(), D::Error>
where
D: serde::Deserializer<'de>,
C: Ciphersuite,
{
if deserializer.is_human_readable() {
- let s: &str = serde::de::Deserialize::deserialize(deserializer)?;
+ let s: String = serde::de::Deserialize::deserialize(deserializer)?;
if s != C::ID {
Err(serde::de::Error::custom("wrong ciphersuite"))
} else {
Ok(())
}
} else {
let buffer: [u8; 4] = serde::de::Deserialize::deserialize(deserializer)?;
if buffer != short_id::<C>() {
Err(serde::de::Error::custom("wrong ciphersuite"))
} else {
Ok(())
}
}
} |
Interesting. The fix seems correct but I can't come up with a test case that shows the issue. I'd like to add a test to exercise it. Can you share how are you serializing/deserializing it? |
are you using serde_json in your test? I just use |
Yes. This test passes:
I think it fails when it can't get a reference to the ciphersuite substring inside the JSON string, which usually happens if the string has some escape code, which isn't the case. |
I was doing something similar to this but reading from a file instead. Not sure why your test pass when I couldn't do that on my end :D |
I'm getting the following error:
after serializing and deserializing a
frost::keys::KeyPackage
with json_serde.The text was updated successfully, but these errors were encountered: