feat(staking): 🎽 IdentityKey is internally represented as bytes
#4151
Conversation
cratelyn
added
the
A-staking
|
NB: while we should eventually move the |
We shouldn't do this, the identity key is staking-specific, which is why it's in the staking component. Also, since the proto lives in the staking proto package, this isn't possible without deprecating the proto type and leaving a duplicate around. So we can plan to not do it. |
| // TODO(kate): | ||
| // decompressed: std::sync::OnceLock<VerificationKey<SpendAuth>>, |
| pub struct IdentityKey { | ||
| /// The *compressed* bytes of the identity verification key. | ||
| bytes: VerificationKeyBytes<SpendAuth>, | ||
| // The *decompressed* identity verification key. | ||
| key: OnceLock<DecompressionResult>, | ||
| } |
There was a problem hiding this comment.
This should just be a wrapper around the VerificationKeyBytes:
pub struct IdentityKey(pub VerificationKeyBytes<SpendAuth>)All we want to be doing here is replacing VerificationKey with VerificationKeyBytes, nothing more or less.
| /// Decompresses this identity key, returning a [`VerificationKey`]. | ||
| /// | ||
| /// This is idempotent and can be called repeatedly, and the bytes will only decompressed | ||
| /// once. | ||
| pub fn key(&self) -> Result<VerificationKey<SpendAuth>, decaf377_rdsa::Error> { | ||
| let Self { bytes, ref key } = *self; | ||
| key.get_or_init(|| VerificationKey::try_from(bytes)).clone() | ||
| } |
There was a problem hiding this comment.
We shouldn't be exposing this as part of the public API, it's not necessary for callers to have a convenient method for.
| key: OnceLock<DecompressionResult>, | ||
| } | ||
|
|
||
| type DecompressionResult = Result<VerificationKey<SpendAuth>, decaf377_rdsa::Error>; |
There was a problem hiding this comment.
We don't need to model this type separately, there should not be anywhere that needs to model it other than signature verification, which already returns an Error.
| impl From<SigningKey<SpendAuth>> for IdentityKey { | ||
| /// An [`IdentityKey`] can be constructed from a [`decaf377_rdsa::SigningKey`]. | ||
| fn from(sk: SigningKey<SpendAuth>) -> Self { | ||
| let vk: VerificationKey<SpendAuth> = sk.into(); | ||
| Self::from(vk) | ||
| } | ||
| } | ||
|
|
||
| impl<'sk> From<&'sk SigningKey<SpendAuth>> for IdentityKey { | ||
| /// An [`IdentityKey`] can be constructed from a reference to a [`decaf377_rdsa::SigningKey`]. | ||
| fn from(sk: &'sk SigningKey<SpendAuth>) -> Self { | ||
| Self::from(*sk) | ||
| } | ||
| } | ||
|
|
||
| impl From<VerificationKey<SpendAuth>> for IdentityKey { | ||
| /// An [`IdentityKey`] can be constructed from a [`decaf377_rdsa::VerificationKey`]. | ||
| fn from(vk: VerificationKey<SpendAuth>) -> Self { | ||
| let bytes: VerificationKeyBytes<SpendAuth> = vk.into(); | ||
| let key: OnceLock<_> = Ok(vk).into(); | ||
|
|
||
| Self { bytes, key } | ||
| } | ||
| } | ||
|
|
||
| impl<'vk> From<&'vk VerificationKey<SpendAuth>> for IdentityKey { | ||
| /// An [`IdentityKey`] can be constructed from a reference to a [`decaf377_rdsa::VerificationKey`]. | ||
| fn from(vk: &'vk VerificationKey<SpendAuth>) -> Self { | ||
| Self::from(*vk) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure it's desirable to have all of these impls. We didn't have them before, do we need them to change the internal representation?
|
|
||
| impl TryFrom<Vec<u8>> for IdentityKey { | ||
| type Error = anyhow::Error; | ||
| fn try_from(bytes: Vec<u8>) -> Result<Self, Self::Error> { | ||
| Self::try_from(bytes.as_slice()) | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<&[u8]> for IdentityKey { | ||
| type Error = anyhow::Error; | ||
| fn try_from(bytes: &[u8]) -> Result<Self, Self::Error> { | ||
| bytes | ||
| .try_into() | ||
| .map(|bytes| Self { | ||
| bytes, | ||
| key: OnceLock::new(), | ||
| }) | ||
| .map_err(Into::into) |
There was a problem hiding this comment.
We should avoid adding any new impls of TryFrom for slices or Vecs; these are an antipattern that encourages users of the API to choose their own serialization format rather than running through the protobuf changes. We don't need to add these to change the internal representation.
@hdevalence The |
|
Ah OK, in that case we should align it with the protos at some point. |
|
this was an exploratory draft, and was not intended or ready to be reviewed. i'm going to close this in favor of #4152. |
it seems we came to agreement on this point. i'll file an issue about it sometime this week. |
work in progress.
fixes #2304.