proto3 compatibility: use uvarint encoding in time instead of fixed sized #224

liamsi · 2018-09-06T15:33:04Z

~~WIP: fuzzer found a crasher, still need to investigate if this is related!~~
The fuzzer just timed out, no real crashers.

resolves #208
resolves #223

- internal seconds and nanos were encoded fixed size which is different from proto's "well known type" TimeStamp

liamsi · 2018-09-08T13:06:34Z

This is actually a bit confusing:
proto3's Timestamp message looks like this:

message Timestamp {
  int64 seconds = 1; // this is proto3's int64
  int32 nanos = 2;
}

which translate into this go code:

type Timestamp struct {
	Seconds int64 `protobuf:"varint,1,opt,name=seconds,proto3" json:"seconds,omitempty"`
	Nanos   int32    `protobuf:"varint,2,opt,name=nanos,proto3" json:"nanos,omitempty"`
}

In protobuf int32 and int64 are casted to uint64 and encoded as unsigned varint. In amino we encode golang's int32 as varint (not unsigned). We treat it as proto's sint32 which is encoded differently (see scalar types). This isn't a problem but probably needs to be documented somewhere.

I've updated amino's time encoding to match with proto's behaviour (will push shortly).

- add tests against "google/protobuf/timestamp.proto and clean up tests slightly - add validation of seconds while encoding - add error type InvalidTimeErr - remove unnecessary var - fix typo - add changelog entry

- ./codec.go:257: PrintTypes passes lock by value: amino.Codec contains sync.RWMutex - remove some unused (unexported) methods

ValarDragon

Looks good to me, just a few minor comments!

ValarDragon · 2018-10-13T01:42:09Z

encoder.go

+	// seconds of 01-01-0001
+	minSeconds = -62135596800
+	// seconds of 10000-01-01
+	maxSeconds = 253402300800


Is this the standard upperbound? Just seems kinda weird since its not near a power of two. Doesn't block the PR though

This constant is straight from the code that converts between time.Time and the wellknown type Timestamp: https://github.com/golang/protobuf/blob/ddf22928ea3c56eb4292a0adbbf5001b1e8e7d0d/ptypes/timestamp.go#L48-L50

ValarDragon · 2018-10-13T01:42:33Z

decoder.go

@@ -261,15 +262,20 @@ func decodeNanos(bz *[]byte, n *int) (int32, error) {
 	if err != nil {
 		return 0, err
 	}
-	if fieldNum == 2 && typ == Typ3_4Byte {
+	if fieldNum == 2 && typ == Typ3_Varint {
 		slide(bz, n, _n)
 		_n = 0
 		// Actually read the Int32.


Outdated comment

Thanks, good catch! Removed.

ValarDragon · 2018-10-13T04:04:41Z

tests/proto3/proto3_compat_test.go

+	assert.Equal(t, p3.ProtoGotTime{T: nil}, pbRes)
+}
+
+func TestProto3CompatTimestampNow(t *testing.T) {


Can this be table driven?

That would be nice. But it's basically one test-case that documents what works how.

liamsi · 2018-10-14T17:07:54Z

Merged! Thanks for the review @ValarDragon

melekes · 2018-10-14T17:46:12Z

decoder.go

+		// the original signed value:
+		res := int64(sec)
+		if res < minSeconds || res >= maxSeconds {
+			return 0, n, InvalidTimeErr(fmt.Sprintf("seconds have to be > %d and < %d, got: %d",


seconds have to be >= %d

melekes · 2018-10-14T17:48:27Z

encoder.go

+		if ns < 0 || ns > maxNanos {
+			// we could as well panic here:
+			// time.Time.Nanosecond() guarantees nanos to be in [0, 999,999,999]
+			return InvalidTimeErr(fmt.Sprintf("nanosecons have to be >= 0 and <= %v, got: %d",


nanoseconds

Thanks. I'll address this and above comment in #233

proto3 compatibility: use varint encoding in time instead of fixed sized

e236d80

- internal seconds and nanos were encoded fixed size which is different from proto's "well known type" TimeStamp

liamsi force-pushed the its_time_again branch from 8336134 to ca36768 Compare September 8, 2018 15:02

switch to UVarint encoding (EncodeUvarint) for seconds and nanos

dbdbbcd

- add tests against "google/protobuf/timestamp.proto and clean up tests slightly - add validation of seconds while encoding - add error type InvalidTimeErr - remove unnecessary var - fix typo - add changelog entry

liamsi force-pushed the its_time_again branch from ca36768 to dbdbbcd Compare September 8, 2018 15:03

go vet:

3af4656

- ./codec.go:257: PrintTypes passes lock by value: amino.Codec contains sync.RWMutex - remove some unused (unexported) methods

liamsi requested review from jaekwon, ebuchman and melekes September 8, 2018 15:08

liamsi changed the title ~~[WIP] proto3 compatibility: use varint encoding in time instead of fixed sized~~ proto3 compatibility: use varint encoding in time instead of fixed sized Sep 8, 2018

liamsi requested a review from ValarDragon September 8, 2018 15:09

liamsi changed the title ~~proto3 compatibility: use varint encoding in time instead of fixed sized~~ proto3 compatibility: use uvarint encoding in time instead of fixed sized Sep 8, 2018

liamsi force-pushed the its_time_again branch from b1924f9 to 9d7df11 Compare September 8, 2018 15:15

fix bound

e2d4648

liamsi force-pushed the its_time_again branch from 9d7df11 to e2d4648 Compare September 8, 2018 16:03

liamsi changed the title ~~proto3 compatibility: use uvarint encoding in time instead of fixed sized~~ [WIP] proto3 compatibility: use uvarint encoding in time instead of fixed sized Sep 8, 2018

liamsi changed the title ~~[WIP] proto3 compatibility: use uvarint encoding in time instead of fixed sized~~ proto3 compatibility: use uvarint encoding in time instead of fixed sized Sep 10, 2018

liamsi requested a review from rigelrozanski September 20, 2018 09:59

Merge branch 'develop' into its_time_again

afb27d1

ValarDragon approved these changes Oct 13, 2018

View reviewed changes

review comment: remove outdated comment

32474b6

liamsi merged commit 8171e3d into develop Oct 14, 2018

melekes reviewed Oct 14, 2018

View reviewed changes

liamsi added a commit that referenced this pull request Oct 15, 2018

Address review comments from #224

8a64121

liamsi deleted the its_time_again branch October 30, 2018 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proto3 compatibility: use uvarint encoding in time instead of fixed sized #224

proto3 compatibility: use uvarint encoding in time instead of fixed sized #224

liamsi commented Sep 6, 2018 •

edited

Loading

liamsi commented Sep 8, 2018 •

edited

Loading

ValarDragon left a comment

ValarDragon Oct 13, 2018

liamsi Oct 14, 2018

ValarDragon Oct 13, 2018

liamsi Oct 14, 2018

ValarDragon Oct 13, 2018

liamsi Oct 14, 2018

liamsi commented Oct 14, 2018

melekes Oct 14, 2018

melekes Oct 14, 2018

liamsi Oct 15, 2018

proto3 compatibility: use uvarint encoding in time instead of fixed sized #224

proto3 compatibility: use uvarint encoding in time instead of fixed sized #224

Conversation

liamsi commented Sep 6, 2018 • edited Loading

liamsi commented Sep 8, 2018 • edited Loading

ValarDragon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Oct 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Sep 6, 2018 •

edited

Loading

liamsi commented Sep 8, 2018 •

edited

Loading