Shred struct redesign
Closed this issue · 2 comments
Consider a new way to organize data for the Shred/*Shred/GenericShred paradigm, with this discussion in mind: #194 (comment)
@0xNineteen said:
out of scope comment:
fields: Fields, const Fields = GenericShred(DataShredHeader, data_shred);
looking back at some prev code - the GenericShred approach seems a bit confusing - it would probs be a lot easier to understand the way agave did it here by putting the common_fields onto the struct itself
the fromPayload method can probably be done with a custom bincode deserialization method
data_shred -> DATA_SHRED_CONSTANTS would be more descriptive, also since its a global constant
@dnut said:
looking back at some prev code - the GenericShred approach seems a bit confusing - it would probs be a lot easier to understand the way agave did it here by putting the common_fields onto the struct itself
I don't like that there are so many layers either. I've been trying to come up with a better alternative but haven't come up with anything yet. In my opinion, the agave approach is even more confusing.
The issue with putting the fields directly in CodingShred and DataShred is that you need to copy identical duplicates for all the generic methods into both of those structs.
In agave, they accomplished this with macros, like impl_shred_common and impl_merkle_shred. Personally I find it easier to make sense of a nested struct rather than wading through macro definitions.
The other approach is just having duplicate code. One disadvantage to this is that you lose the idea that these methods apply to all shreds. Another thing to keep in mind is that we are going to need a lot more of them. The insert shreds pr doubles the number of generic methods on shreds. Maintaining duplicates of each is extra work and will almost inevitably lead to accidental divergence at some point.
I can spend some time looking for an approach that flattens some nesting without resorting to confusing metaprogramming or excessive code duplication. Some repetition may be inevitable, but I'd like to avoid duplicating every single detail. I think it will make more sense to do this on top of the changes in the insert shreds pr, first of all because GenericShred isn't introduced by the current pr, and also the insert shreds pr has more concrete methods that are very relevant to this. I'd like to ensure any change to the structure here is compatible with all of those methods, and avoid too many complicated merges.
@dnut - can we implement an improvement for this before the blogpost - would be great to be able to show the struct in a code snippet and be very easy to understand - im thinking duplicate + comments to specify the common fields will be the best trade-off imo
const DataShred = struct {
data_field1: ...
data_field2: ...
/// COMMON SHRED FIELDS
common_field1: ...
common_field2: ...
}
Duplicating every field and every method would make the situation worse in my opinion. Duplicating only the fields but sharing common functions could be good though. I'll see what I can do