Types for positive i64

[greg-szabo]

The problem with using int64 (i64) in protobuf:

most of the values where we use i64, can't be negative. (Vote.Height, etc) We can convert them to u64 with a check when they come in from the wire and carry on with life...
.
..until, we need to convert things back to protobuf on the wire.

Currently, we expect our internal structures to always be valid so we can use From/Into for converting domain types onto the wire. But u64->i64 can fail when the value is above the maximum of i64. The current internal type would be something like "u63" that can be directly converted to i64.

One option is exactly that: implement a "u63" type that has checks and conversion to i64 and use it internally for types that don't have their own domaintype.

Another option is to loosen our requirement for internal types to be correct, but I feel that's a very good restriction to have.

Any other ideas?

[tarcieri]

Any other ideas?

Fallible serialization

(Edit: whoops, meant to hit comment, not close, haven't had coffee yet 😅 )

[greg-szabo]

No problem :D

That's definitely always an option. I really like the idea of "our internal structures are always valid" because it expects all structures to have a TryFrom<wiretype> for struct coming in from the wire and a From<struct> for wiretype going to the wire. This is easy to achieve with most of our structs and easy to remember.
The only exception is u64 which is not "ours" so it's harder to put limits on it. It breaks the expected pattern of being able to use From/Into when encoding to the wire and it's especially stressful when it happens within a From method of another type. The other type should be infallible, but because of a fallible serialization for u64, now that other type has to be fallible too.

The example I can bring is Vote.Height. Vote currently has an infallible conversion to encode it to the wire. It's awesome that we can trust our internal structure. Because of Height being u64 (and i64 on the wire), now either I panic if I have an invalid height (argh), or I make Vote fallible too. (There goes the nice neighbourhood.)

[tarcieri]

The example I can bring is Vote.Height. Vote currently has an infallible conversion to encode it to the wire. It's awesome that we can trust our internal structure. Because of Height being u64 (and i64 on the wire), now either I panic if I have an invalid height (argh), or I make Vote fallible too. (There goes the nice neighbourhood.)

There's a block::Height type which could enforce some kind of maximum:

https://github.com/informalsystems/tendermint-rs/blob/master/tendermint/src/block/height.rs

Or at least, it could've until dda7afc, which made the inner u64 public.

[greg-szabo]

Yes, apologies, my example was not good enough. Fortunately, Vote.Height is our own type, so I can just implement these limitations there and get on with life. Vote.Round is the better example, which is still an integer.

But, this also gives me an idea: I can just implement a Round struct with these restrictions and again, get on with life.

Would that be acceptable? I'm a bit worried about exploding the number of structs, but maybe I'm just overthinking it.

Edit: I'm working on removing that public tag from Height.

[romac]

I personally do not mind having many of these simple wrapper structs. That's a common pattern in other languages, eg. Haskell's "newtypes" or Scala's "value classes").

I actually like these wrapper structs more than just a I63 type because:

1. they provide more semantic information
2. they help catch bugs such as when one supplies a I63 round for a I63 height