tendermint / go-amino

Protobuf3 with Interface support - Designed for blockchains (deterministic, upgradeable, fast, and compact)
Other
259 stars 78 forks source link

Disallow multidimensinal arrays #272

Closed liamsi closed 5 years ago

liamsi commented 5 years ago

Amino allows encoding of [][]T directly or as fields (where T can be different from []byte). This does not have a matching representation in protobuf, meaning that sth. like message MsgName { repeated repeated T = 1;} is not allowed in protobuf (for some message T { /* ...*/ }.

Not sure how much this is used in the SDK. Tendermint only uses [][]byte as far as I can see.

(note that [][]byte fields are still ok as it can be represented as repeated bytes is valid in protobuf, too)

alexanderbez commented 5 years ago

I don't think this is a problem for the SDK. We don't use [][]T in amino encodings from what I can see.

liamsi commented 5 years ago

I was under the same impression. But I was not 100% sure as one can't easy spot type aliases where this still might be used implicitly, e.g.

type SomeType struct {
  FooBars []Foo
}

// this can be in a different package:
type Foo []Bar

Amino certainly allows this as one can see in the tests, e.g.:

https://github.com/tendermint/go-amino/blob/716e3402f1779c3b724841e212eb5753d08cb945/binary_test.go#L16-L20

My suggestion here would be to throw an error in case amino detects this. This would ensure that users won't use this in the future, too.

alexanderbez commented 5 years ago

Ahhh yes! Good point. I'll have to double check that we're not doing this, but I doubt we are. And if we are, I'm sure we can resolve it.

liamsi commented 5 years ago

Thanks @alexanderbez!