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

Cosmos ledger can't sign TX #4731

Closed
mhagel opened this issue Aug 8, 2023 · 9 comments · Fixed by #4811
Closed

Cosmos ledger can't sign TX #4731

mhagel opened this issue Aug 8, 2023 · 9 comments · Fixed by #4811
Assignees
Labels
5 Few days task bug Something isn't working

Comments

@mhagel
Copy link
Contributor

mhagel commented Aug 8, 2023

Describe the bug

Unable to sign with ledger.
Error on Keplr: Incompatible Signing Requested - Error: SIGN_MODE_DIRECT can't be signed on Ledger. Contact the web app provider to fix this issue

To Reproduce

Attempt to create a proposal with a ledger account.

Expected behavior

I can sign a tx with a ledger address (create a proposal)

Additional context

Quasar reported

@mhagel mhagel added bug Something isn't working needs estimate labels Aug 8, 2023
@mhagel mhagel changed the title Cosmos ledger signing Cosmos ledger can't sign TX Aug 8, 2023
@mhagel mhagel self-assigned this Aug 8, 2023
@mhagel mhagel added 5 Few days task and removed needs estimate labels Aug 9, 2023
@mhagel
Copy link
Contributor Author

mhagel commented Aug 9, 2023

Progress so far:

I am able to sign vote and create-text-proposal transactions with ledger by using window.keplr.getOfflineSignerOnlyAmino.

But the only proposal type supported by CosmJS so far is TextProposal. So it throws this error for Community Spend proposal:
Unsupported proposal type: '/cosmos.distribution.v1beta1.CommunityPoolSpendProposal'. Ledger requires amino encoding for anything you pass from keplr to a ledger device.

The relevant line of code is here: https://github.com/cosmos/cosmjs/blob/358260bff71c9d3e7ad6644fcf64dc00325cdfb9/packages/stargate/src/modules/gov/aminomessages.ts#L169-L192

There is some movement on updating amino types in cosmjs-types via telescope: confio/cosmjs-types#68 , but I don't know yet if this will add support for CommunityPoolSpendProposal.

I may raise a request with cosmjs to support other proposal types for amino MsgSubmitProposal.

Another option: We may be able to implement it ourselves with telescope (like we did with gov v1), but this is non-trivial.

Immediate option is to enable Ledger support, but leave CommunitySpendProposal ledger support as a follow-up issue.

@mhagel
Copy link
Contributor Author

mhagel commented Aug 10, 2023

check makeSignDoc for similar issue

@mhagel
Copy link
Contributor Author

mhagel commented Aug 10, 2023

I'm pretty sure we need to make our own implementation of

https://github.com/cosmos/cosmjs/blob/main/packages/stargate/src/signingstargateclient.ts#L392

so we can manually register the needed amino types.

@mhagel
Copy link
Contributor Author

mhagel commented Aug 15, 2023

Current challenge. Using a Ledger device, submit a MsgProposalSubmit with a type /cosmos.distribution.v1beta1.CommunityPoolSpendProposal.

My problem right now:

  • I tried using Telescope to generate the needed aminoTypes myself. It seems that the typing was insufficient. I can make a successful MsgSubmitProposal TX with CommunitySpendProposal, but the chain responds with error: Broadcasting transaction failed with code 5 (codespace: gov). Log: proposal title cannot be blank: invalid proposal content even though the title is populated.

    • So the generated typing is wrong, or the chain code itself is not ready for Amino CommunitySpend proposals (unlikely).
  • I have also tried using cosmJs makeSignDoc to encode in bytes instead of JSON. I have been unsuccessful with this so far, probably because I haven't tried to bypass cosmJs as the signingClient. I believe it may work if I make an alternate signing client that doesn't enforce the registered amino types. This is my best plan right now. Note the loss of amino-type-checking using this plan.

    • One problem with this method: it requires the user to sign an encoded message instead of being able to see human-readable JSON.

This article is useful in that it explains how Amino encoding can be in JSON or binary.

@jnaviask
Copy link
Collaborator

@mhagel

Re makeSignDoc, let's nix this approach -- we don't want users signing random binary blobs.

Re Telescope -- the part that confuses me is that I see the types/encoding for CommunityPoolSpendProposal right in node_modules/cosmjs-types/cosmos/distribution/v1beta1/distribution.d.ts. What's preventing us from encoding using this logic, or alternatively, what's the issue with the existing encodeCommunitySpend call?

@mhagel
Copy link
Contributor Author

mhagel commented Aug 15, 2023

what's the issue with the existing encodeCommunitySpend call?

This encodes correctly, but when routed through cosmJs it gives the original error noted, Unsupported proposal type: '/cosmos.distribution.v1beta1.CommunityPoolSpendProposal'

I see the types/encoding for CommunityPoolSpendProposal right in node_modules/cosmjs-types/cosmos/distribution/v1beta1/distribution.d.ts. What's preventing us from encoding using this logic?

The current encodeCommunitySpend uses this, so same problem.


So cosmJS is being used twice here:

  1. for encoding (this works as expected)
  2. as a signingStargateClient:
    a. triggering the offlineSigner (direct or amino) to prompt for user signature,
    b. double-checking the amino-types
    c. broadcasting the TX to the chain and handling the response.

The issue arises in 2b. CosmJS only supports TextProposal in amino, thus the error.

My thought for using Telescope is that we could generate the amino code we need, and manually add support for CommunitySpend if necessary. Telescope also provides a SigningClient.

@jnaviask

@jnaviask
Copy link
Collaborator

@mhagel this is clarifying, thanks! Investigating further: node_modules/@cosmjs/stargate/build/signingstargateclient.js has a list of defaultRegistryTypes which excludes /cosmos.distribution.v1beta1.CommunityPoolSpendProposal -- perhaps we can provide it at construction time via options.registry by appending the correct tx type to the exported defaultRegistryTypes array? Might be a quick fix to get it accepting that tx type.

@mhagel
Copy link
Contributor Author

mhagel commented Aug 15, 2023

@jnaviask Yes this is probably the recommended way. I considered this before and got sidetracked somehow, but I will give it another shot.

Edit: although I believe the options.aminoTypes is the area that needs modification. I'll check both though.

@mhagel
Copy link
Contributor Author

mhagel commented Aug 23, 2023

I made some devnet improvements to make this easier to test. That added at least 3 points of effort that I hadn't accounted for in my initial estimate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 Few days task bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants