feat: add customized Ledger support by austinchandra · Pull Request #12935 · cosmos/cosmos-sdk

austinchandra · 2022-08-16T16:13:43Z

Description

Increase customized signing capabilities for Ledger by:

Adding the capability to use different Ledger apps and key types

By increasing the degree to which higher-level chains can customize Ledger usage and signing, the Cosmos SDK can become better-suited to serve chains that differ from the default; this can include EVM-based chains that do not use the Cosmos Ledger app, or Cosmos-based chains with their own Ledger implementation.

PR

This PR introduces the ability to customize Ledger support for apps with secp256k1 ECDSA keys, such as Ethereum and Cosmos. Key changes and relevant files:

Add a new Options type within Ledger to track customizations
- Add various Ledger options to the Keyring options, where they can be passed in from higher-level chains
- Update the Ledger options using the Keyring options via setter functions
- Update the Ledger implementation to use these options
Add LedgerHasProtobuf field to Context to indicate whether the Ledger instance can support Protobuf SignDocs

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

alexanderbez

utACK

I left a few minor comments and we also need a changelog entry for this as well as a note in the upgrading doc IMO

alexanderbez · 2022-08-17T13:29:00Z

crypto/keyring/keyring.go

Can't ledger derive keys that aren't on the SECP curve?

Currently, Ledger devices are represented using the SECP256k1 interface. This PR intended only to generalize Ledger support within the realm of secp256k1 keys, but it's possible that generalizing it even further might be more appropriate. Do you think that would be the case?

alexanderbez · 2022-08-17T13:30:26Z

crypto/ledger/ledger_secp256k1.go

Suggested change

} else {

return convertDERtoBER(sig)

}

}

return convertDERtoBER(sig)

alexanderbez · 2022-08-17T13:30:58Z

crypto/ledger/ledger_secp256k1.go

is there a way to not use a private global? Or is this inherent to how the ledger is setup and used?

The implementation of the Ledger functions depends on the options provided, so there must be some form of local state. Let me know if there is a way to improve it, I am open to any ideas.

Could we use dependency injection here @kocubinski?

alexanderbez · 2022-08-17T13:31:25Z

crypto/ledger/ledger_real.go

There's two code blocks that do the exact same thing -- I would move this setup into a helper function.

alexanderbez

utACK

alexanderbez · 2022-08-21T03:35:09Z

@austinchandra could you allow access for us to update your branch or just rebase main yourself prior to merge pls?

austinchandra · 2022-08-22T18:27:47Z

@alexanderbez Rebased and added CHANGELOG and UPGRADING entries

alexanderbez · 2022-08-22T19:40:40Z

@austinchandra could you quickly address the linting errors and we'll merge 👍

austinchandra · 2022-08-22T21:58:54Z

@alexanderbez Fixed and updated

alexanderbez · 2022-08-23T14:01:12Z

@austinchandra can you allow access to update your branches for us? That way we can rebase main without you having to do it each time. Otherwise, you can update it.

austinchandra · 2022-08-23T15:54:06Z

@alexanderbez Rebased and granted permissions

codecov · 2022-08-24T12:51:57Z

Codecov Report

Merging #12935 (9d6791f) into main (f9e8a04) will decrease coverage by 0.30%.
The diff coverage is 30.00%.

❗ Current head 9d6791f differs from pull request most recent head 412cdcb. Consider uploading reports for the commit 412cdcb to get more accurate results

@@            Coverage Diff             @@
##             main   #12935      +/-   ##
==========================================
- Coverage   54.02%   53.71%   -0.31%     
==========================================
  Files         645      645              
  Lines       55238    55271      +33     
==========================================
- Hits        29841    29690     -151     
- Misses      23019    23198     +179     
- Partials     2378     2383       +5

Impacted Files	Coverage Δ
client/cmd.go	`57.73% <0.00%> (ø)`
client/context.go	`54.49% <0.00%> (-0.88%)`	⬇️
crypto/keyring/keyring.go	`60.72% <0.00%> (-1.23%)`	⬇️
crypto/ledger/ledger_secp256k1.go	`50.96% <45.45%> (-1.94%)`	⬇️
crypto/ledger/ledger_mock.go	`58.46% <100.00%> (+0.64%)`	⬆️
x/distribution/simulation/decoder.go	`0.00% <0.00%> (-100.00%)`	⬇️
x/distribution/simulation/operations.go	`0.00% <0.00%> (-80.65%)`	⬇️
x/group/keeper/keeper.go	`56.64% <0.00%> (+0.39%)`	⬆️
crypto/keys/internal/ecdsa/privkey.go	`83.01% <0.00%> (+1.88%)`	⬆️
x/simulation/operation.go	`13.04% <0.00%> (+13.04%)`	⬆️
... and 1 more

…s/general-ledger-support

CHANGELOG.md

amaury1093 · 2022-09-01T14:25:17Z

client/context.go


+// WithLedgerHasProto returns the context with the provided boolean value, indicating
+// whether the target Ledger application can support Protobuf payloads.
+func (ctx Context) WithLedgerHasProtobuf(val bool) Context {


@austinchandra Just to understand the context of this PR: Is Evmos creating a custom ledger app just for Evmos?

The SDK team is working on SIGN_MODE_TEXTUAL, which improves ledger signing in general, and comes with a Cosmos-wide Ledger app. https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-050-sign-mode-textual.md

Evmos plans to create a custom Ledger app in the future, which will probably attempt to integrate some variant of SIGN_MODE_TEXTUAL. For now, we are using the Ethereum app to sign Cosmos payloads using EIP-712 Typed Messages, which this PR helps to enable.

…s/general-ledger-support

austinchandra requested a review from a team August 16, 2022 16:13

github-actions bot added the C:CLI label Aug 16, 2022

austinchandra changed the title ~~feat: increase customized signing capabilities~~ feat: add customized Ledger support Aug 16, 2022

alexanderbez suggested changes Aug 17, 2022

View reviewed changes

alexanderbez approved these changes Aug 21, 2022

View reviewed changes

austinchandra force-pushed the evmos/general-ledger-support branch from dbcf92a to 37ae302 Compare August 22, 2022 18:17

austinchandra force-pushed the evmos/general-ledger-support branch from 2b8dd66 to 03acc49 Compare August 22, 2022 21:48

austinchandra force-pushed the evmos/general-ledger-support branch from 03acc49 to d8ef53d Compare August 22, 2022 21:59

alexanderbez added the A:automerge label Aug 23, 2022

austinchandra force-pushed the evmos/general-ledger-support branch from d8ef53d to 6d61f0a Compare August 23, 2022 15:53

austinchandra added 7 commits August 23, 2022 11:02

Add general Ledger options to customize signing and behavior

4d28f24

Minor code cleanup

9ebdbc7

Revert reformatting changes to isolate Ledger functionality

f147f07

Update Ledger test initializations

158e14b

Code cleanup and refactor

1da973d

Add changelog and upgrading entries

820e738

Update ledger_notavail.go to address lint

5b385e2

austinchandra force-pushed the evmos/general-ledger-support branch from 6d61f0a to 5b385e2 Compare August 23, 2022 18:02

amaury1093 self-assigned this Aug 24, 2022

alexanderbez and others added 2 commits August 24, 2022 17:07

Merge branch 'main' into evmos/general-ledger-support

6b9221f

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into evmo…

0f3dc68

…s/general-ledger-support

JeancarloBarrios self-assigned this Aug 29, 2022

amaury1093 reviewed Sep 1, 2022

View reviewed changes

austinchandra and others added 4 commits September 1, 2022 09:25

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into evmo…

d171440

…s/general-ledger-support

Add formatted changelog entry

0b4fc3c

Merge branch 'main' into evmos/general-ledger-support

9d6791f

Merge branch 'main' into evmos/general-ledger-support

412cdcb

alexanderbez enabled auto-merge (squash) September 7, 2022 02:43

alexanderbez merged commit 4a07259 into cosmos:main Sep 7, 2022

-	} else {
-		return convertDERtoBER(sig)
-	}
+	}
+	return convertDERtoBER(sig)

Conversation

austinchandra commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR

Author Checklist

Reviewers Checklist

Uh oh!

alexanderbez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderbez left a comment

Choose a reason for hiding this comment

Uh oh!

alexanderbez commented Aug 21, 2022

Uh oh!

austinchandra commented Aug 22, 2022

Uh oh!

alexanderbez commented Aug 22, 2022

Uh oh!

austinchandra commented Aug 22, 2022

Uh oh!

alexanderbez commented Aug 23, 2022

Uh oh!

austinchandra commented Aug 23, 2022

Uh oh!

codecov bot commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

austinchandra commented Aug 16, 2022 •

edited

Loading

codecov bot commented Aug 24, 2022 •

edited

Loading