[c-881][Ledger] add support for multisig with eip712 signautures#82
[c-881][Ledger] add support for multisig with eip712 signautures#82dbrajovic wants to merge 5 commits intov0.50.x-injfrom
Conversation
github-actions
bot
added
C:x/feegrant
C:x/tx
C:x/circuit
C:x/evidence
C:x/nft
C:x/upgrade
labels
|
@dbrajovic your pull request is missing a changelog! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBumps Go toolchain to 1.23.9 and updates numerous dependencies across multiple module go.mod files; adds HandlerMap.AddSignModeHandler for dynamic sign-mode registration; and adds EIP712_V2 sign-mode mappings in both API↔internal translation functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/tx/signing/handler_map.go`:
- Around line 62-64: The AddSignModeHandler method on HandlerMap mutates shared
state unsafely; add a sync.RWMutex field to HandlerMap and protect writes in
AddSignModeHandler (use Lock/Unlock) and reads in GetSignBytes() and
SupportedModes() (use RLock/RUnlock), change AddSignModeHandler to validate
inputs (non-nil handler, matching SignMode type), detect and return an error on
duplicate registration instead of panicking, and update callers/tests to handle
the error; leave NewHandlerMap initialization logic intact to register handlers
at startup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 269f5aeb-afa7-4510-bd29-346ea5bfc759
⛔ Files ignored due to path filters (5)
x/circuit/go.sumis excluded by!**/*.sumx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/nft/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
x/circuit/go.modx/evidence/go.modx/feegrant/go.modx/nft/go.modx/tx/signing/handler_map.gox/upgrade/go.mod
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/auth/signing/verify.go (1)
28-42:⚠️ Potential issue | 🟠 MajorMissing symmetric case in
APISignModeToInternalfor EIP712_V2.The new
SignMode_SIGN_MODE_EIP712_V2was added tointernalSignModeToAPIbut the corresponding case is missing fromAPISignModeToInternal. This asymmetry will cause incoming transactions using EIP712_V2 to fail with "unsupported sign mode" when converting from the protobuf API representation to the internal SDK type.🐛 Proposed fix to add symmetric conversion
func APISignModeToInternal(mode signingv1beta1.SignMode) (signing.SignMode, error) { switch mode { case signingv1beta1.SignMode_SIGN_MODE_DIRECT: return signing.SignMode_SIGN_MODE_DIRECT, nil case signingv1beta1.SignMode_SIGN_MODE_LEGACY_AMINO_JSON: return signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, nil case signingv1beta1.SignMode_SIGN_MODE_TEXTUAL: return signing.SignMode_SIGN_MODE_TEXTUAL, nil case signingv1beta1.SignMode_SIGN_MODE_DIRECT_AUX: return signing.SignMode_SIGN_MODE_DIRECT_AUX, nil + case signingv1beta1.SignMode_SIGN_MODE_EIP712_V2: + return signing.SignMode_SIGN_MODE_EIP712_V2, nil default: return signing.SignMode_SIGN_MODE_UNSPECIFIED, fmt.Errorf("unsupported sign mode %s", mode) } }Also applies to: 55-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/auth/signing/verify.go` around lines 28 - 42, APISignModeToInternal is missing a case for signingv1beta1.SignMode_SIGN_MODE_EIP712_V2, causing conversion failures for EIP712_V2; add a symmetric case in APISignModeToInternal that returns signing.SignMode_SIGN_MODE_EIP712_V2 (mirroring internalSignModeToAPI), so the function handles the new sign mode and avoids the "unsupported sign mode" error when converting from the protobuf API representation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@x/auth/signing/verify.go`:
- Around line 28-42: APISignModeToInternal is missing a case for
signingv1beta1.SignMode_SIGN_MODE_EIP712_V2, causing conversion failures for
EIP712_V2; add a symmetric case in APISignModeToInternal that returns
signing.SignMode_SIGN_MODE_EIP712_V2 (mirroring internalSignModeToAPI), so the
function handles the new sign mode and avoids the "unsupported sign mode" error
when converting from the protobuf API representation.
1 participant
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Chores