R4R: Fixes gaia/keybase storage of ledger accounts #3586

jleni · 2019-02-10T13:45:02Z

Closes #3573

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

alessio

ACK - pending tests

adrianbrink · 2019-02-10T15:08:18Z

Sending tokens from a ledger doesn't work for me with this patch.

➜  cosmos-sdk git:(fix/ledger_add) gaiacli tx send cosmos1eysta0x7quczxkecrm6f36assewqddr0879wgv 33333photino --ledger --chain-id=gaia-11001 --node="tcp://35.198.155.173:26657"
ERROR: No account with address cosmos1550dq7 was found in the state.
Are you sure there has been a transaction involving it?

The sending account has coins.

➜  cosmos-sdk git:(fix/ledger_add) gaiacli query account cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0 --trust-node --output json --node="tcp://35.198.155.173:26657" | jq
{
  "type": "auth/Account",
  "value": {
    "address": "cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0",
    "coins": [
      {
        "denom": "photino",
        "amount": "1000000"
      }
    ],
    "public_key": null,
    "account_number": "989",
    "sequence": "0"
  }
}

And is registered with gaiacli.

➜  cosmos-sdk git:(fix/ledger_add) gaiacli keys list
NAME:   TYPE:   ADDRESS:                                                PUBKEY:
test_ledger     ledger  cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0   cosmospub1addwnpepqduy2jxt3rmuggcanwa6nf894s4e5ncckrjzvnxum2f8ehd07zhzgr05gay
test_local      local   cosmos1swsspfxys4frpzm3h35xef89x9qdcy04nan6q4   cosmospub1addwnpepqf25d2z48n6l6ynf4n3pzmckwn74zwxnl7r7f4cu0tzaagprdgf0jlsgmrq

When I use --ledger do I need to specify a key name with --from?

When I use it with --from

➜  cosmos-sdk git:(fix/ledger_add) gaiacli tx send cosmos1eysta0x7quczxkecrm6f36assewqddr0879wgv 33333photino --ledger --from test_ledger --chain-id=gaia-11001 --node="tcp://35.198.155.173:26657"
ERROR: please open Cosmos app on the Ledger device - error: Error code: 6804

jleni · 2019-02-10T15:10:00Z

Sending tokens from a ledger doesn't work for me with this patch.

➜  cosmos-sdk git:(fix/ledger_add) gaiacli tx send cosmos1eysta0x7quczxkecrm6f36assewqddr0879wgv 33333photino --ledger --chain-id=gaia-11001 --node="tcp://35.198.155.173:26657"
ERROR: No account with address cosmos1550dq7 was found in the state.
Are you sure there has been a transaction involving it?

Did you create the ledger address again? the problem was not in send but in add

adrianbrink · 2019-02-10T15:10:41Z

Yes, I deleted all the existing ledger addresses and added this one with gaiacli keys add test_ledger --ledger

jleni · 2019-02-10T15:11:48Z

I did test it using --from maybe there is still another problem when from is not used.

adrianbrink · 2019-02-10T15:36:01Z

Hm, I still get this error:

➜  cosmos-sdk git:(fix/ledger_add) gaiacli tx send cosmos1eysta0x7quczxkecrm6f36assewqddr0879wgv 33333photino --from test_ledger --chain-id=gaia-11001 --node="tcp://35.198.155.173:26657"
ERROR: please open Cosmos app on the Ledger device - error: Error code: 6804

Which version of the Cosmos Ledger application are you using?

jleni · 2019-02-10T15:37:19Z

This is what I get on my side:

WORKS:
gaiacli tx send cosmos13jvkxxer957fr603n430qqyv7xrg9nntgnlat9 20photino --from=nanotest --ledger --fees=20000photino --chain-id=gaia-11001

DOES NOT WORK (for a different reason)
gaiacli tx send cosmos13jvkxxer957fr603n430qqyv7xrg9nntgnlat9 20photino --ledger --fees=20000photino --chain-id=gaia-11001

adrianbrink · 2019-02-10T15:40:58Z

Which app version are you running?

Does this error mean that I'm running the wrong app version? ERROR: please open Cosmos app on the Ledger device - error: Error code: 6804

jleni · 2019-02-10T15:41:40Z

The app is v1.0.1 (available at Ledger's app store)

jleni · 2019-02-10T15:47:28Z

Which version of the Cosmos Ledger application are you using?

I've just created a new issue to have a better error message in case the Cosmos app version is not supported.

#3588

Even if that might not be the case here, a better message will be anyway useful.

adrianbrink · 2019-02-10T15:47:31Z

I've tried with the app from Ledger Live, but I still get the same error. Let me try and sync a node locally so that I don't need the --node flag.

adrianbrink · 2019-02-10T15:48:28Z

As far as I can tell we are running the same setup, but for me it fails.

Different question: Is running make install enough to pull the newest dependencies?

jleni · 2019-02-10T15:50:25Z

Different question: Is running make install enough to pull the newest dependencies?

I think so, but I've seen people in Riot having trouble with that. I would need to investigate myself. Other people in the team should probably know better.

adrianbrink · 2019-02-10T15:57:58Z

Okay I'm getting somewhere. At least the error changed.

➜  cosmos-sdk git:(fix/ledger_add) gaiacli query account cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0 --trust-node --output json --node="tcp://35.198.155.173:26657" | jq
{
  "type": "auth/Account",
  "value": {
    "address": "cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0",
    "coins": [
      {
        "denom": "photino",
        "amount": "12000000"
      }
    ],
    "public_key": null,
    "account_number": "989",
    "sequence": "0"
  }
}
➜  cosmos-sdk git:(fix/ledger_add) ./build/gaiacli tx send cosmos13jvkxxer957fr603n430qqyv7xrg9nntgnlat9 1photino --from=nanotest --ledger --chain-id=gaia-11001 --node="tcp://35.234.98.115:26657"
ERROR: address cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0 doesn't have enough coins to pay for this transaction

The account clearly has enough photinos to pay for this transaction.

jleni · 2019-02-10T15:58:50Z

I've got that because of photino vs photinos

jleni · 2019-02-10T16:00:02Z

but this does not seem to be case here..

adrianbrink · 2019-02-10T16:01:15Z

This also happens with muon.

➜  cosmos-sdk git:(fix/ledger_add) gaiacli query account cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0 --trust-node --output json --node="tcp://35.198.155.173:26657" | jq
{
  "type": "auth/Account",
  "value": {
    "address": "cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0",
    "coins": [
      {
        "denom": "muon",
        "amount": "11943"
      },
      {
        "denom": "photino",
        "amount": "12011943"
      }
    ],
    "public_key": null,
    "account_number": "989",
    "sequence": "0"
  }
}
➜  cosmos-sdk git:(fix/ledger_add) ./build/gaiacli tx send cosmos13jvkxxer957fr603n430qqyv7xrg9nntgnlat9 943muon --from=nanotest --ledger --chain-id=gaia-11001 --fees=20000photino --node="tcp://35.234.98.115:26657"
ERROR: address cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0 doesn't have enough coins to pay for this transaction

adrianbrink · 2019-02-10T16:01:48Z

It never actually talks to the ledger. This appears even if the ledger isn't connected.

jleni · 2019-02-10T16:05:52Z

weird.. I have just sent you from my ledger to your account

gaiacli keys list                                                                                                                                                                                                                                                                                                                               14.9s  Sun 10 Feb 2019 17:02:51 CET
NAME:   TYPE:   ADDRESS:                                                PUBKEY:
nanorcv local   cosmos13jvkxxer957fr603n430qqyv7xrg9nntgnlat9   cosmospub1addwnpepqfnyjqgzk2p766ma82fljlf9a8e6y9eeu4nu79wmm9zzphwe6varcfk8f3q
nanotest        ledger  cosmos13vfzpfmg6jgzfk4rke9glzpngrzucjtanq9awx   cosmospub1addwnpepqvmzyf4dq3fjl9h2n00dydmf33dt6e7y8vrrlg4wcmvrag3qnrfsvm0pzu6

gaiacli tx send  cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0  20photino --from=nanotest --ledger --fees=20000photino --chain-id=gaia-11001
                                                                                                                                                                                                235ms  Sun 10 Feb 2019 17:02:31 CET
Response:
  Height: 10518
  TxHash: 8F8A3CD278FE28CC57B1931C2E31EB4D4FE44DE76B17B02D706CD0B86B439D09
  Log: Msg 0: 
  GasWanted: 200000
  GasUsed: 28407
  Tags: 
    - action = send
    - sender = cosmos13vfzpfmg6jgzfk4rke9glzpngrzucjtanq9awx
    - recipient = cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0

jleni · 2019-02-10T16:08:43Z

I will investigate a bit more tomorrow morning..
All these issues seem to be related to the way gaiacli is keeping track of accounts in the keybase.
This PR definitely addresses one of those where derivation paths were not stored.

I would say we merge this PR and keep the issue open to investigate other cases where the CLI be failing.

adrianbrink · 2019-02-10T16:21:35Z

Fixed it. It works now. I was using the wrong fullnode. duh

zmanian · 2019-02-10T16:38:48Z

Thank you guys

jleni · 2019-02-10T16:42:53Z

I would say the --ledger argument is not necessary...

If nanotest is known to be a ledger account:

NAME:   TYPE:   ADDRESS:                                                PUBKEY:
nanotest        ledger  cosmos13vfzpfmg6jgzfk4rke9glzpngrzucjtanq9awx   cosmospub1addwnpepqvmzyf4dq3fjl9h2n00dydmf33dt6e7y8vrrlg4wcmvrag3qnrfsvm0pzu6

If I write --from=nanotest :

gaiacli tx send  cosmos1qumptahqxn7awk6urce7sp4yrr2fmcdnnh8fu0  20photino --from=nanotest --ledger --fees=20000photino --chain-id=gaia-11001

gaiacli should automatically recognize that the source is a ledger address.

I will open an issue to track this small improvement

client/keys/add.go

crypto/ledger_test.go

Co-Authored-By: jleni <juan.leni@zondax.ch>

jackzampolin

Great fix @jleni and thank you for the debugging @adrianbrink !

jleni added 3 commits February 10, 2019 14:06

reproduced problem with a test

de9cc37

Adding additional failing test

bee9c24

Serializing derivation paths

2810075

jleni added T:Bug C:Keys Keybase, KMS and HSMs C:Ledger Issues and features related Ledger integration and functionality labels Feb 10, 2019

jleni requested review from alessio and jackzampolin February 10, 2019 13:45

jleni requested review from cwgoes, ebuchman and rigelrozanski as code owners February 10, 2019 13:45

jleni added 2 commits February 10, 2019 15:09

restructuring + pending.md

ceff918

Improve test

7c2b326

jleni changed the title ~~WIP: Fixes gaia/keybase storage of ledger accounts~~ R4R: Fixes gaia/keybase storage of ledger accounts Feb 10, 2019

Update PENDING.md

7edd0f1

alessio approved these changes Feb 10, 2019

View reviewed changes

Fixing test_cover issues

eef89e9

jleni mentioned this pull request Feb 10, 2019

gaiacli send tx should not require a --ledger argument #3589

Closed

alexanderbez reviewed Feb 10, 2019

View reviewed changes

client/keys/add.go Outdated Show resolved Hide resolved

crypto/ledger_test.go Outdated Show resolved Hide resolved

alexanderbez and others added 2 commits February 10, 2019 19:55

Update client/keys/add.go

3b5d9da

Co-Authored-By: jleni <juan.leni@zondax.ch>

review fixes

172a323

jackzampolin added this to the v0.31.2 (Minor prelaunch fixes) milestone Feb 11, 2019

jackzampolin approved these changes Feb 11, 2019

View reviewed changes

jackzampolin merged commit 94dccf3 into cosmos:develop Feb 11, 2019

jleni mentioned this pull request Feb 20, 2019

"gaiacli keys" Error(SDK 0.32.0) #3682

Closed

4 tasks

jleni deleted the fix/ledger_add branch February 21, 2019 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Fixes gaia/keybase storage of ledger accounts #3586

R4R: Fixes gaia/keybase storage of ledger accounts #3586

jleni commented Feb 10, 2019 •

edited

Loading

alessio left a comment •

edited

Loading

adrianbrink commented Feb 10, 2019 •

edited

Loading

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019 •

edited

Loading

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

zmanian commented Feb 10, 2019

jleni commented Feb 10, 2019 •

edited

Loading

jackzampolin left a comment

R4R: Fixes gaia/keybase storage of ledger accounts #3586

R4R: Fixes gaia/keybase storage of ledger accounts #3586

Conversation

jleni commented Feb 10, 2019 • edited Loading

alessio left a comment • edited Loading

Choose a reason for hiding this comment

adrianbrink commented Feb 10, 2019 • edited Loading

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019 • edited Loading

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

jleni commented Feb 10, 2019

jleni commented Feb 10, 2019

adrianbrink commented Feb 10, 2019

zmanian commented Feb 10, 2019

jleni commented Feb 10, 2019 • edited Loading

jackzampolin left a comment

Choose a reason for hiding this comment

jleni commented Feb 10, 2019 •

edited

Loading

alessio left a comment •

edited

Loading

adrianbrink commented Feb 10, 2019 •

edited

Loading

jleni commented Feb 10, 2019 •

edited

Loading

jleni commented Feb 10, 2019 •

edited

Loading