Basic serialization fixes #10

[theoreticalbts]

PR for issue #10

[goldibex]

The two big changes here aren't "bugs", they're design decisions. Why follow the awkward JSONified format of static_variants in Python (it's terribly un-Pythonic), and why not treat assets for serialization as objects with the properties amount, precision, and symbol?

[theoreticalbts]

Why follow the awkward JSONified format of static_variants in Python

Think about a simple use case:

• (1) Generate a transaction
• (2) Serialize it locally
• (3) Sign it
• (4) Submit it to steemd

If the Python serializer uses a different JSON format than steemd does, in Step (2) the JSON form of any static_variant in the transaction will need to be the "Python way," and in Step (4) any static_variant in the transaction will need to be the "C++ way". Meaning that in between Step (3) and Step (4) we'll have to insert a translation function to convert between the two formats, and user code will need to invoke this translation function.

It seems messy and inconvenient compared to having a single JSON format used by both Python and C++.

Perhaps you intend to suggest that the C++ JSON format be changed as well? That's an interesting idea, and from a release management perspective now would be an ideal time for such a change, since we're breaking compat anyway with appbase. But it seems like a good deal of work (across all other client libs as well), and anyway that idea is better discussed in the steemd repo, not here.

For now, let's keep it in sync with what the C++ code does currently, we can always revert or rewrite to work differently.

[theoreticalbts]

why not treat assets for serialization as objects with the properties amount, precision, and symbol

This is really the same problem as above: If the JSON must be compatible steemd, we can't leverage the object representation. We need this code to process it as a string, because that's what steemd does.

What the C++ backend produces will soon change, but the new spec was not 100% clear from that ticket and the code does not pass CI yet. So I made the judgment call to make simple_steem_client compatible with what the C++ backend currently does, so we're not blocked by the ongoing work in that issue.

If this is merged, we should make an issue to change this later.

[theoreticalbts]

why create our own ArgumentError rather than just using Python's? Is it un-Pythonic to use the built-in exceptions?

Here's why:

$ python2
Python 2.7.12 (default, Nov 20 2017, 18:23:56) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> ArgumentError
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'ArgumentError' is not defined
>>> 
$ python3
Python 3.5.2 (default, Nov 23 2017, 16:37:01) 
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> ArgumentError
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'ArgumentError' is not defined
>>> 

It is not defined in the default namespace on either of the system Python versions on Ubuntu 16.04 LTS. Perhaps it is defined somewhere in the standard library (a quick Google suggests perhaps argparse module?) but throwing argparse.ArgumentError seems like it should only be done when processing command-line arguments.

Defining the exception class wasn't a deep architectural decision, it was a quick two-liner to get the code working.

[theoreticalbts]

why doesn't [signed_transaction] embed the ordinary transaction serializer?

It should, but I wasn't immediately sure how to do it. I thought about it for a bit, and I think I figured out how.

[theoreticalbts]

Hopefully all of the above is addressed by these changes.

[goldibex]

Please continue the discussion of comments on the comment thread; don't come back out to here, it disorganizes everything. And thanks, you were very thorough.

If the Python serializer uses a different JSON format than steemd does

The input to the serializer isn't JSON, it's Python objects. Those may just happen to be dicts or other objects that were obtained through deserialization of JSON, but it isn't guaranteed.

If you want me to rewrite the serializer to convert JSON bytes into fc bytes I can do that, but it will take quite a lot of time.

If the line of argument here is that the shape of the input objects to the serializer ought to be the same shape as what Python produces as the output of a JSON deserializer operating over data received from steemd, I would only accept this as a compatibility hack and I would want the serializer to operate over both types. I would also want complete documentation on how steemd's JSON serializer operates; it appears to be bespoke.

The same argument applies to assets. If we have to work a little harder in the library so the user isn't bothered with the particulars of steemd's highly peculiar JSON output, we will work a little harder.

[theoreticalbts]

If the line of argument here is that the shape of the input objects to the serializer ought to be the same shape as what Python produces as the output of a JSON deserializer operating over data received from steemd, I would only accept this as a compatibility hack and I would want the serializer to operate over both types

OK, we need to specify exactly what the input to the serializer is. This isn't really documented anywhere. My first attempt to use the serializer was literally this:

tx = {
   "ref_block_num":0x1234,
   "ref_block_prefix":0x56789abc,
   "expiration":"2018-01-03T12:00:00",
   "operations":[
      ["transfer",
         {"from":"alice",
          "to":"bob",
          "amount":"7.625 STEEM",
          "memo":"transfer some tokens"
         }]],
   "extensions":[],
   "signatures":[]
}

ser = Serializer()
ser.signed_transaction(tx)
data = ser.flush()

It didn't work. So I assumed that meant the serializer was broken, since it seems quite obvious to me that this code should work, given that tx is correctly formatted according to the rules of steemd. Technically tx isn't JSON, it's a Python object which may be represented as JSON, as it is only constructed from str | int | float | bool literals and list | dict composed of them. Let's call such an object a "JSON-shaped object."

Now that we've got the naming issues out of the way, the design principle I want to enforce is that if a serializer accepts JSON-shaped objects, then the JSON-shaped objects accepted by the serializer should be the same as the JSON-shaped objects produced / accepted by steemd.

AFAIK steem-python trivially follows this principle, as it doesn't accept JSON-shaped objects; for example it has a class Transaction and all transactions given to the serializer must be instances of this class. I call the steem-python approach "type peering", because the "Transactionclass may be regarded as the "peer" of the C++transaction` struct.

Now that we have the additional requirement of steem-python compatibility, we need to implement type peering. Since the existing JSON-shaped layer is a useful tool and it mostly works as-is, I would suggest creating a type peering layer as different code on top of the JSON-shaped layer. Since all of this kind of code will hopefully be ripped out and replaced with auto-generated code in the future, I would suggest holding off on manually writing any type peering code.

[theoreticalbts]

Please continue the discussion of comments on the comment thread; don't come back out to here, it disorganizes everything.

I've had bad experiences with Github's in-code commeting. These comments tend to get lost if you force-push, which I frequently do on feature branches.

I would also want complete documentation on how steemd's JSON serializer operates; it appears to be bespoke.

Yes, by all means, let's document it!

[theoreticalbts]

While the above discussion is interesting and important, the more immediate concern is, what still remains to be done for this PR to be mergeable?

[goldibex]

approved. sigh.