feat: multi ledger support #375
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
📝 WalkthroughWalkthroughThe pull request refines ledger device handling by enhancing functions in the keyring and ledger packages. In the keyring package, the Changes
Sequence Diagram(s)sequenceDiagram
participant UA as User Application
participant KO as Keyring Options
participant LD as Ledger Device
UA->>KO: Call EthSecp256k1Option
KO->>KO: Call validateFlags (using viper config)
KO->>LD: Attempt to detect Ethereum ledger
alt Ethereum ledger found
LD-->>KO: Return Ethereum ledger details
KO->>KO: Process Ethereum ledger (validate coin & key type)
else Ethereum ledger not found
KO->>LD: Attempt to detect Cosmos ledger
LD-->>KO: Return Cosmos ledger details
KO->>KO: Set LedgerAppName to "Cosmos"
end
KO->>UA: Return appropriate public key message
sequenceDiagram
participant App as Application
participant FLE as FindLedgerEthereumApp
participant IL as InitiaLedger
App->>FLE: Invoke FindLedgerEthereumApp
FLE->>FLE: Check if initiaLedger is nil
alt Uninitialized
FLE->>IL: Create new InitiaLedger instance
end
FLE->>IL: Call connect()
alt Ethereum app offline
IL-->>FLE: Close wallet and return error
else Ethereum app online
IL-->>FLE: Return ledger.SECP256K1 instance
end
FLE->>App: Return ledger instance or error
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crypto/ledger/ledger.go (1)
58-66: Extract "Ethereum app offline" into a constant.
Instead of checking against a hard-coded string, define a constant to reduce the risk of typos and improve maintainability.crypto/keyring/options.go (2)
41-62: Switch to a structured logging approach.
Multiple multi-line messages usefmt.Printfandfmt.Println. Using a logger (or more structured output) may improve readability and maintainability, especially for debugging in production.
110-124: Clarify function parameter naming.
ParameterseCoinTypeandeKeyTypeare used for both Ethereum and Cosmos ledgers. Consider using more neutral names to avoid confusion, such asrequiredCoinTypeorexpectedKeyType.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (4)
crypto/keyring/options.go(2 hunks)crypto/ledger/ledger.go(2 hunks)crypto/ledger/ledger_mock.go(1 hunks)crypto/ledger/ledger_notavail.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
crypto/ledger/ledger_notavail.go (2)
crypto/ledger/ledger_mock.go (1) (1)
FindLedgerEthereumApp(11-13)crypto/ledger/ledger.go (1) (1)
FindLedgerEthereumApp(24-30)
crypto/ledger/ledger_mock.go (2)
crypto/ledger/ledger_notavail.go (1) (1)
FindLedgerEthereumApp(11-13)crypto/ledger/ledger.go (1) (1)
FindLedgerEthereumApp(24-30)
crypto/keyring/options.go (4)
crypto/ledger/ledger_mock.go (1) (1)
FindLedgerEthereumApp(11-13)crypto/ledger/ledger_notavail.go (1) (1)
FindLedgerEthereumApp(11-13)crypto/ledger/ledger.go (1) (1)
FindLedgerEthereumApp(24-30)crypto/hd/algorithm.go (1) (1)
EthSecp256k1Type(16-16)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
crypto/ledger/ledger_mock.go (1)
11-13: Mock ledger device approach looks good.
No issues found with returning nil and an error for unavailable ledger scenarios in a mock context.crypto/ledger/ledger_notavail.go (1)
11-13: Consistent message for unavailable ledger.
Returning nil and a not-available error cleanly conveys that ledger support is not compiled into this build.crypto/keyring/options.go (1)
37-105: Review the handling ofisCosmosLedger.
Once Ethereum ledger detection fails, this variable is flipped to true, causing all subsequent derivations to skip Ethereum checks. Consider re-initializing or scopingisCosmosLedgerinside the returned closure to support repeated attempts or device reconnection scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crypto/ledger/ledger.go (1)
22-22: Global variable concurrency caution
DeclaringinitiaLedgeras a global pointer could lead to race conditions if multiple goroutines invoke initialization concurrently. Consider guarding it with sync.Once or a similar pattern for thread safety.crypto/keyring/options.go (2)
38-95: Consolidated ledger derivation logic
Combining both Ethereum and Cosmos ledger flows here improves discoverability. However, there is a fair amount of duplicated structure and console output. Consider abstracting the shared checks and prints—potentially using a helper function to cut repetition and centralize error messaging.
97-105: Conditionally printing ledger type
Showing "Using Ethereum Pubkey" vs. "Using Cosmos Pubkey" can be helpful in development. In a production setting, you might want to move these statements to a debug log level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (4)
crypto/keyring/options.go(1 hunks)crypto/ledger/ledger.go(2 hunks)crypto/ledger/ledger_mock.go(1 hunks)crypto/ledger/ledger_notavail.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
crypto/ledger/ledger_mock.go (2)
crypto/ledger/ledger_notavail.go (1) (1)
FindLedgerEthereumApp(11-13)crypto/ledger/ledger.go (1) (1)
FindLedgerEthereumApp(24-30)
crypto/ledger/ledger_notavail.go (2)
crypto/ledger/ledger_mock.go (1) (1)
FindLedgerEthereumApp(11-13)crypto/ledger/ledger.go (1) (1)
FindLedgerEthereumApp(24-30)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
crypto/ledger/ledger_mock.go (1)
11-12: Consistent mock return for ledger unavailability
This function serves as a direct placeholder for ledger device usage, returning a fixed error to indicate that such support is unavailable. Maintaining the same error message across different mock or "not available" files keeps the behavior consistent.crypto/ledger/ledger_notavail.go (1)
11-12: Uniform "not available" approach
Returning anilinterface and a uniform error as in the mock ensures consistent handling across builds without ledger support.crypto/ledger/ledger.go (2)
24-30: Simplified approach to finding the Ledger
By assigning and returninginitiaLedger.connect(), the logic is streamlined. However, confirm that re-initialization or re-connection flows are handled properly if the device is disconnected or reconnected at runtime.Could you verify if repeated calls to
FindLedgerEthereumApp()handle a plugged/unplugged sequence gracefully?
58-66: Robust error handling for offline Ethereum app
Closing the wallet before returning an error prevents resource leaks. Consider adding structured logging or counters, so any offline occurrences can be easily found in monitoring.crypto/keyring/options.go (5)
4-5: Addition of thefmtimport
This import is used for console output. It’s acceptable given the new print statements below, but consider switching to a logging library for more consistent user messages.
9-10: Expanded imports for multi-ledger support
These imports facilitate both Ethereum- and Cosmos-specific ledger functions. Verify that you align versions across modules to avoid potential dependency conflicts.Would you like me to confirm version consistency in your go.mod?
Also applies to: 12-12, 13-13, 15-15, 19-20
27-27: Refined supported algorithms
By including EthSecp256k1 alongside Cosmos Secp256k1, you’ve properly enabled multi-ledger signing. This aligns with the expanded ledger functionalities.Also applies to: 31-31
106-106: Default ledger app setting
Defaulting to "Cosmos" is valid but might risk confusion if Ethereum is the more common scenario. Double-check that the user understands which ledger application is being used.
111-111: Centralized flag validation
Encapsulating validation of coin type and key type invalidateFlagsimproves maintainability. Ensure all relevant commands consistently call this function.Also applies to: 113-125
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crypto/ledger/ledger.go (1)
22-30:⚠️ Potential issueAddress the Concurrency Issue in Ledger Initialization
The function
FindLedgerEthereumApp()checks and initializesinitiaLedgerwithout any synchronization. This could lead to race conditions if multiple goroutines invoke the function concurrently.Use a synchronization mechanism such as
sync.Onceto ensure that the ledger is initialized only once:var initiaLedger *InitiaLedger +var ledgerOnce sync.Once func FindLedgerEthereumApp() (ledger.SECP256K1, error) { - if initiaLedger == nil { - initiaLedger = new(InitiaLedger) - } + ledgerOnce.Do(func() { + initiaLedger = new(InitiaLedger) + }) return initiaLedger.connect() }Don't forget to add
"sync"to your imports.
🧹 Nitpick comments (2)
crypto/ledger/ledger.go (1)
58-66: Consider using a constant for the Ethereum app status messageGood enhancement to error handling, but the string "Ethereum app offline" is used both for checking in the condition and for returning the error. This could cause issues if the message changes in the future.
Consider defining a constant:
+const EthereumAppOfflineStatus = "Ethereum app offline" // In the connect() method: // check if ethereum app is offline or not status, err := wallet.Status() if err != nil { wallet.Close() return nil, errors.Wrap(err, "failed to get wallet status") - } else if status == "Ethereum app offline" { + } else if status == EthereumAppOfflineStatus { wallet.Close() return nil, errors.New(status) }crypto/keyring/options.go (1)
38-96: Good implementation for multi-ledger support, consider extracting error messagesThe implementation correctly handles both Ethereum and Cosmos ledger devices with proper validation and fallback strategy. However, the error messages contain a lot of formatting and newlines which reduce code readability.
Consider extracting these error message templates to constants or helper functions:
+// Error message templates +const ( + ledgerValidationErrorTemplate = ` +Failed to validate flags for %s ledger: +%s + +Please make sure you have the correct coin type and key type set for the %s ledger. + +You can use the following command to set the correct flags: +--coin-type %d --key-type %s + +` +) +// formatLedgerValidationError creates a formatted error message for ledger validation failures +func formatLedgerValidationError(ledgerType string, err error, coinType uint32, keyType string) string { + return fmt.Sprintf(ledgerValidationErrorTemplate, ledgerType, err.Error(), ledgerType, coinType, keyType) +}Then use these in your validation checks:
// For Ethereum ledger validation if err := validateFlags(60, string(ethhd.EthSecp256k1Type)); err != nil { - fmt.Printf(` -Failed to validate flags for Ethereum ledger: -%s - -Please make sure you have the correct coin type and key type set for the Ethereum ledger. - -You can use the following command to set the correct flags: ---coin-type 60 --key-type %s - -`, err.Error(), ethhd.EthSecp256k1Type) + fmt.Printf(formatLedgerValidationError("Ethereum", err, 60, string(ethhd.EthSecp256k1Type))) return nil, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
crypto/keyring/options.go(1 hunks)crypto/ledger/ledger.go(2 hunks)
🔇 Additional comments (2)
crypto/keyring/options.go (2)
97-108: Good public key selection logic with clear user feedbackThe implementation correctly selects between Ethereum and Cosmos public keys based on the detected ledger type and provides helpful console output to indicate which one is being used.
111-125: Good flag validation implementationThe
validateFlagsfunction properly encapsulates the validation logic for coin and key types, improving code modularity and readability. The function correctly handles cases where flags are not set (empty string for keyType or 0 for coinType).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
==========================================
- Coverage 39.93% 39.83% -0.10%
==========================================
Files 277 277
Lines 26862 26923 +61
==========================================
Hits 10726 10726
- Misses 14478 14539 +61
Partials 1658 1658
🚀 New features to boost your workflow:
|
Description
Closes: #XXXX
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...
!in the type prefix if API or client breaking changeReviewers 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...