Skip to content

Conversation

@ByteZhang1024
Copy link
Contributor

@ByteZhang1024 ByteZhang1024 commented Oct 30, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Improved EVM typed data signing support for hardware devices.
    • Enhanced larger data handling for classic1s devices.
    • Added firmware version requirement (4.4.0+) for model_touch larger data support.
    • Increased data processing limit to 1536 bytes for compatible configurations.

@revan-zhang
Copy link
Contributor

revan-zhang commented Oct 30, 2025

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

Status Scanner Critical High Medium Low Total (0)
Licenses 0 0 0 0 0 issues
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

Walkthrough

The change modifies EVM typed data signing to support device-specific firmware version checks. Classic1s models now unconditionally support bigger data, while model_touch requires firmware version 4.4.0 or later. An unused import was removed.

Changes

Cohort / File(s) Summary
EVM Typed Data Signing
packages/core/src/api/evm/EVMSignTypedData.ts
Removed EDeviceType import. Updated hasBiggerData logic to check device type via getDeviceType(). Added device-specific firmware version conditions: classic1s supports bigger data unconditionally, model_touch requires firmware ≥ 4.4.0. Increased biggerLimit from 1024 to 1536.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify the device type detection logic is correct
  • Confirm firmware version comparison for model_touch (4.4.0 threshold)
  • Ensure biggerLimit increase aligns with device capabilities
  • Check that removed EDeviceType import is no longer used elsewhere in the file

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix: classic1s signTypedData bigger data" directly reflects the main change in the pull request. The changeset specifically modifies how bigger data is handled for classic1s devices in the signTypedData function, replacing a simple version check with a device-model-aware condition. The title uses clear, specific terminology that accurately conveys the fix's purpose without vague language or misleading claims.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/classicSignTypedData

📜 Recent 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 c489d0f and 3238168.

📒 Files selected for processing (1)
  • packages/core/src/api/evm/EVMSignTypedData.ts (2 hunks)
🔇 Additional comments (3)
packages/core/src/api/evm/EVMSignTypedData.ts (3)

4-4: LGTM! Unused import removed.

Cleanup of the unused EDeviceType import is correct.


343-346: Clarify firmware requirement for classic1s bigger data support.

The code grants bigger data to all classic1s devices regardless of firmware, but other features (like includeNode support and transaction signing) require classic1s to meet minimum versions (3.12.0 and 3.13.0 respectively). Confirm whether classic1s needs a firmware version check here too, or if bigger data support works on all classic1s firmware versions.


343-346: Verify whether model_mini and model_classic should support the 1.5k limit.

The code explicitly handles only model_classic1s and model_touch for the 1.5k data limit. The remaining two device models—model_mini and model_classic—silently fall through to the default 1k limit. No comments explain this choice. Confirm if this behavior is intentional or if these models should also support supportBiggerData logic.


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

@ByteZhang1024 ByteZhang1024 enabled auto-merge (squash) October 30, 2025 10:40
@wabicai wabicai self-requested a review November 10, 2025 07:02
@ByteZhang1024 ByteZhang1024 merged commit 34bf841 into onekey Nov 10, 2025
10 checks passed
@ByteZhang1024 ByteZhang1024 deleted the fix/classicSignTypedData branch November 10, 2025 07:02
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.

4 participants