fix: ledger discover

[amritkumarj]

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests
• Added new benchmarks to generated graphs, if any. More info here.

This PR fixes ledger integration with gno (fixes #1119 )

Create ledger account

Sign and broadcast

[thehowl]

Hey Amrit, thank you for your contribution! 🎉

The core team is officially back from tomorrow. Somebody should hopefully pick it up for review next week. Appreciate your work!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b619351) 55.80% compared to head (f356de0) 55.80%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1502   +/-   ##
=======================================
  Coverage   55.80%   55.80%           
=======================================
  Files         436      436           
  Lines       66168    66168           
=======================================
  Hits        36922    36922           
  Misses      26356    26356           
  Partials     2890     2890           

Flag	Coverage Δ
go-1.21.x	∅ <ø> (∅)
misc	∅ <ø> (∅)
misc-_test.genstd	∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zivkovicmilos]

Thank you for the contribution, and sorry for the late review 🙏

I've tested it locally with my Ledger X, works great 🎉

A couple of things:

• What do you think about adding a comment somewhere, or a README on how people would connect their ledger devices to Gnokey? For example, I had to install the Cosmos app on the Ledger device before gnokey worked.

Can you please check why the CI / some tests are failing? 🙏
I see that you need to tidy go mods in subfolders.

Overall works fine, I've left a minor nitpick comment

[zivkovicmilos]

@amritkumarj

Please check the failing tests 🙏

Seems like the test TestCreateLedger is failing

[thehowl]

Hey @amritkumarj, if you can apply the test you can fix the tests and we can merge it. Thanks!

commit 5edcbc608879dd865b0b58da56275b8f1ca6e9a2
Author: Morgan Bazalgette <[email protected]>
Date:   Thu Jan 25 18:55:57 2024 +0100

    update error value

diff --git a/tm2/pkg/crypto/keys/keybase_test.go b/tm2/pkg/crypto/keys/keybase_test.go
index d7660ac3..5ee88f16 100644
--- a/tm2/pkg/crypto/keys/keybase_test.go
+++ b/tm2/pkg/crypto/keys/keybase_test.go
@@ -45,7 +45,7 @@ func TestCreateLedger(t *testing.T) {
 	ledger, err := kb.CreateLedger("some_account", Secp256k1, "cosmos", 3, 1)
 	if err != nil {
 		assert.Error(t, err)
-		assert.Equal(t, "no Ledger discovery function defined", err.Error())
+		assert.Contains(t, "LedgerHID device (idx 0) not found.", err.Error())
 		assert.Nil(t, ledger)
 		t.Skip("ledger nano S: support for ledger devices is not available in this executable")
 		return
diff --git a/tm2/pkg/crypto/ledger/ledger_secp256k1.go b/tm2/pkg/crypto/ledger/ledger_secp256k1.go
index d6560977..f154dbf3 100644
--- a/tm2/pkg/crypto/ledger/ledger_secp256k1.go
+++ b/tm2/pkg/crypto/ledger/ledger_secp256k1.go
@@ -17,9 +17,6 @@ import (
 	"github.com/gnolang/gno/tm2/pkg/errors"
 )
 
-// discoverLedger defines a function to be invoked at runtime for discovering
-// a connected Ledger device.
-
 type (
 	// discoverLedgerFn defines a Ledger discovery function that returns a
 	// connected device or an error upon failure. Its allows a method to avoid CGO
@@ -48,6 +45,8 @@ type (
 	}
 )
 
+// discoverLedger defines a function to be invoked at runtime for discovering
+// a connected Ledger device.
 var discoverLedger discoverLedgerFn = func() (LedgerSECP256K1, error) {
 	device, err := ledger.FindLedgerCosmosUserApp()
 	if err != nil {

[kristovatlas]

Although merged, a light security review is in progress and I'll follow up with a comment when completed.

[kristovatlas]

Thank you for the contribution, this LGTM. I'm excited to be able to use my Ledger with the testnet tokens and gnokey now.

I also tested this on my Ledger Nano S in concert with the Cosmos Ledger app. Some notes on what I reviewed:

• Imports in github.com/cosmos/ledger-cosmos-go 0.13.3
  • 8 stars on GitHub
  • 138-commit history starting 6 years ago, with most recent commit 3 months ago
    11 contributors including @jleni, @tac0turtle, and @​​julienrbrt who are members of the COSMOS org on GitHub
  • The repo does not have a security policy, any published advisories, or indications of security audits. I recommend that we flag ledger-cosmos-go for a security audit in the future.
  • 0.13.3 is the most recent tag
• Sub-dependency on a couple repos from Zondax, which is a GitHub-verified Swiss blockchain service provider
• I had an inline question about assumptions concerning the length fields for the s and r values when parsing signatures. This was essentially resolved by Morgan.
• Out of scope for this review:
  • Does the ecdsa.ParseDERSignature handle all edge cases and potential vulnerabilities parsing signatures?
  • Are there any non-obvious issues with big.Int parsing r and s?
  • Are any side channel attacks introduced by failing to work in constant time?
  • The code uses secp.ModNScalar to handle the scalar s. Are the IsOverHalfOrder check and the subsequent adjustments implemented correctly to prevent potential vulnerabilities related to scalar manipulation?

[kristovatlas]

One other callout, since convertDERtoBER gets into some more complex bit arithmetic I would like to see unit tests added for this function to ensure that it works as intended. But it looks correct at first glance.