Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: StdTx #52

Closed
wants to merge 9 commits into from
Closed

WIP: StdTx #52

wants to merge 9 commits into from

Conversation

ebuchman
Copy link
Member

@ebuchman ebuchman commented Apr 11, 2020

Took a first stab at #47. Very rough draft (some of these types will belong in tendermint-rs, etc.), just posting to show what I was able to do:

  • marshal a StdTx with the MsgCreateClient to JSON
  • sign the JSON with gaiacli
  • load the signed StdTx back into Rust
  • almost fully Amino encode it

There are lots of rough edges, and I found some roughness in the Go too (eg. cosmos/cosmos-sdk#5983)

Note the Iqlusion StdTx library was somewhat useful, but not fully:

  • it expects Msgs to be specified in a toml file
  • from what I can tell, Msgs are limited to certain "basic" fields, so you can't eg easily encode a Tendermint header in there
  • the StdTx type in there expects the Msg to be already Amino encoded
  • the JSON encoding isn't fully compliant with Tendermint

That said, the StdFee, StdSignature, and Address types from that lib were useful, after a few modifications. But we might want to consider just moving those types and logic, if not the whole crate, into here.

As for Amino encoding, it seems we still need to make some fixes to amino-rs to fully get there. I'm comparing the Amino encoding of my MsgCreateClient with one generated from the Go and it's still off by just a little (mostly seems to be around extra length prefixing). I included the Go script in here (amino.go), but it should be run from the gaia repo (ibc-alpha branch).

Meanwhile, I had to make changes to tendermint-rs, amino-rs, and stdtx crates to get here:

Note the Cargo's were updated to use local paths to facilitate development - I'm not sure if there's a nicer way to be doing that without messing up the file :)

At a higher level, I'm not sure if any of this is even the right approach at all, especially since once we go full proto, we'll need to adjust. But I wanted to try and get something working with the current state of things. Hopefully the protobuf changes for Gaia and Tendermint will land soon; it probably doesn't make sense to go too much further here in Amino land unless we can keep it relatively simple. We might want to start looking at the development branches for the proto migration and see if it makes sense to work against those so we can get a feel for the ultimate state of things sooner.


For contributor use:

  • Unit tests written
  • Added test to CI if applicable
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer

@ebuchman
Copy link
Member Author

ebuchman commented Aug 4, 2020

Old and stale so closing. Proto txs have landed in the SDK so we can use that

@romac romac deleted the bucky/stdtx branch April 8, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant