Review, and followup on ledger integration #176
Conversation
This PR adds the ledger integration as a backend to the KMS. There is still more work required to ensure that the Ledger application knows how to correctly decode/encode Tendermint votes.
+ Adding a basic command line
Refactoring and adjusting to new ledger-tm library
Upgrading crates + cargo fmt fixes
Disabling ledgertm tests until a ledgermock is available
in tmkms.toml.example to clarify why no other options are provided
This reverts commit 562109d Code does not compile, is not properly formatted and this is a different concern.
- ledger sub-commands do not do anything
- ledger sub-commands do not do anything - fix ledgertm::init (config -> ledgertm_configs) - fmt
|
@liamsi Looks good to me. We can add additional features after this PR. |
|
Thanks for testing this locally and thanks for the review! |
| } | ||
| let provider = Box::new(Ed25519LedgerTmAppSigner::connect()?); | ||
| let pk = provider.public_key()?; | ||
| let signer = Signer::new(LEDGER_TM_PROVIDER_LABEL, LEDGER_TM_ID.to_string(), provider); |
There was a problem hiding this comment.
@jleni Will there be an equivalent to a key_id we can put into the config? e.g. for yubihsm we can have sth like keys = [{ id = "gaia-9000", key = 1 }]. I guess this makes sense here, too?
I'll add a TODO here and merge for now.
There was a problem hiding this comment.
The validator app needs to rely on Ledger's crypto API for Ed25519 so in practice there is a Bip32 derivation path. At the moment, this is not exposed in the API but it would be actually possible to have different keys to allow for something like that. Actually, it could be even possible to have both secp256k1 and ed25519 in the same device.
Should we open an issue for this new feature?
There was a problem hiding this comment.
Yes, please that would be awesome! Thanks :)
There was a problem hiding this comment.
Created issues for this:
#177
cosmos/ledger-cosmos#108
Supersedes #172
see #172 (comment)
cc @jleni @adrianbrink