-
Notifications
You must be signed in to change notification settings - Fork 9
feat(legacy): add EVM chain PHAROS support #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1. fix the tron memory related issue 2. fix the issue when the client first connect to hardware after creating a new wallet
WalkthroughThis release adds Pharos (PROS) network support with chain ID 1672, increases Tron message buffer capacity, removes mnemonic storage logic from reset flow, and bumps the version to 3.17.0. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this 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: Organization 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.
📒 Files selected for processing (7)
legacy/firmware/ethereum_networks.clegacy/firmware/ethereum_networks_onekey.h.makolegacy/firmware/ethereum_onekey.hlegacy/firmware/protob/messages-tron.optionslegacy/firmware/reset.clegacy/firmware/tron.clegacy/firmware/version.h
💤 Files with no reviewable changes (1)
- legacy/firmware/reset.c
🧰 Additional context used
🪛 Clang (14.0.6)
legacy/firmware/ethereum_onekey.h
[warning] 100-100: macro argument should be enclosed in parentheses
(bugprone-macro-parentheses)
🪛 Cppcheck (2.19.0)
legacy/firmware/tron.c
[style] 176-176: The function 'is_mode_unprivileged' is never used.
(unusedFunction)
⏰ 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: Defs check
- GitHub Check: Style check
- GitHub Check: Gen check
🔇 Additional comments (8)
legacy/firmware/version.h (1)
10-13: LGTM!Version bump to 3.17.0 is consistent and properly applied to both macro and string.
legacy/firmware/ethereum_onekey.h (1)
99-101: LGTM!Pharos chain (1672) mapping added correctly and consistently with the existing pattern. The placement maintains numerical order.
Note: Static analysis flags missing parentheses around the
chain_namemacro parameter (line 100), but this is a pre-existing pattern throughout the macro definition, not introduced by this change.legacy/firmware/ethereum_networks.c (1)
7-7: LGTM!Network count correctly incremented to 12 to reflect the new Pharos entry.
legacy/firmware/ethereum_networks_onekey.h.mako (3)
38-39: LGTM!Pharos (1672) suffix mapping added correctly. The " PROS" suffix matches the symbol defined in ethereum_networks.c.
Note: This change also adds DTT (9798) suffix, which isn't mentioned in the PR objectives. Confirm this is intentional or if it should be a separate change.
48-50: LGTM!Pharos slip44 (1672) correctly recognized as valid Ethereum-compatible chain.
61-62: LGTM!Chain-to-slip44 mapping for Pharos (1672 → 1672) is consistent with the network definition.
legacy/firmware/protob/messages-tron.options (1)
25-25: Document the serialized_tx buffer size calculation.The max_size increased from 2048 to 2946 bytes. Add a comment explaining the breakdown: cmessage buffer (2560 bytes) + capi buffer (64 bytes) + signature (65 bytes) + protobuf field overhead. This clarifies why 2946 was chosen and connects it to the cmessage buffer in tron.c.
legacy/firmware/tron.c (1)
176-176: Stack capacity is adequate for the 2560-byte buffer.The cmessage buffer allocation of 2560 bytes represents only ~2% of the available 127K stack (128K SRAM minus the 1024-byte safety margin already enforced by the firmware). The firmware's stack limit is set with a 1024-byte safety cushion, and similar large allocations (e.g., unsignedEvent_bytes[2048] in nostr.c) exist elsewhere in the codebase without issues. No further action needed.
Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.