Change Label to be a Union of strings and bytes #1580

arajasek · 2022-03-12T19:17:04Z

Implements FIP-0027. Will need a migration.

anorth

Thanks, very helpful.

anorth · 2022-03-13T21:27:22Z

actors/builtin/market/deal.go

+
+var EmptyDealLabel = DealLabel{}
+
+func NewDealLabelFromString(s string) (DealLabel, error) {


Nit: deal.NewLabelFromString, don't repeat the word "deal".

actors/builtin/market/deal.go

codecov-commenter · 2022-04-05T00:36:32Z

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 · 2022-04-05T18:02:25Z

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

rvagg

👌 very nice, I'm excited about this

arajasek

(Disclaimer: I'm not sure how much of this came from @ZenGround0, I may be disagreeing with past-Aayush)

I have some concerns about the variance in what can and cannot be constructed. For example:

You can unmarshal labels whose lengths are longer than DealMaxLabelSize. I'm not sure what the impact of this might be, but it feels undesirable -- we could mitigate by having the Unmarshal method use the same NewLabelFromString and NewLabelFromBytes constructors?
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?
Is the ToString / ToBytes method purely for testing? It feels a little weird to have them -- in my head the point of this struct is to say "your label can be 1 of 2 (potentially very different) types, and everything will work seamlessly". Converting between them is odd to me.

actors/builtin/market/deal.go

ZenGround0 · 2022-04-06T14:40:22Z

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

I would have done a few things a bit differently, but this looks correct.

Stebalien · 2022-04-06T17:52:10Z

actors/builtin/market/deal.go

+type DealLabel struct {
+	s  *string
+	bs *[]byte
+}


personal opinion: I'd write this as

type DealLabel struct { bs []byte notString bool }

Your version is more "go-like", but also requires extra allocations and may be a bit annoying to deal with...

Ironically I was following up on your comment here is this not the "put them behind pointers" approach?

I'm going to move it to the above approach

~~But I think we want isString to make the 0 value serialize to cbor text not cbor bytes~~ nvm notString is the way it is to make this nice

Ironically I was following up on your comment #1580 (comment) is this not the "put them behind pointers" approach?

Your version is the "put them behind pointers approach". My version here is the "enum thing" approach (which, after looking at this, seems simpler.

nvm notString is the way it is to make this nice

Welcome to go! zero values are over there 👉 .

Stebalien · 2022-04-06T17:54:32Z

actors/builtin/market/deal.go

+func (label DealLabel) IsEmpty() bool {
+	return label.s == nil && label.bs == nil
+}


Is this accurate? I'd assume that IsEmpty would mean "is empty". I.e., "" would also be empty.

Stebalien · 2022-04-06T18:03:24Z

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 · 2022-04-06T18:18:54Z

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.

Stebalien · 2022-04-06T20:18:22Z

actors/builtin/market/deal.go

+			return err
+		}
+
+		if _, err := w.Write((label.bs)[:]); err != nil {


Suggested change

if _, err := w.Write((label.bs)[:]); err != nil {

if _, err := w.Write(label.bs); err != nil {

Stebalien · 2022-04-06T20:19:13Z

actors/builtin/market/deal.go

+			return err
+		}
+	} else { // label.IsString()
+		if len(label.bs) > cbg.MaxLength {


Se use MaxByteArrayLen in both cases (on further inspection). I'd just lift this case to the top.

Stebalien · 2022-04-06T20:20:48Z

actors/builtin/market/deal.go

+	return bytes.Equal(l.bs, o.bs) && l.notString == o.notString
+}
+
+func (label *DealLabel) MarshalCBOR(w io.Writer) error {


I think we can vastly simplify this now:

If null, write empty string.

If too large, return an error.

Set a majorType variable depending on the type.

Write the value (no need for a conditional).

Stebalien · 2022-04-06T20:22:47Z

actors/builtin/market/deal.go

+	if maj == cbg.MajTextString {
+
+		if length > cbg.MaxLength {
+			return fmt.Errorf("label string was too long (%d), max allowed (%d)", length, cbg.MaxLength)
+		}
+
+		buf := make([]byte, length)
+		_, err = io.ReadAtLeast(br, buf, int(length))
+		if err != nil {
+			return err
+		}
+		s := string(buf)
+		if !utf8.ValidString(s) {
+			return fmt.Errorf("label string not valid utf8")
+		}
+		label.bs = buf
+		label.notString = false
+	} else if maj == cbg.MajByteString {
+
+		if length > cbg.ByteArrayMaxLen {
+			return fmt.Errorf("label bytes was too long (%d), max allowed (%d)", length, cbg.ByteArrayMaxLen)
+		}
+
+		bs := make([]uint8, length)
+		label.bs = bs
+		label.notString = true
+		if _, err := io.ReadFull(br, bs[:]); err != nil {
+			return err


We should be able to unify these codepaths.

Make sure we have either a text string or a byte string.

Set "not string" if it's a byte string.

Check the length.

Read the value.

If a string, check if it's utf8.

Stebalien

code nits but otherwise LGTM

actors/builtin/market/deal.go

Stebalien · 2022-04-06T20:51:36Z

actors/builtin/market/deal.go

+		if _, err := io.WriteString(w, string("")); err != nil {
+			return err
+		}
+		return nil


Can just return.

Stebalien · 2022-04-06T20:51:50Z

actors/builtin/market/deal.go

+		if _, err := w.Write(label.bs); err != nil {
+			return err
+		}
+	}
+
+	return nil


Can just return.

Stebalien · 2022-04-06T20:52:26Z

actors/builtin/market/deal.go

+	if !label.notString {
+		s := string(buf)
+		if !utf8.ValidString(s) {


Suggested change

if !label.notString {

s := string(buf)

if !utf8.ValidString(s) {

if !label.notString && !utf8.ValidString(string(buf)) {

arajasek changed the title ~~Market state: Change Label to be a Union of strings and bytes~~ DO NOT MERGE: Market state: Change Label to be a Union of strings and bytes Mar 12, 2022

arajasek mentioned this pull request Mar 12, 2022

FIP-0027: Use a Union type for the Label field filecoin-project/FIPs#318

Merged

anorth reviewed Mar 13, 2022

View reviewed changes

jennijuju mentioned this pull request Mar 14, 2022

prototype FIP 0027 filecoin-project/builtin-actors#107

Closed

Stebalien reviewed Mar 14, 2022

View reviewed changes

actors/builtin/market/deal.go Outdated Show resolved Hide resolved

Stebalien reviewed Mar 14, 2022

View reviewed changes

actors/builtin/market/deal.go Outdated Show resolved Hide resolved

Stebalien reviewed Mar 14, 2022

View reviewed changes

actors/builtin/market/deal.go Outdated Show resolved Hide resolved

Market state: Change Label to be a Union of strings and bytes

636a958

ZenGround0 force-pushed the asr/label branch from c755c04 to 636a958 Compare April 4, 2022 22:53

ZenGround0 changed the base branch from release/v7 to master April 4, 2022 22:55

ZenGround0 added 2 commits April 4, 2022 17:01

Post rebase cbor gen fixup

4568ac6

Clean up label serdeser

eafb6ed

Testing stubs

a00a367

ZenGround0 changed the title ~~DO NOT MERGE: Market state: Change Label to be a Union of strings and bytes~~ Change Label to be a Union of strings and bytes Apr 5, 2022

ZenGround0 added 4 commits April 5, 2022 10:47

CBOR Unmarshal from bytes

a4595c0

Finish label tests

91ed535

Cleanup

e60239d

Regen determinism

084fa4b

ZenGround0 marked this pull request as ready for review April 5, 2022 18:01

ZenGround0 requested a review from a team as a code owner April 5, 2022 18:01

ZenGround0 requested a review from Stebalien April 5, 2022 18:01

ZenGround0 added 3 commits April 5, 2022 12:03

make gen

e8608cf

Lint

580cef0

Fix test

73e61eb

ZenGround0 mentioned this pull request Apr 5, 2022

DealProposal's Label field serializes as non-UTF8 #1248

Open

rvagg approved these changes Apr 6, 2022

View reviewed changes

arajasek commented Apr 6, 2022

View reviewed changes

actors/builtin/market/deal.go Outdated Show resolved Hide resolved

Review Response

ef787d8

ZenGround0 force-pushed the asr/label branch from 546514c to ef787d8 Compare April 6, 2022 17:25

Stebalien approved these changes Apr 6, 2022

View reviewed changes

ZenGround0 added 2 commits April 6, 2022 14:04

Move to tag impl with no separate empty case

ec267e2

Cleanup

b793733

ZenGround0 requested a review from Stebalien April 6, 2022 20:09

Stebalien reviewed Apr 6, 2022

View reviewed changes

ZenGround0 added 2 commits April 6, 2022 14:47

Simplify (un)marshal code

c8a070c

Fix comment

f65d10f

Stebalien approved these changes Apr 6, 2022

View reviewed changes

Last round

66045fb

ZenGround0 merged commit cc74c2c into master Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Label to be a Union of strings and bytes #1580

Change Label to be a Union of strings and bytes #1580

arajasek commented Mar 12, 2022 •

edited

Loading

anorth left a comment

anorth Mar 13, 2022

codecov-commenter commented Apr 5, 2022 •

edited

Loading

ZenGround0 commented Apr 5, 2022

rvagg left a comment

arajasek left a comment

ZenGround0 commented Apr 6, 2022

Stebalien left a comment

Stebalien Apr 6, 2022

ZenGround0 Apr 6, 2022

ZenGround0 Apr 6, 2022

ZenGround0 Apr 6, 2022 •

edited

Loading

Stebalien Apr 6, 2022

Stebalien Apr 6, 2022

Stebalien commented Apr 6, 2022

arajasek commented Apr 6, 2022

Stebalien Apr 6, 2022

Stebalien Apr 6, 2022

Stebalien Apr 6, 2022

Stebalien Apr 6, 2022

Stebalien left a comment

Stebalien Apr 6, 2022

Stebalien Apr 6, 2022

Stebalien Apr 6, 2022


		var EmptyDealLabel = DealLabel{}

		func NewDealLabelFromString(s string) (DealLabel, error) {

	if _, err := w.Write((label.bs)[:]); err != nil {
	if _, err := w.Write(label.bs); err != nil {

Change Label to be a Union of strings and bytes #1580

Change Label to be a Union of strings and bytes #1580

Conversation

arajasek commented Mar 12, 2022 • edited Loading

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 5, 2022 • edited Loading

Codecov Report

ZenGround0 commented Apr 5, 2022

rvagg left a comment

Choose a reason for hiding this comment

arajasek left a comment

Choose a reason for hiding this comment

ZenGround0 commented Apr 6, 2022

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Apr 6, 2022

arajasek commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arajasek commented Mar 12, 2022 •

edited

Loading

codecov-commenter commented Apr 5, 2022 •

edited

Loading

ZenGround0 Apr 6, 2022 •

edited

Loading