Conversation
|
I need to do another round of self-review before marking this ready for review |
| pub inner: FixedBytes<SIGNATURE_SIZE>, | ||
| pub inner: LeanSigSignature, |
There was a problem hiding this comment.
Not sure if this is the right way to do it, but since FixedBytes is how Ream stores it internally and needs conversion to LeanSigSignature to do any operation anyway, so I'm thinking we could just remove this interim FixedBytes and use LeanSigSignature directly
There was a problem hiding this comment.
Will LeanSigSignature encode to FixedBytes though is the question? I will read your code, but that would be the concern
There was a problem hiding this comment.
In case it's useful this is what I recall. I think FixedBytes was introduced to the spec in devnet0 just because leanSig wasn't stable yet so we just used FixedBytes<SIGNATURE_SIZE> as the mock Signature type in the specs.
And then in devnet1 we started with Bytes3116 in the specs even with the actual signatures.
But towards end of devnet1, the class Signature(Bytes3116) was removed from the specs by leanEthereum/leanSpec#210 and the canonical Signature type becomes this class Signature(Container).
So since then, leanSig and leanSpec uses the Signature container type that's variable length, and the fixed bytes is no longer used. And so that's why I'm thinking we could just use LeanSigSignature directly without conversion to fixed bytes. But I dunno maybe I'm missing something...
There was a problem hiding this comment.
Yeah I did some regression testing and this looks fine 👍
…ead of fixed bytes
1d195d9 to
9e95a6d
Compare
| /// Creates a passthrough JSON wrapper that deserializes directly to the inner type. | ||
| macro_rules! passthrough_conversion { | ||
| ($name:ident, $inner:ty) => { | ||
| #[derive(Debug, Deserialize, Clone)] | ||
| #[serde(transparent)] | ||
| pub struct $name(pub $inner); | ||
|
|
||
| impl TryFrom<&$name> for $inner { | ||
| type Error = anyhow::Error; | ||
| fn try_from(value: &$name) -> anyhow::Result<Self> { | ||
| Ok(value.0.clone()) | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| /// Creates a TryFrom impl where all fields are copied directly. | ||
| macro_rules! simple_conversion { | ||
| ($json:ident => $target:ty { $($field:ident),+ }) => { | ||
| impl TryFrom<&$json> for $target { | ||
| type Error = anyhow::Error; | ||
| fn try_from(value: &$json) -> anyhow::Result<Self> { | ||
| Ok(Self { | ||
| $($field: value.$field),+ | ||
| }) | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| /// Creates a TryFrom impl where all fields are converted via try_into(). | ||
| macro_rules! nested_conversion { | ||
| ($json:ident => $target:ty { $($field:ident),+ }) => { | ||
| impl TryFrom<&$json> for $target { | ||
| type Error = anyhow::Error; | ||
| fn try_from(value: &$json) -> anyhow::Result<Self> { | ||
| Ok(Self { | ||
| $($field: (&value.$field).try_into()?),+ | ||
| }) | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| /// Creates a TryFrom impl with custom conversion expression per field. | ||
| macro_rules! custom_conversion { | ||
| ($json:ident as $val:ident => $target:ty { $($field:ident: $conv:expr),+ $(,)? }) => { | ||
| impl TryFrom<&$json> for $target { | ||
| type Error = anyhow::Error; | ||
| #[allow(clippy::redundant_closure_call)] | ||
| fn try_from($val: &$json) -> anyhow::Result<Self> { | ||
| Ok(Self { | ||
| $($field: $conv),+ | ||
| }) | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
wouldn't the code just be easier to read without macro's? this kind of feels like we are abusing macros here without much benefits.
| /// Create a mock signature for testing purposes. | ||
| /// | ||
| /// Note: This generates a real signature which is expensive. Prefer `blank()` when | ||
| /// you just need a placeholder signature. | ||
| pub fn mock() -> Self { | ||
| use rand::rng; |
There was a problem hiding this comment.
can you just remove this function as it isn't used
| /// Create a blank/placeholder signature. | ||
| /// | ||
| /// This decodes from minimal valid SSZ bytes, avoiding expensive key generation. | ||
| /// Only use in contexts where the signature won't be validated. | ||
| pub fn blank() -> Self { | ||
| Self::new(Default::default()) | ||
| // 40 bytes: offset_path(4) + rho(28 zeros) + offset_hashes(4) + path(4) | ||
| const BYTES: [u8; 40] = [ | ||
| 36, 0, 0, 0, // offset_path = 36 | ||
| 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // rho (28 zeros) | ||
| 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // | ||
| 40, 0, 0, 0, // offset_hashes = 40 | ||
| 4, 0, 0, 0, // path: empty HashTreeOpening | ||
| ]; | ||
| Self::from_ssz_bytes(&BYTES).expect("blank signature bytes are valid") |
There was a problem hiding this comment.
is this some universial constant? how did you derive it?
| /// Wrapper around leansig's signature type. | ||
| /// Uses leansig's built-in SSZ encoding for interoperability with other clients. | ||
| #[derive(Clone, Serialize, Deserialize)] | ||
| pub struct Signature { | ||
| pub inner: FixedBytes<SIGNATURE_SIZE>, | ||
| pub inner: LeanSigSignature, | ||
| } | ||
|
|
||
| impl std::fmt::Debug for Signature { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("Signature") | ||
| .field("inner", &"<LeanSigSignature>") | ||
| .finish() | ||
| } | ||
| } | ||
|
|
||
| impl PartialEq for Signature { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| // Compare by SSZ encoding since LeanSigSignature doesn't implement PartialEq | ||
| self.inner.as_ssz_bytes() == other.inner.as_ssz_bytes() | ||
| } | ||
| } | ||
|
|
||
| impl Eq for Signature {} | ||
|
|
||
| impl Encode for Signature { | ||
| fn is_ssz_fixed_len() -> bool { | ||
| <LeanSigSignature as Encode>::is_ssz_fixed_len() | ||
| } | ||
|
|
||
| fn ssz_bytes_len(&self) -> usize { | ||
| self.inner.ssz_bytes_len() | ||
| } | ||
|
|
||
| fn ssz_append(&self, buf: &mut Vec<u8>) { | ||
| self.inner.ssz_append(buf) | ||
| } | ||
| } | ||
|
|
||
| impl From<&[u8]> for Signature { | ||
| fn from(value: &[u8]) -> Self { | ||
| Self { | ||
| inner: FixedBytes::from_slice(value), | ||
| } | ||
| impl Decode for Signature { | ||
| fn is_ssz_fixed_len() -> bool { | ||
| <LeanSigSignature as Decode>::is_ssz_fixed_len() | ||
| } | ||
|
|
||
| fn from_ssz_bytes(bytes: &[u8]) -> Result<Self, DecodeError> { | ||
| Ok(Self { | ||
| inner: LeanSigSignature::from_ssz_bytes(bytes)?, | ||
| }) |
There was a problem hiding this comment.
| /// Wrapper around leansig's signature type. | |
| /// Uses leansig's built-in SSZ encoding for interoperability with other clients. | |
| #[derive(Clone, Serialize, Deserialize)] | |
| pub struct Signature { | |
| pub inner: FixedBytes<SIGNATURE_SIZE>, | |
| pub inner: LeanSigSignature, | |
| } | |
| impl std::fmt::Debug for Signature { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| f.debug_struct("Signature") | |
| .field("inner", &"<LeanSigSignature>") | |
| .finish() | |
| } | |
| } | |
| impl PartialEq for Signature { | |
| fn eq(&self, other: &Self) -> bool { | |
| // Compare by SSZ encoding since LeanSigSignature doesn't implement PartialEq | |
| self.inner.as_ssz_bytes() == other.inner.as_ssz_bytes() | |
| } | |
| } | |
| impl Eq for Signature {} | |
| impl Encode for Signature { | |
| fn is_ssz_fixed_len() -> bool { | |
| <LeanSigSignature as Encode>::is_ssz_fixed_len() | |
| } | |
| fn ssz_bytes_len(&self) -> usize { | |
| self.inner.ssz_bytes_len() | |
| } | |
| fn ssz_append(&self, buf: &mut Vec<u8>) { | |
| self.inner.ssz_append(buf) | |
| } | |
| } | |
| impl From<&[u8]> for Signature { | |
| fn from(value: &[u8]) -> Self { | |
| Self { | |
| inner: FixedBytes::from_slice(value), | |
| } | |
| impl Decode for Signature { | |
| fn is_ssz_fixed_len() -> bool { | |
| <LeanSigSignature as Decode>::is_ssz_fixed_len() | |
| } | |
| fn from_ssz_bytes(bytes: &[u8]) -> Result<Self, DecodeError> { | |
| Ok(Self { | |
| inner: LeanSigSignature::from_ssz_bytes(bytes)?, | |
| }) | |
| /// Wrapper around leansig's signature type. | |
| /// Uses leansig's built-in SSZ encoding for interoperability with other clients. | |
| #[derive(Clone, Serialize, Deserialize, Encode, Decode, PartialEq, Eq)] | |
| #[ssz(struct_behaviour = "transparent")] | |
| pub struct Signature { | |
| pub inner: LeanSigSignature, | |
| } | |
| impl std::fmt::Debug for Signature { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| f.debug_struct("Signature") | |
| .field("inner", &"<LeanSigSignature>") | |
| .finish() | |
| } | |
| } | |
We don't need to implement Encode, Decode, PartialEq ourselves, do we even need to implement Debug?
| pub inner: FixedBytes<SIGNATURE_SIZE>, | ||
| pub inner: LeanSigSignature, |
There was a problem hiding this comment.
Yeah I did some regression testing and this looks fine 👍
What was wrong?
Implement leanEthereum/leanSpec#380
TreeHashderive fromSignedAttestationbecause it's not used and it's in turn requiringSignatureto implementTreeHashas well which requires custom logic but also not used.Signature::innerfromFixedBytes<SIGNATURE_SIZE>toLeanSigSignatureso that the finalSignaturescheme type is used directly without needingFixedBytesin-betweenpassthrough_conversion,simple_conversion,nested_conversion,custom_conversionto handle the json camelCase conversion to Ream types.To-Do