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

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

Open
0xForerunner opened this issue Dec 9, 2022 · 20 comments

Comments

@0xForerunner
Copy link

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?

@0xForerunner
Copy link
Author

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

@tony-iqlusion
Copy link
Member

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
Copy link
Member

tony-iqlusion commented Dec 9, 2022

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.

@0xForerunner
Copy link
Author

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.

@0xForerunner
Copy link
Author

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
Copy link
Member

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.

@0xForerunner
Copy link
Author

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
Copy link
Member

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.

@0xForerunner
Copy link
Author

I can look for one if you want.

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

@0xForerunner
Copy link
Author

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

@tony-iqlusion
Copy link
Member

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

@0xForerunner
Copy link
Author

0xForerunner commented Dec 9, 2022

Steps:

  • Tx info is serialized with serde_json and sent via apdu to the ledger.
{
 "account_number": {number},
 "chain_id": {string},
 "fee": {
   "amount": [{"amount": {number}, "denom": {string}}, ...],
   "gas": {number}
 },
 "memo": {string},
 "msgs": [{arbitrary}],
 "sequence": {number}
}

the data is returned and converted to a signature via

Signature::from_der(data.as_slice())

Then I build the TxRaw

let anys = msgs.iter().map(|msg| msg.to_any()).collect::<Result<Vec<_>, _>>().unwrap();
let tx_body = tx::Body::new(anys, memo.clone(), timeout_height);
let signer_info = SignerInfo {
    public_key: Some(public_key.into()),
    mode_info:  ModeInfo::Single(Single { mode: SignMode::LegacyAminoJson }),
    sequence:   account.sequence,
};
let ledger_auth_info = AuthInfo { signer_infos: vec![signer_info], fee: fee.clone() };
let raw = cosmos_sdk_proto::cosmos::tx::v1beta1::TxRaw {
    body_bytes:      tx_body.into_bytes().unwrap(),
    auth_info_bytes: ledger_auth_info.into_bytes().unwrap(),
    signatures:      vec![signed.to_vec()],
}

@0xForerunner
Copy link
Author

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
Copy link
Member

tony-iqlusion commented Dec 9, 2022

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.

@0xForerunner
Copy link
Author

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.

@0xForerunner
Copy link
Author

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
Copy link
Member

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

@0xForerunner
Copy link
Author

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.

@0xForerunner
Copy link
Author

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.

@0xForerunner
Copy link
Author

@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.

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

No branches or pull requests

2 participants