eoscanada
Added support for decoding action results
Test fails however, one related to binary extensions not being last anymore. Maybe the other is related (or not?)
fixed the one about the binary extensions, seems like the abi serialization fails as well, probably because the action_results field has been added to the abi?
That's a high possibility indeed and probably exactly why it failed. And I think ABI are binary decode when read from console reader, so would need to be double-checked that reading ABI emitted by nodeos still works.
And would mean also that when the change get merged in, only probably 3.1 will be supported on release >= X. I'm not against by the way, handling those struct changes has always been a hard problem.
If it's used in the console reader though, think about backward compatibility and what happens with older version.
That's a high possibility indeed and probably exactly why it failed. And I think ABI are binary decode when read from console reader, so would need to be double-checked that reading ABI emitted by nodeos still works.
what's the best way to update the test cases? Do I just deploy the abi to a contract on a testnet that is already running 3.1 and then get the code from the setabi action and compare it to the binary encoded one from eos-go?
If it's used in the console reader though, think about backward compatibility and what happens with older version.
because we need this to read the dmlog from before the update to 3.1 I guess? I'll think about how to preserve backward compatibility here
what's the best way to update the test cases?
Like you said is a good way. Otherwise if you feel brave enough, you could find where exactly in the bytes sequence where to add 0x00 which would mean a vector of something that is empty, which is the case for ActionResult[]. Not attached to one of the other (although try to find a serialized ABI that exercices most of the fields defined in the struct).
because we need this to read the dmlog from before the update to 3.1 I guess
Yeah and that is mostly for you guys, I'm thinking for example of WAX for example which is not upgraded yet. What I started doing when I added initial support for 3.0 is to actually move the console reader required struct in dfuse-eosio directly and and different version for different supported version of the console reader.
- https://github.com/pinax-network/dfuse-eosio/tree/develop/codec/eosio/v2.0
- https://github.com/pinax-network/dfuse-eosio/tree/develop/codec/eosio/v2.1
You will see in those packages same struct with slight variations in the struct definitions. The hydrator is now used to pick which struct it should use for deserialization.
Some something we could always do, eos-go always follows latest of EOSIO, backward compatible struct required for console reading are duplicated and kept in dfuse-eosio.
Like you said is a good way.
I've deployed a small contract which we used to test the decoding of action_results to Kylin. I used the abi hex code from the setabi transaction and then got the deployed api from /v1/chain/get_abi and used that in the abi_test.go file. For some reason the hex code from setabi has one 0x00 byte somewhere in the middle which the marshalled binary code from eos-go doesn't have.
Not sure if this indicates that the setabi has one more empty vector flag set than eos-go? However, looking at the abi_def struct in the leap code we should have all fields in eos-go as well: https://github.com/AntelopeIO/leap/blob/7da37b6bc41a63a9eaef5e79ff7aaf2aea854826/libraries/chain/include/eosio/chain/abi_def.hpp#L113
The action_results are wrapped into a may_not_exist<>, not sure if this is reflected into the binary code as well? But this is also true for variants which encode fine.
Any idea where this 0x00 byte difference might come from?
So I checked the linked element, it's defined as:
may_not_exist<vector<action_result_def>> action_results;
Which I think it's why we use eos: "binary_extension", so the definition should be:
...
Variants []VariantDef `json:"variants,omitempty" eos:"binary_extension"`
ActionResults []ActionResultDef `json:"action_results,omitempty" eos:"binary_extension"`
And if the check still fails saying binary_extension should be last, it should be extended to ensure multi fields can work.
The 0x00 comes most probably from variants in this case, the order of the fields is super important so not sure why I suggested to re-order them (it was a bad suggestion).
Just to be clear, the order to respect is the one defined by the FC_REFLECT macro:
FC_REFLECT( eosio::chain::abi_def , (version)(types)(structs)(actions)(tables)
(ricardian_clauses)(error_messages)(abi_extensions)(variants)(action_results) )
Yes I also tested different field orderings and applying the binary_extension as well to action results. But that still ends up with an 0x00 flag more on the setabi code only this time it's at the end vs in the middle.
Probably running just the decoding test with full tracing TRACE=true might give some clues here.
Could be caused from:
// Here we remove the last byte (in hex so 2 characeters) since variants are always serialized, but we want a test where they are not there
buffer, err := hex.DecodeString(systemABIGeneratedV1_2[0 : len(systemABIGeneratedV1_2)-2])
require.NoError(t, err)
I played around a bit with the TestABI_ReadPacked_V1_2 test. Seems like the abi decoder works just fine, it marshals into the same json as the abi to compare to (if we remove the omitempty json tags). It also works fine if I remove the trailing 0x00 byte from the systemABIGeneratedV1_2 string. So it seems like this byte is not holding any data.
I assume that there is either some rule or flag we are missing that has been added to the abi serialisation or there is a bug in the leap code responsible for this serialisation. So I'll create an issue on the leap repository for investigation.
Edit: I've commented out that last byte removal on my test so it's not affecting the decoding.
The issue regarding the empty byte is solved (was using the wrong cleos version which created some weird abi deployment issues).
I've removed the tests for eosio abi version 1.0 and 1.1. The test for 1.2 got updated with a larger ABI utilising more fields. Decoding and encoding tests are running fine.
Merging this in will drop support for anything below 3.1.