Adding symmetrical serialization to eds, row and sample
#495
Conversation
|
Tagging @zvolin for visibility |
| type Error = Error; | ||
|
|
||
| fn try_from(raw: RawExtendedDataSquare) -> std::result::Result<Self, Self::Error> { | ||
| ExtendedDataSquare::from_raw(raw, AppVersion::V2) |
There was a problem hiding this comment.
I don't think we can assume any particular AppVersion here, and this is likely the reason why there is no Deserialize derived here nor the try_from. I don't have any idea if we could do better there than just require using ExtendedDataSquare::from_raw
There was a problem hiding this comment.
In the rpc we work this around by requiring whole ExtendedHeader instead of just height when requesting EDS (and other share related stuff). We made an exception for the RowNamespaceData as requesting it for parity shares through celestia-node rpc is not supported
There was a problem hiding this comment.
After looking through the PR, I'm not sure if there is a way to support this correctly. It's a problem we're facing porting celestia to rust that the structures here aren't self contained and often require additional information to make sure everything is sound. See celestiaorg/celestia-node#3977 for EDS and celestiaorg/CIPs#128 for shwap types.
I'm happy to revisit this if you find a way to make it correct, but I'm afraid it's not possible with current state as types are not self contained. With current way celestia operates, the only correct (and binary) encoding of those is through .encode and .decode, passing extra infos through corresponding Id types
| let mut shares = Vec::with_capacity(raw.shares_half.len()); | ||
| for shr in raw.shares_half { | ||
| shares.push(Share::try_from(shr)?); | ||
| } |
There was a problem hiding this comment.
this isn't correct and again I'm not sure if we can do better than Row::from_raw. Row is a structure which holds the whole row from EDS, but in it's serialized form, it drops a half which needs to be reconstructed using leopard RS. However with just a plain shares_half, it is not possible to know which side needs to be reconstructed.
| type Error = Error; | ||
|
|
||
| fn try_from(raw: RawSample) -> std::result::Result<Self, Self::Error> { | ||
| let share = raw.share.ok_or(Error::MissingShares)?.try_into()?; |
There was a problem hiding this comment.
this won't work for parity shares, and there is again not much information to judge if share should be treated as parity or not
Ok, I see the issue with hard coding particular version of app into the deserialization. But what can be workaround here? We rely on support of serde to serialize/deserialize those things in binary format. If I guess we can use |
There is no symmetry in case of EDS because If you need serde, I think you could wrap them in a struct containing also additional information required to make the deserialization correct |
|
We don't like this too really... and we would love to have all the types self contained. It's mostly an idiomacy differences between go and rust, where in Go they happily serialize / deserialize everything and validate constrains where needed, and in Rust the idiomatic way is to not allow constructing invalid state at all |
Good point about I will try to go that route and encapsulate all needed information in custom deserialization for types that are used in our adapter |
Follow up of #493