fix: sigverify to attach eip191 prefix, only it is legacy amino json mode #377
Conversation
📝 WalkthroughWalkthroughThis pull request simplifies the signature verification process for Ethereum public keys by removing specific logic related to the "interwoven-1" chain and the EIP-191 prefix handling. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant TX as Transaction
participant VX as Verify.go
participant PK as PubKey (crypto)
TX->>VX: Request signature verification
VX->>PK: Call VerifySignature (without EIP-191 prefix)
PK-->>VX: Return verification result
VX-->>TX: Return result to caller
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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crypto/ledger/ledger.go (1)
145-161: Consider limiting or securing the log output.Pretty-printing the entire signing bytes may expose sensitive or private content (e.g., transaction details or addresses). Consider only logging when in debug mode or sanitizing the output to avoid leaking sensitive information into logs.
cmd/initiad/root.go (1)
125-134: Clarify the keyring override logic.Overriding an already-initialized
initClientCtx.Keyringmay lead to confusion about which keyring is actually in use. Ensure that users and documentation are aware of this override, especially ifinitClientCtx.Keyringwas previously modified elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
app/ante/sigverify/verify.go(0 hunks)cmd/initiad/root.go(2 hunks)crypto/ethsecp256k1/ethsecp256k1.go(1 hunks)crypto/keyring/keyring.go(1 hunks)crypto/ledger/ledger.go(2 hunks)tx/eip191.go(2 hunks)
💤 Files with no reviewable changes (1)
- app/ante/sigverify/verify.go
🚧 Files skipped from review as they are similar to previous changes (1)
- crypto/ethsecp256k1/ethsecp256k1.go
🧰 Additional context used
🧬 Code Definitions (1)
crypto/keyring/keyring.go (3)
crypto/ethsecp256k1/ethsecp256k1.go (15)
pubKey(173-185)pubKey(188-193)pubKey(196-198)pubKey(201-203)pubKey(206-208)pubKey(211-213)pubKey(216-223)pubKey(226-230)pubKey(233-235)pubKey(241-243)pubKey(246-267)_(48-48)_(49-49)_(151-151)_(152-152)tx/eip191.go (2)
_(27-27)RemoveEIP191Prefix(55-57)crypto/ledger/ledger.go (1)
_(23-23)
🔇 Additional comments (8)
cmd/initiad/root.go (1)
93-94: Protobuf support for Ledger is enabled.Attaching
.WithLedgerHasProtobuf(true)is beneficial if your code requires signing or parsing Protobuf transactions directly on a Ledger device. This looks good.crypto/keyring/keyring.go (7)
1-2: New package definition.No issues here; the package declaration aligns with Go best practices.
3-17: Imports look appropriate.All imports, including “errors” and “github.com/cosmos/cosmos-sdk/...”, are necessary and consistent with usage throughout the file.
19-21: Struct embedding of Keyring.Embedding
cosmoskeyring.Keyringas a field is a clean way to extend functionality.
23-30: Keyring construction is straightforward.The
NewKeyringfunction properly initializes and returns a wrappedKeyring.
32-44: Ledger vs. non-ledger signing logic looks correct.Delegating to
SignWithLedgerwhen the record includesGetLedger()is clear, and otherwise falling back to the base Keyring is appropriate.
46-49: Error variables.These specialized error variables clarify user intent for sign mode errors. No concerns here.
51-100:❓ Verification inconclusive
Request verification of sign message matching.
This method removes the EIP-191 prefix before calling the ledger’s signing function, then verifies the signature using the original
msg. To confirm correctness, ensure that the Ledger re-adds the prefix before signing internally so that verification with the originalmsgremains valid.Below is a script to locate references to
SignLedgerAminoJSONcalls and reveal how the prefix is handled:
🏁 Script executed:
#!/bin/bash rg -A 5 "SignLedgerAminoJSON"Length of output: 322
Action Required: Confirm Internal EIP-191 Prefix Re-Addition in Ledger Signing
In the
SignWithLedgermethod incrypto/keyring/keyring.go(lines 51–100), when using an Ethereum public key the code explicitly removes the EIP-191 prefix before callingSignLedgerAminoJSON, while the signature is later verified against the original message (which includes the prefix). Please verify that the ledger’s implementation ofSignLedgerAminoJSONinternally re-adds the EIP-191 prefix before signing. If this behavior is confirmed—via documentation or tests—it would be helpful to add a clarifying comment or reference in the code to make this dependency explicit.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #377 +/- ##
==========================================
- Coverage 39.83% 39.72% -0.12%
==========================================
Files 277 278 +1
Lines 26923 26968 +45
==========================================
- Hits 10726 10713 -13
- Misses 14539 14598 +59
+ Partials 1658 1657 -1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crypto/keyring/keyring.go (3)
10-11: Redundant imports for crypto/types packageThere are duplicate imports for the same package:
"github.com/cosmos/cosmos-sdk/crypto/types" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"Choose one consistent import style to improve code clarity:
-"github.com/cosmos/cosmos-sdk/crypto/types" -cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" +cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
57-61: Use defined error variables for consistencyError messages are currently hardcoded inline. For better error handling and consistency, define and use error variables similar to how other errors are handled in the codebase.
- if isEthPubKey && signMode != signing.SignMode_SIGN_MODE_EIP_191 { - return nil, nil, errors.New("must use --sign-mode=eip-191 for Ethereum Ledger") - } else if !isEthPubKey && signMode != signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON { - return nil, nil, errors.New("must use --sign-mode=amino-json for Cosmos Ledger") + if isEthPubKey && signMode != signing.SignMode_SIGN_MODE_EIP_191 { + return nil, nil, ErrInvalidSignModeEthereum + } else if !isEthPubKey && signMode != signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON { + return nil, nil, ErrInvalidSignModeCosmosAdd these error variables at the package level:
var ErrInvalidSignModeEthereum = errors.New("must use --sign-mode=eip-191 for Ethereum Ledger") var ErrInvalidSignModeCosmos = errors.New("must use --sign-mode=amino-json for Cosmos Ledger")
78-79: Explicit error return value neededThe function returns without explicitly setting an error value, which could lead to confusion. Always explicitly return the error value:
- if err != nil { - return + if err != nil { + return nil, nil, err
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
crypto/ethsecp256k1/ethsecp256k1.go(1 hunks)crypto/keyring/keyring.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crypto/ethsecp256k1/ethsecp256k1.go
🧰 Additional context used
🧬 Code Definitions (1)
crypto/keyring/keyring.go (3)
crypto/ethsecp256k1/ethsecp256k1.go (15)
pubKey(173-185)pubKey(188-193)pubKey(196-198)pubKey(201-203)pubKey(206-208)pubKey(211-213)pubKey(216-223)pubKey(226-230)pubKey(233-235)pubKey(241-243)pubKey(246-267)_(48-48)_(49-49)_(151-151)_(152-152)tx/eip191.go (2)
_(27-27)RemoveEIP191Prefix(55-62)crypto/ledger/ledger.go (1)
_(23-23)
🔇 Additional comments (3)
crypto/keyring/keyring.go (3)
64-68: LGTM: Correct handling of EIP191 prefix for Ethereum keysThe implementation correctly removes the EIP191 prefix from the message before signing with a Ledger device for Ethereum keys. This aligns with the PR objective to "fix: sigverify to attach eip191 prefix, only it is legacy amino json mode".
90-92: LGTM: Proper signature verificationGood practice to verify the signature against the original message before returning it.
32-44: LGTM: Well-designed Sign methodThe Sign method correctly handles different key types, with a clear separation between Ledger and non-Ledger keys. This design makes the code more maintainable and allows for specialized handling where needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crypto/keyring/keyring.go (1)
46-96: Consider adding unit tests for the SignWithLedger function.The
SignWithLedgerfunction is central to the PR's objective and contains important logic for EIP-191 handling. Consider adding comprehensive unit tests to verify:
- Correct handling of different key types
- Proper validation of signing modes
- EIP-191 prefix removal for Ethereum keys
- Error cases for key mismatches and invalid signatures
Tests would help ensure this critical functionality remains correct through future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (1)
crypto/keyring/keyring.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crypto/keyring/keyring.go (3)
crypto/ethsecp256k1/ethsecp256k1.go (15)
pubKey(173-185)pubKey(188-193)pubKey(196-198)pubKey(201-203)pubKey(206-208)pubKey(211-213)pubKey(216-223)pubKey(226-230)pubKey(233-235)pubKey(241-243)pubKey(246-267)_(48-48)_(49-49)_(151-151)_(152-152)tx/eip191.go (2)
_(27-27)RemoveEIP191Prefix(55-62)crypto/ledger/ledger.go (1)
_(23-23)
🔇 Additional comments (7)
crypto/keyring/keyring.go (7)
19-21: Well-designed extension of the Cosmos keyring structure.The
Keyringstruct extendscosmoskeyring.Keyringthrough embedding, which is a clean way to add custom functionality while maintaining API compatibility.
23-30: LGTM - Good error handling in the constructor.The
NewKeyringfunction properly initializes the keyring with context parameters and returns appropriate errors if initialization fails.
32-44: Clear separation of signing logic.The
Signmethod nicely separates the signing logic between Ledger and non-Ledger keys, delegating to the appropriate implementation. This clean separation improves maintainability.
55-61: Validation of signing modes is accurate.Correct validation enforcing EIP-191 for Ethereum keys and LEGACY_AMINO_JSON for Cosmos keys, which aligns with the PR objective to ensure proper EIP-191 handling.
63-69: EIP-191 prefix handling addresses the PR objective.This segment properly removes the EIP-191 prefix for Ethereum keys to prevent double-prefixing, which is exactly what the PR aims to fix. The code calls
tx.RemoveEIP191Prefixto handle the prefix removal.The comment documentation clearly explains the reasoning: the Ledger device automatically adds the prefix, so removing it here prevents invalid signatures from double-prefixing.
76-84: Strong public key validation ensures security.The code securely verifies that the public key from the keyring matches the one from the Ledger device, preventing potential key mismatches. The error message is informative and includes both keys for comparison.
91-93: Important signature verification step.The signature verification step is crucial for ensuring the integrity of the signing process. This extra validation layer prevents returning invalid signatures.
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...