feat: ethereum ledger integration #365
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe changes update cryptographic signing and verification to incorporate EIP-191 message formatting. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PrivKey
participant Formatter
Client->>PrivKey: Sign(msg)
PrivKey->>Formatter: Check for EIP-191 prefix
Formatter-->>PrivKey: Return formatted msg
PrivKey-->>Client: Return signature
sequenceDiagram
participant User
participant LedgerAPI
participant Wallet
User->>LedgerAPI: LedgerDerivationFn()
LedgerAPI->>Wallet: connect()
Wallet-->>LedgerAPI: Provide wallet instance
User->>LedgerAPI: GetAddressPubKeySECP256K1(hdPath)
LedgerAPI->>Wallet: Derive public key & address
Wallet-->>LedgerAPI: Return credentials
LedgerAPI-->>User: Return pubkey and address
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 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 (
|
Dependency ReviewThe following issues were found:
Vulnerabilitiesgo.mod
Only included vulnerabilities with severity moderate or higher. License Issuesgo.mod
Denied Licenses: GPL-1.0-or-later, LGPL-2.0-or-later OpenSSF ScorecardScorecard details
Scanned Files
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
tx/eip191.go (1)
45-50: Validate message length handling within EIP-191 formatting.
Appending the decimal size of the message is correct for EIP-191, but consider edge cases with very large messages or handling empty messages.crypto/ethsecp256k1/ethsecp256k1.go (1)
135-152: Guard against double EIP-191 prefixing.
The logic checks if the message is prefixed; however, if a message were partially prefixed, it could slip through. Consider a stricter check to avoid accidentally reformatting.-if !strings.HasPrefix(string(msg), tx.EIP191MessagePrefix) { +if !strings.HasPrefix(string(msg), tx.EIP191MessagePrefix) && 8000 + !strings.Contains(string(msg), tx.EIP191MessagePrefix) { msg = tx.FormatEIP191Message(msg) }crypto/keyring/ledger.go (6)
1-16: Check consistent error strategy across the codebase.
Here,"github.com/pkg/errors"is used, while other parts rely onerrorsmod. Adopting one error approach fosters clarity.
27-30: Struct naming clarity.
InitiaLedgeris straightforward, but if future expansions involve multiple ledger types, consider consistent naming (e.g.,LedgerIntegration).
32-57: Allow selection of specific wallet if multiple are detected.
Currently, the implementation grabs the first wallet (wallets[0]). Offer a user option or config for selection if more are present.
59-62: Close might be called multiple times inadvertently.
Given user flows, consider a no-op check or re-entrancy guard if the wallet is already closed.
64-89: DRY up repeated wallet-opening code.
wallet.Open("")appears often. You could unify it into a helper method to ensure all open logic is consistent.
113-136: Remove debug prints for production code.
Printing signature length and hex is useful in development but can clutter logs. Remove or guard them behind debug flags.- fmt.Println("signature", len(sig)) - fmt.Println("signature", hex.EncodeToString(sig))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (4)
crypto/ethsecp256k1/ethsecp256k1.go(3 hunks)crypto/keyring/ledger.go(1 hunks)crypto/keyring/options.go(1 hunks)tx/eip191.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run test and upload codecov
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
crypto/keyring/options.go (1)
31-31: Consider verifying error-handling flows once Ledger derivation is invoked.
Assigningoptions.LedgerDerivation = LedgerDerivationFn()is fine, but ensure consumers effectively handle the returned error and thoroughly test ledger-connection failures.tx/eip191.go (1)
42-43: Keep consistent EIP-191 formatting across all signers.
ReturningFormatEIP191Message(aminoJSONBz)ensures uniform message formatting. Confirm that all signing contexts are updated to align with this approach (especially in tests).crypto/ethsecp256k1/ethsecp256k1.go (1)
274-278: Maintain consistency in verification logic.
Adding the prefix before verifying is aligned with the signing process. Confirm that corner cases (e.g., messages already prefixed) are handled gracefully.crypto/keyring/ledger.go (1)
17-25: Consider concurrency in the closure-based derivation approach.
WrappingInitiaLedgerin a closure is an elegant solution, but ensure it’s thread-safe if used in parallel contexts.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #365 +/- ##
==========================================
- Coverage 41.15% 41.03% -0.12%
==========================================
Files 269 270 +1
Lines 25812 25914 +102
==========================================
+ Hits 10622 10634 +12
- Misses 13534 13624 +90
Partials 1656 1656
🚀 New features to boost your workflow:
|
11 tasks
This was referenced Mar 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This requires a chain upgrade, so let's do merge this later.
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...