Change Label to be a Union of strings and bytes

[arajasek]

Implements FIP-0027. Will need a migration.

[codecov-commenter]

Codecov Report

Merging #1580 (f65d10f) into master (5f5089f) will increase coverage by 0.0%. The diff coverage is 69.2%.

@@          Coverage Diff           @@
##           master   #1580   +/-   ##
======================================
  Coverage    69.0%   69.0%           
======================================
  Files          73      74    +1     
  Lines        8798    8887   +89     
======================================
+ Hits         6077    6140   +63     
- Misses       1859    1873   +14     
- Partials      862     874   +12     

[ZenGround0]

Can't tag you for review since you opened this @arajasek but please review this too

[ZenGround0]

Yes bullets 1 and 2 were inherited from your draft of the PR.

For the first point, this is just the current state of the chain, after unmarshalling proposals we then further check we're not exceeding max label length. Seems like the right breakdown. Let cbor worry about cbor limits let application code worry about application limits.

Am I misunderstanding, or would NewLabelFromString("").IsEmpty() return false? Is that intentional? We could mitigate by making IsEmpty() return len(label.s) == 0 && len(label.bs) == 0?

Yeah its intentional. Its a type thing not a value thing. Empty means contains no string or bytes and the empty string is a string. EmptyLabel serializes to a string but its not a string. This is all just shorthand to be a bit clearer than comparing both fields to nil in a few places. I'll add a comment but don't think this needs to change. Your suggestion is outdated with the pointer representation I changed to on steb's suggestion. b and s are pointers so never have length.

For the final point you added these when you made this PR and they turned out useful for testing. I made ToBytes convert a string on steb's suggestion. But I think your right that converting here doesn't make sense so I'll change that back to erroring if its not bytes.

[Stebalien]

Yeah its intentional. Its a type thing not a value thing. Empty means contains no string or bytes and the empty string is a string. EmptyLabel serializes to a string but its not a string. This is all just shorthand to be a bit clearer than comparing both fields to nil in a few places. I'll add a comment but don't think this needs to change. Your suggestion is outdated with the pointer representation I changed to on steb's suggestion. b and s are pointers so never have length.

We shouldn't distinguish between the two kinds of empty. That is, DealLabel{} and DealLabel{s: ""} should be indistinguishable.

[arajasek]

Sorry, I'm still confused about the emptiness thing. Some qs:

• Does the canonical EmptyDealLabel change if it's serialized and deserialized? I think it might go from nil, nil to &"", nil, which seems odd if it's canonical?
• Can the IsEmpty() property change if a DealLabel is serialized and deserialized? I think it should be a requirement that none of the query-able properties change through the serialization-deserialization process, but I think that's not always the case.

I might be wrong about some of these claims.