nspcc-dev / neo-go

Go Node and SDK for the NEO blockchain
MIT License
119 stars 77 forks source link

Null handling in unwrap library #2795

Open roman-khimov opened 1 year ago

roman-khimov commented 1 year ago

Another issue I'm thinking about is that array of something can be naturally substituted by Null stackitem and our unwrappers won't accept it. Maybe it's worth to add Null support to unwrap package for arrays?

Originally posted by @AnnaShaleva in https://github.com/nspcc-dev/neo-go/pull/2792#pullrequestreview-1178887775

It's true that we can have Null in many cases. We can have nil in Go at the same time. Maybe it's OK to return it.

AnnaShaleva commented 7 months ago

Ping to this issue, because NNS wrapper (#3291) uses getRecord NNS method that returns Null stackitem in case of missing record of the specified type. And neither unwrap.UTF8String nor unwrap.Bytes can handle Null stackitem. As a result in the described case we've got an error from the NNS wrapper side on attempt to unwrap Null into Go's string while NNS itself doesn't return any error.

IMO, this behaviour of NNS wrapper is not correct and it will be fixed by this issue. We likely should return an empty string and no error form NNS wrapper in the described case. So something like the following will be useful, but we need to evaluate the usages of unwrap.Bytes and similar methods. Maybe we can directly modify unwrap.Bytes and etc. to properly handle Null.

+// Bytes expects correct execution (HALT state) with a single stack item
+// returned. A slice of bytes is extracted from this item and returned.
+func BytesOrNull(r *result.Invoke, err error) ([]byte, error) {
+       itm, err := Item(r, err)
+       if err != nil {
+               return nil, err
+       }
+       if itm.Equals(stackitem.Null{}) {
+               return nil, nil
+       }
+       return itm.TryBytes()
+}
+

+// UTF8String expects correct execution (HALT state) with a single stack item
+// returned. A string is extracted from this item and checked for UTF-8
+// correctness, valid strings are then returned.
+func UTF8StringOrNull(r *result.Invoke, err error) (string, error) {
+       b, err := BytesOrNull(r, err)
+       if err != nil {
+               return "", err
+       }
+       if b == nil {
+               return "", nil
+       }
+       if !utf8.Valid(b) {
+               return "", errors.New("not a UTF-8 string")
+       }
+       return string(b), nil
+}
+

@roman-khimov, what do you think?

@AliceInHunterland, this issue is related to error returned from recordData, err := nnsReader.GetRecord(domainName, recordType)

AnnaShaleva commented 7 months ago

I'd propose to include this issue in 0.106.0 milestone.

roman-khimov commented 7 months ago

It's pretty straightforward for unwrap.Bytes (or arrays), but UTF8String is different.

empty string and no error

The problem is, it's not nil. And you can't differentiate NULL and real empty string in this case.

Try to check for more examples of how these APIs are used currently.

AnnaShaleva commented 7 months ago

The problem is, it's not nil. And you can't differentiate NULL and real empty string in this case.

You're right. Then maybe we need to change the NNS wraper itself to return (*string, error)? It doesn't look good to me either, but returning an error also seems not very correct to me.

roman-khimov commented 7 months ago

It's one of:

None of them is perfect. But which one to choose depends on typical usage patterns.

cthulhu-rider commented 2 months ago

still gives me goosebumps, very waiting for the release!

didn't see this thread right away, so shared some my opinion in https://github.com/nspcc-dev/neofs-contract/issues/415