Json.Serialize doesn't support Buffer type

[roman-khimov]

Describe the bug
It looks like Json.Serialize syscall added with #785 and refined by #1353 doesn't support Buffer item type introduced with neo-project/neo-vm#272. It probably should've been added with #1374, but either it was missed or not added intentionally.

Expected behavior
I think Buffer should be OK to serialize, it's almost like a ByteString and it's going to be used a lot, so chances are someone will try to serialize it.

[shargon]

I will fix it

[erikzhang]

If you serialize a Buffer to JSON, when you deserialize it, you will get a ByteString instead of a Buffer. This is weird. Do you think this is acceptable?

[shargon]

I think that it should be serializable as base64 string, and don't be able to deserialize again. What do you think?

[roman-khimov]

If you serialize a Buffer to JSON, when you deserialize it, you will get a ByteString instead of a Buffer. This is weird.

If you're to serialize an Integer bigger than JNumber.MAX_SAFE_INTEGER you'd get a string that then will be deserialized as a string, so it's no weirder than that. Same thing with Map keys that get converted to strings no matter what their original PrimitiveType is.

So we either need to keep type information when serializing or live with the fact that it's lossy and there is no 1:1 correspondence between serialized and deserialized StackItem. And that choice depends on expected usage pattern, if the main thing that we care about is deserializing arbitrary JSONs then we're fine as we are now, if we want to use this API to precisely transfer StackItems between contracts/oracles/whatever, then this JsonSerializer should be more similar to BinarySerializer.

[roman-khimov]

If you're to serialize an Integer bigger than JNumber.MAX_SAFE_INTEGER you'd get a string that then will be deserialized as a string

Sorry, that one probably is not true since #1353, we have both Serialize and SerializeToByteArray in the code now and it's a bit confusing (and Serialize doesn't seem to be used now). Still, the main choice is the same.

[erikzhang]

if we want to use this API to precisely transfer StackItems between contracts/oracles/whatever

I think BinarySerializer should be used in this scenario.

[roman-khimov]

I think BinarySerializer should be used in this scenario.

And I would agree with that which leaves us with the question of whether we want to have Json.Serialize API as a free (lossy) addition to Json.Deserialize or just remove it as potentially dangerous (BTW, even with SerializeToByteArray implementation there is a type loss for Struct, it gets serialized into JSON array and then deserialized into Array). If we're to keep it then serializing Buffer makes sense as well as restoring a kludge for Integers bigger than MAX_SAFE_INTEGER.

I think we can keep it, maybe it'll still be useful in some scenarios even with these limitations, but of course it should be carefully documented to set the expectations of its user (and binary serialization/deserialization should be recommended for 1:1 transfers of StackItems).

[roman-khimov]

Seems like this one was fixed in #1715.