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 Amino support for CommunityPoolSpendProposal #4811

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

mhagel
Copy link
Contributor

@mhagel mhagel commented Aug 15, 2023

Link to Issue

Closes: #4731

Description of Changes

  • Adds Ledger (amino) support for all supported Cosmos onchain transactions except:
    • MsgSubmitProposal with CommunityPoolSpendProposal in v1 (note that this does support it in v1beta1)
    • The automated test for this is skipped and marked for new issue...
  • Bumps cosmJs to 0.31.0, and makes associated migration changes

"How We Fixed It"

Added gov/aminomessages.ts to amino registry when creating signingClient

Test Plan

  • Note: I (or anybody) can send tokens to whoever wants to QA with a Ledger.
    • Recommended for QA:
      • if running Redis: redis-cli > FLUSHALL (this will clear devnet sandbox params which change with this PR)
      • In Keplr, Settings > General > Manage Non-native Chains > Delete Cosmos Devnet chains (we will re-add them)
  • Amino tests added to devnet suite (create both types of proposals, vote).
  • CA (click around) tested on local:
    • Using a Ledger device with staked tokens,
    • create Text Proposal and Community Spend Proposal on /csdk-beta, and vote
    • create Text Proposal and vote on /csdk (spend prop for v1 is a follow-up, see below)

Deployment Plan

Other Considerations

@mhagel mhagel force-pushed the mark.4731-ledger-aminoTypes-mod branch 3 times, most recently from 466ade4 to 7b8d31b Compare August 16, 2023 19:16
@mhagel
Copy link
Contributor Author

mhagel commented Aug 17, 2023

Ledger support is ready here for everything except v1 CommunityPoolSpendProposal tx. v1beta1 works, but in v1 I am getting a classic (miselading) Cosmos error from the chain:

Broadcasting transaction failed with code 4 (codespace: sdk). Log: signature verification failed; please verify account number (9799), sequence (2) and chain-id (kyve-1): unauthorized

See https://github.com/hicommonwealth/commonwealth/actions/runs/5883047794/job/15954867599?pr=4811

In the past I have seen this when amino encoding is incorrect, although it is also a general error that pops up if anything is wrong with the TX. Despite the wording of the error, this is probably not a signature error. In manual testing, the message appears correct, so it will take some more investigation to resolve.

I propose that we split off a separate ticket for v1 CommunityPoolSpendProposal ledger support. That will allow the rest of the ledger support to move forward.

@CowMuon
Copy link
Contributor

CowMuon commented Aug 17, 2023

Great update Mark. Please go ahead and add that new ticket, cross-linking to original ticket for this PR (I know this ain't Jira) and let's get this out of draft.

@mhagel
Copy link
Contributor Author

mhagel commented Aug 17, 2023

Please go ahead and add that new ticket,

@CowMuon New ticket: #4821

let's get this out of draft.

I need to make some updates to our devnet deployments to make this QA-ready. I have tested other chains, but csdk and csdk-beta are not registering unstaked tokens on Keplr. Whoever QAs it would also need a physical ledger device to test with.

@NakulManchanda I believe we may need to get rid of the nginx integration, or adjust it, because it may be affecting RPC requests. Or the problem may be something else. I will get in touch with you for help.

@mhagel mhagel force-pushed the mark.4731-ledger-aminoTypes-mod branch 2 times, most recently from 1cec0b5 to f2726f5 Compare August 17, 2023 05:05
@mhagel
Copy link
Contributor Author

mhagel commented Aug 22, 2023

Latest commit fixes devnet issues. The bootstrap code included is already deployed to the sandboxes and the CI images.

It fixes the native send functionality for keplr. Which means we can test other ledger devices by sending tokens to them.

mhagel and others added 2 commits August 22, 2023 17:37
…er CommunitySpend in MsgSubmitProposal

- encodeCommunitySpend to utils, devnet tests for communitySpend
- DRY up tests and add amino tests for all
- null check for mindeposit.denom to fix intermittent bug
fix dasel syntax
unstaked tokens visible in keplr
fix platform for docker
update wiki
new image ref for CI tests
@mhagel mhagel force-pushed the mark.4731-ledger-aminoTypes-mod branch from 0c2c659 to 3fa6469 Compare August 22, 2023 23:38
@mhagel mhagel marked this pull request as ready for review August 23, 2023 14:59
@jnaviask jnaviask merged commit d2e5e96 into master Aug 23, 2023
6 checks passed
@jnaviask jnaviask deleted the mark.4731-ledger-aminoTypes-mod branch August 23, 2023 16:10
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.

Cosmos ledger can't sign TX
3 participants