Skip to content

Conversation

ByteZhang1024
Copy link
Contributor

@ByteZhang1024 ByteZhang1024 commented Sep 30, 2025

Summary by CodeRabbit

  • New Features
    • Adds a capability check to detect EIP‑712 typed data signing support on compatible devices.
  • Bug Fixes
    • Corrects signing method selection on Mini devices to avoid unsupported EIP‑712 flows.
    • Ensures legacy signing is used when EIP‑712 support isn’t available, improving reliability.
    • Removes reliance on firmware version alone for enabling typed data signing, preventing misclassification of device capabilities.

Copy link
Contributor

coderabbitai bot commented Sep 30, 2025

Walkthrough

Adds a new capability check (supportEip712OnClassic) and updates EVMSignTypedData.run() control flow: Mini devices now follow the legacy signing path only when the new capability is not present. Removes the prior firmware/capability-based allowance for EthereumSignTypedData on Classic variants.

Changes

Cohort / File(s) Summary
EIP-712 capability gating and control flow
packages/core/src/api/evm/EVMSignTypedData.ts
Introduced supportEip712OnClassic flag. Adjusted Mini device branching to depend on this flag, gating legacy vs. typed-data signing. Removed older firmware/capability condition for Classic1s/ClassicPure. Kept validation and downstream signing unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App
  participant EVMSignTypedData
  participant Device

  App->>EVMSignTypedData: run(request)
  EVMSignTypedData->>Device: getCapabilities()
  Note over EVMSignTypedData: Determine device type and supportEip712OnClassic

  alt Device is Mini AND NOT supportEip712OnClassic
    EVMSignTypedData->>Device: legacySign(request)
    Device-->>EVMSignTypedData: legacy signature
  else Device supports EIP-712 on Classic or not Mini
    EVMSignTypedData->>Device: signTypedData(request)
    Device-->>EVMSignTypedData: EIP-712 signature
  end

  EVMSignTypedData-->>App: result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “fix: eip 712” directly points to the core change of correcting EIP-712 handling, making it concise, targeted, and reflective of the pull request’s main focus on updating the EIP-712 signing logic.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/1sEip712

🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

@ByteZhang1024 ByteZhang1024 enabled auto-merge (squash) September 30, 2025 08:56
@revan-zhang
Copy link
Contributor

revan-zhang commented Sep 30, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 245ea17 and 4a9f3af.

📒 Files selected for processing (1)
  • packages/core/src/api/evm/EVMSignTypedData.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build (22)
  • GitHub Check: lint (22)
  • GitHub Check: Analyze (javascript-typescript)

@wabicai wabicai self-requested a review October 9, 2025 08:43
@ByteZhang1024 ByteZhang1024 merged commit fb00650 into onekey Oct 9, 2025
10 checks passed
@ByteZhang1024 ByteZhang1024 deleted the fix/1sEip712 branch October 9, 2025 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants