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: Better JSON support #391

Closed
wants to merge 2 commits into from

Conversation

ebuchman
Copy link

I've been working on prototype of IBC txs in informalsystems/hermes#52.

Tried to use this lib where I could, but had a few issues. It's possible I misunderstood how to use it, so please let me know if I'm doing something super wrong, but here's what I found:

  • Wasn't clear what the use case of defining Msgs in a toml is. I need to be able to define them in Rust (for now, later in proto)
  • Seems it currently only works for Msgs with primitive and hardcoded types (?). I couldn't see how to use a Msg that eg. contained a Tendermint header
  • Seems I can't get compliant JSON encodings.

To unblock my current work, I made a few changes here. I'm not sure exactly what the plans are with this crate, or if these changes make sense for it, but regardless I think Informal would be happy to maintain it, or something like it, in the ibc-rs.

The changes I made were:

  • implement Serialize/Deserialize for the Address so it can be used in Msg types and properly bech32 encoded and decoded in JSON (I see the to_json_value methods but didn't quite figure out how to use them); one caveat here is the cosmos prefix is hardcoded, will take more work to generalize that (maybe it should be a field in the Address?)
  • derive Serialize/Deserialize for Coin, StdFee, and StdSignature
  • implement custom encoder/decoder to map u64 to string (for proto3 and tendermint JSON)
  • implement custom encoder/decoder to map bytes to base64 string (for proto3 and tendermint JSON)
  • fix the pub_key field name in StdSignature

With these changes, and the StdTx I defined in informalsystems/hermes#52, I was able to create an unsinged StdTx JSON for an ibc message, sign it using gaiacli, unmarshal the signed JSON back into Rust, and then encode it as not-fully-correct Amino (that part's still a WIP).

@ebuchman ebuchman mentioned this pull request Apr 11, 2020
6 tasks
@ebuchman ebuchman marked this pull request as draft April 11, 2020 04:28
@tony-iqlusion
Copy link
Member

tony-iqlusion commented Apr 11, 2020

Wasn't clear what the use case of defining Msgs in a toml is. I need to be able to define them in Rust (for now, later in proto)

The use case for TOML schema definitions was supporting runtime schema registration via configuration files for KMS-backed transaction signing in such a way that the KMS can build and sign transactions for arbitrary Tendermint applications with configuration changes alone, rather than adding application-specific code changes for every transaction format under the sun:

tendermint/tmkms#386

(As it were, I think this particular problem of dynamically handling transactions produced by arbitrary applications may be unique to the KMS in the entire Cosmos ecosystem)

We tried going down the latter route in our fork of deep_space, and the result was rather messy (and couldn't actually properly serialize a validating StdTx). The goal of this crate is to solve that problem once in a declarative, configuration-driven manner, rather than having to do a release of e.g. deep_space each time someone wants to add support for another app (or add/update transactions for a chain), and then bump the deep_space dependency in the KMS and then do a KMS release.

All of that said, the TOML is just the serde serialization of the equivalent Rust types, which you can write in Rust syntax if you prefer.

  • Seems it currently only works for Msgs with primitive and hardcoded types (?). I couldn't see how to use a Msg that eg. contained a Tendermint header
  • Seems I can't get compliant JSON encodings.

Admittedly I implemented only what was needed to generate the transaction messages for Terra oracles (we have this crate deployed in production generating them). It's doubtless incomplete in many areas.

That said, this crate is working in production today producing compliant JSON encodings (for signing) which are validating on Terra columbus-3. I'm guessing you're not using the right APIs if it's not working?

I'm not sure exactly what the plans are with this crate, or if these changes make sense for it, but regardless I think Informal would be happy to maintain it, or something like it, in the ibc-rs.

This crate is a stopgap to do KMS-backed transaction signing with the current Amino-based StdTx format. I'd love to phase it out eventually when the proto3 changes all land, and would be happy to figure out a way to merge it in its current form into ibc-rs as it's not exactly something I'm super excited about maintaining.

implement Serialize/Deserialize for the Address so it can be used in Msg types and properly bech32 encoded and decoded in JSON (I see the to_json_value methods but didn't quite figure out how to use them); one caveat here is the cosmos prefix is hardcoded, will take more work to generalize that

Avoiding the hardcoded prefix is why Address doesn't impl Serialize/Deserialize at present, i.e. it's already generalized.

The to_json_value methods use a bech32 prefix sourced from the schema for a particular chain contextually depending on if it's an account or validator address:

https://github.com/iqlusioninc/crates/blob/develop/stdtx/src/msg/value.rs#L61

(maybe it should be a field in the Address?)

If the Address is parsed from the binary Amino serialization, the bech32 prefix can't be known a priori, hence the current schema-driven approach.

The rest of the changes sound like you're tracking upstream Tendermint v0.33 changes? (not sure myself) All I know is it's working today in its current form in production with Terra columbus-3.

Perhaps what'd make sense is to leave this crate as is, upstream the code into ibc-rs, and then you can make the changes you need and this crate can continue working with apps based on legacy versions of Tendermint until they're phased out, at which point this crate can be retired as well.

@ebuchman
Copy link
Author

Wow thank you for the very detailed and informative explanation! I am targeting a quite bleeding edge version of the SDK (ibc-alpha branch), so probably the incompatibilities are all due to that. I will close this PR and leave this crate as is, and start copying what we need into ibc-rs for now - thanks!

As for the schema-based use case, that's actually very cool, and conceivably relevant to a full-time relayer node, so at some point it might be interesting to try and expand the scope of msg schemas in can support!

@ebuchman ebuchman closed this Apr 12, 2020
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.

2 participants