cosmos / cosmos-rust

The home of all shared Rust resources for the Cosmos ecosystem.
Apache License 2.0
306 stars 122 forks source link

Add alternate type for proto::cosmos::tx::v1beta1::TxRaw that is compatible with ledger. #309

Open ewoolsey opened 1 year ago

ewoolsey commented 1 year ago

I could be a little off base here as my understanding of this is pretty shallow at best. Ledgers expect a specific version of "SignDoc" that must have all json objects sorted by key. you can see the spec here. It seems to me that that would requires a different form for Raw, as the current implementation separates "auth_info" and "body". When these are passed to the ledger, they must first be sorted. The sorted version is signed, and then transforming back into Raw causes a jumble, and invalidates the signature. Is my understanding of this problem correct?

ewoolsey commented 1 year ago

Perhaps both body and auth_info will need different implementations as well.

tony-iqlusion commented 1 year ago

To my knowledge the only format the Ledger supports is Amino JSON, which predates and is orthogonal to Protobufs.

There's no support for Amino in this crate. If there's enough interest we could support Amino JSON, but that's a question for #309

tony-iqlusion commented 1 year ago

FWIW, I wrote an Amino library a couple years ago, though it was mainly aimed at the binary format:

https://github.com/iqlusioninc/crates/tree/main/stdtx

It does have some JSON support, although at this point I couldn't tell you what state it's in.

ewoolsey commented 1 year ago

So the problem I'm having in particular is how to package up the TxRaw after signing. I can properly get the json payload to the ledger and sign it, but I don't know how I can then take the signature that's returned, and bundle it into TxRaw. There's very little documentation around for me to go off of so I'm wandering in the dark a bit haha.

ewoolsey commented 1 year ago

That library could be helpful, but Everything I've written so far is dependent on cosmrs, and I'd like to avoid branching out if possible. Do you not think it would be possible to accomplish this with cosmrs?

tony-iqlusion commented 1 year ago

You need to use the LegacyAminoJson sign mode: https://docs.rs/cosmrs/latest/cosmrs/tx/enum.SignMode.html#variant.LegacyAminoJson

I believe the transaction will need to be encoded in the Amino binary wire format (even though the signature is over the JSON).

The easiest way to confirm any of this would be to find a working transaction and use cosmrs to parse it. That should give you an idea of what the inputs to cosmrs need to be.

ewoolsey commented 1 year ago

That's very helpful thank you. I'll look into that right now!

The easiest way to confirm any of this would be to find a working transaction and use cosmrs to parse it. That should give you an idea of what the inputs to cosmrs need to be.

Could you elaborate on that idea a little more?

tony-iqlusion commented 1 year ago

Find the Protobuf binary serialization of any working transaction signed by a Ledger and parse that.

We can add it to the test vectors if you'd like.

I can look for one if you want.

ewoolsey commented 1 year ago

I can look for one if you want.

Sure if you'd like to help! That would be great.

ewoolsey commented 1 year ago

I switched to using the legacy amino sign mode but my transactions are still failing the signature verification step.

tony-iqlusion commented 1 year ago

I believe that's because they need to be serialized using the Amino binary encoding. Are you doing that?

ewoolsey commented 1 year ago

Steps:

ewoolsey commented 1 year ago

I believe that's because they need to be serialized using the Amino binary encoding. Are you doing that?

at what point in the process are you suggesting it be serialized using the Amino binary encoding? I'm just using

let tx_commit_response = tx_raw.broadcast_commit(client).await?;

to broadcast

tony-iqlusion commented 1 year ago

FWIW here's stdtx's Amino JSON builder:

https://github.com/iqlusioninc/crates/blob/0d941bae638dbb25182b69e2bdbc33ded047bc2e/stdtx/src/amino/builder.rs#L73-L106

at what point in the process are you suggesting it be serialized using the Amino binary encoding?

I don't know it's actually doing that. The transaction is hopefully still Protobuf even though it's using Amino JSON for signing.

I'm just guessing here. This is why we need an example transaction which uses SignMode::LegacyAminoJson to use as a reference.

ewoolsey commented 1 year ago

This is why we need an example transaction which uses SignMode::LegacyAminoJson

Not exactly sure the easiest way to get something like that. I'm so close to making this work this is literally the last step I'm stuck on ahah.

ewoolsey commented 1 year ago

FWIW here's stdtx's Amino JSON builder:

https://github.com/iqlusioninc/crates/blob/0d941bae638dbb25182b69e2bdbc33ded047bc2e/stdtx/src/amino/builder.rs#L73-L106

at what point in the process are you suggesting it be serialized using the Amino binary encoding?

I don't know it's actually doing that. The transaction is hopefully still Protobuf even though it's using Amino JSON for signing.

I'm just guessing here. This is why we need an example transaction which uses SignMode::LegacyAminoJson to use as a reference.

Once you get the signature how do you then broadcast with that library?

tony-iqlusion commented 1 year ago

You won't be able to with stdtx.

It will need to use https://docs.rs/cosmrs/latest/cosmrs/tx/struct.Raw.html#method.broadcast_commit, but you'll have to actually solve this issue before that will work

ewoolsey commented 1 year ago

sounds good to me. Well thanks for your help on this issue! Really appreciate it. If you come up with any other ideas for how to get this to work let me know. This is the repository I'm currently working on for reference.

ewoolsey commented 1 year ago

Following up here! Did some digging and found that tmkms uses your stdtx package to solve exactly this problem. Examples can be found in tx_signer.rs. I'll be using that as an example, and hopefully I can get this all sorted.

ewoolsey commented 1 year ago

@tony-iqlusion you mentioned above that it might be possible to get amino support within cosmrs and I think that's exactly what my use case requires! I'll try and build around that for now, but as a long term goal that would be fantastic.