Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Create ABI for all native contracts #215

Closed
bytemaster opened this issue Aug 22, 2017 · 16 comments
Closed

Create ABI for all native contracts #215

bytemaster opened this issue Aug 22, 2017 · 16 comments

Comments

@bytemaster
Copy link
Contributor

all of the built-in contracts need to define a proper ABI so that the JSON-RPC interface works properly.

@heifner
Copy link
Contributor

heifner commented Aug 25, 2017

The wallet web ui guys are going to need this soon for create account (types::newaccount). Maybe you could do that part first or assign that part to me.

@heifner
Copy link
Contributor

heifner commented Aug 28, 2017

Dan's branch:
https://github.com/EOSIO/eos/commits/pushtrx-json
Has a fix that allows push_transcation Message data to be json.

@heifner
Copy link
Contributor

heifner commented Aug 29, 2017

I added types::newaccount in branch wallet-ui-test-240 for the wallet ui guys to use to test. So now as long as this is done by test network release they should be good.

@bytemaster
Copy link
Contributor Author

I believe this is complete once the latest 'experience' branch is merged.

@brianjohnson5972
Copy link
Contributor

@bytemaster You assigned this to me, but your last statement sounds like it is completed. I'm presuming that means I just need to track this to make sure it is wrapped up.

@heifner
Copy link
Contributor

heifner commented Sep 5, 2017

@brianjohnson5972 Could add some tests for Dan's additions.

@jcalfee
Copy link
Contributor

jcalfee commented Sep 6, 2017

A cpp test would be good.. Assuming my test is correct, if a type is does not have an ABI and you send in a json object in Message.data, the error message will be: Invalid cast from type 'object_type' to string

A better error message is going to help someone in the future.

This is a test from eosjs:

api error => http://127.0.0.1:8888/v1/chain/push_transaction {"refBlockNum":52,"refBlockPrefix":609021817,"expiration":"2017-09-06T18:25:51","scope":["inita"],"readscope":[],"messages":[{"code":"eos","type":"newaccount","authorization":[{"account":"inita","permission":"active"}],"data":{"creator":"inita","name":"acct54","owner":{"threshold":1,"keys":[{"key":"EOS6MRyAjQq8ud7hVNYcfnVPJqcVpscN5So8BhtHuGYqET5GDW5CV","weight":1}],"accounts":[]},"active":{"threshold":1,"keys":[{"key":"EOS6MRyAjQq8ud7hVNYcfnVPJqcVpscN5So8BhtHuGYqET5GDW5CV","weight":1}],"accounts":[]},"recovery":{"threshold":1,"keys":[{"key":"EOS6MRyAjQq8ud7hVNYcfnVPJqcVpscN5So8BhtHuGYqET5GDW5CV","weight":1}],"accounts":[]},"deposit":{"amount":"10","symbol":"0"}}}],"signatures":["1f51cb9ac98cb46c5b33b428ad051f7521640c5b8215fb501c39535bc42ead9a6b2daa3841ee8a8f61e0e7fdb4914cf99d831d6ab93cb5e8a2729d88b4c2be0e03"]} { Error: 7 bad_cast_exception: Bad Cast
Invalid cast from type 'object_type' to string
    {"type":"object_type"}
    thread-0  variant.cpp:565 get_string

@brianjohnson5972
Copy link
Contributor

@heifner Can this be tested in unit tests, or can we only get to this interface using ./eosd?

@heifner
Copy link
Contributor

heifner commented Sep 6, 2017

For native I think you should just be able to do a to/from json using fc::json and verify the types are decoded appropriately.

@jcalfee
Copy link
Contributor

jcalfee commented Sep 7, 2017

In the unit tests, please consider this case:

You know how message.data in the json transaction can be hex right? This happens when there is no ABI for the message.type. Well, a Hex "string" already has a known length so the message.data hex string should not need a length prefix (in hex).

@bytemaster
Copy link
Contributor Author

Yes, this needs "tested" using ApiSerializer on all native messages.

@jcalfee
Copy link
Contributor

jcalfee commented Sep 8, 2017

This is another test you might want to include or document.

Always length prefix the binary (on-chain) representation message.data. This is the existing behavior. Reason: parsers without access an ABI require this prefix to parse the remaining transaction.

@jcalfee
Copy link
Contributor

jcalfee commented Sep 9, 2017

I just found out, the serialization behavior is 100% correct. The only bug I see is that when calling post_transaction, message.data must be hex for the newaccount transaction (probably others too). The transfer transaction works as json or hex.

I did suspect a serialization bug, however, this is not the case..

@brianjohnson5972
Copy link
Contributor

I was working through adding tests for all the native message types. I should be able to complete it by tomorrow morning.
@jcalfee and @heifner can you guys comment more on @jcalfee 's comment from 4 days ago? The comment is a little confusing and I thought that message.data was fixed over a week ago (I don't know how wallet API app could be working otherwise).

@heifner
Copy link
Contributor

heifner commented Sep 13, 2017

@brianjohnson5972 I'm not sure what @jcalfee is referring to. The newaccount did require hex before the changes made for this issue by Dan. I've verified via curl that pure json works for newaccount and transfer both of which are needed for the wallet ui demo.

@brianjohnson5972
Copy link
Contributor

Created pull request for unit tests for all type definitions defined in native_contract_chain_initializer::prepare_database.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants