Serialize Uint64 to string in JSON

[mslipper]

Closes #1924.

[anorth]

I think the whole point of the Uint64 type is to achieve JSON encoding, so without that we might be better off nuking it.

I am 100% in favour, but I think there might be history here (I'm aware of Javascript's issues with large integers, but this isn't Javascript and most sophisticated JSON-decoders support custom deserialisation). @ZenGround0 can I ask you to shepherd this PR?

@phritz you added this in #598, could you please shed light on the history?

[phritz]

the point of Uint64 was to have a custom type that we could encode efficiently using leb128, as opposed to say a fixed size integer encoding. however that whole exercise turns out to have been a waste of time because i recently learned we are using cbor's native variable length encoding filecoin-project/specs#131 (comment). i cannot think of a good reason to keep Uint64 off the top of my head except for custom json encoding and maybe something to do with the go-ipld-cbor library which uses a global (!) type registry eg https://github.com/filecoin-project/go-filecoin/blob/e1697c1c33b9024f042dc546c715cea9713337d3/types/atto_fil.go#L19. if you have a type you want encoded two different ways you have to create a type alias for it, so it could be the case that there is a custom registry or default behavior for uint64 that we might want to circumvent for some reason. @warpfork has canonical bits for how refmt works, but will be lacking any context from go-filecoin.

the fact that this change doesn't break any schema tests https://github.com/filecoin-project/go-filecoin/search?q=requireschemaconformance&unscoped_q=requireschemaconformance i guess is not surprising because the field still has string type. but we definitely want a test that verifies that this change has the behavior intended. i think we need a test in commands that asks for json output and verifies that the height for example we get out is an integer string, as opposed to an base64 encoding of leb128-encoded bytes. i seem to want to recall some hidden gotcha here, but can't off the top of my head.

would any other context be helpful?

[ZenGround0]

@anorth @phritz are we ok writing a followup issue to decide the fate of uint64 and merging this?

[anorth]

I think we need the tests @phritz requested, but otherwise yes I'm very happy for a follow-up to address Uint64.

[mslipper]

Cool, I'll add the tests tonight.

[warpfork]

which uses a global (!) type registry

fwiw, that's somewhat in "known issue" town and will be fixed again when we get to go-ipld-prime, which is intended to supplant the current go-ipld-cbor and address design issues like this. (It's a little odd that go-ipld-cbor ended up with a global type registry, since refmt was explicitly designed to avoid this, but, that ship sailed a while ago now.)

---

I'll refrain from any other strong opinions about the semantics being discussed here, since I'm not super synced up with the overall context, but as a general note, numerics right around the max bit size limits tend to terrify me. I always wonder to myself -- if the 64th bit really mattered, is there some reason that one was more important than the 63rd, and is there a stable reason the 65th never will? In other words, is it certain this shouldn't bite the bullet and do a BigInt? If it's literally a 64-bit bitfield for some algorithmic reason, that's fine, but any other application is quite scary; and if it's a bitfield rather than a number, don't bytes as a type make more sense anyway? /2c.

[travisperson]

I think the whole point of the Uint64 type is to achieve JSON encoding, so without that we might be better off nuking it.

i cannot think of a good reason to keep Uint64 off the top of my head except for custom json encoding

I think that is a big reason for keeping the custom type around. We shouldn't encode it to a JSON number because JSON doesn't define the semantics of what a number is besides the sequence of valid characters. This makes it much harder to work with in different languages or with different JSON libraries.

We could use JSON numbers, but make it much harder for others to support as well. Regardless we would need the custom type to handle the encoding if we wanted to have more than 53 bits (or 52 I can never remember) of precision as golang IEEE-754 for float64 (which is the internal type used when handling JSON numbers).

It seems much easier to use a string, and then allow application developers to custom convert to a numerical type they want after whatever library / built-in parses the JSON tokens.

[mishmosh]

The uint64 discussion is parked in #2267.

Tagging @phritz for maintainer review now.

[phritz]

In other words, is it certain this shouldn't bite the bullet and do a BigInt?

fair point. i think native types are just easier to work with, so we prefer them where they are going to last a long time, but i can see both sides.

[anorth]

I have reverted this because the test failed on master. I'm not quite sure what went wrong, but please try again!

[mslipper]

tar: go/src/github.com/gogo/protobuf/proto/table_merge.go: Cannot write: Cannot allocate memory - seems transient, since this was part of the 'restore cache' step. Will rebase and rebuild.