feat(legacy): add support for EIP-712 signing #168
Conversation
|
Caution Review failedFailed to post review comments WalkthroughAdds EIP-712 (typed-data) OneKey signing support: new capability, new OneKey protobufs/options, reserved message IDs, wire_no_fsm marks, typed-data envelope/encoding and signing code, a synchronous call() API with manual message processing, UI/display and OLED API adjustments, build/linker updates, and nanopb bump. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Host
participant Device as Device FSM
participant ETH as Ethereum OneKey
participant TD as TypedData Engine
participant UI as UI
rect rgb(237,245,255)
note over Host,Device: EIP-712 OneKey signing flow
Host->>Device: MessageType_EthereumSignTypedDataOneKey (wire_in)
Device->>ETH: fsm_msgEthereumSignTypedDataOneKey(msg)
ETH->>TD: Initialize envelope, collect types
loop Collect struct definitions
ETH->>Device: TypedDataStructRequestOneKey (wire_out)
Device->>Host: Send request
Host-->>Device: TypedDataStructAckOneKey (wire_in, no_fsm)
Device->>ETH: Provide struct members
end
loop Collect values
ETH->>Device: TypedDataValueRequestOneKey (wire_out)
Device->>Host: Send request
Host-->>Device: TypedDataValueAckOneKey (wire_in, no_fsm)
Device->>ETH: Provide value
end
TD->>TD: Compute type/domain/message hashes (Keccak)
UI->>UI: Show summaries & confirmations
ETH->>ETH: ECDSA sign (secp256k1)
ETH-->>Device: EthereumTypedDataSignatureOneKey
Device-->>Host: Signature (wire_out)
end
sequenceDiagram
autonumber
participant Caller as Firmware caller
participant FSM as fsm.c call()
participant MSG as messages.c
participant USB as Transport
note over FSM,MSG: Synchronous manual processing call()
Caller->>FSM: call(req_type, msg_ptr, expected_type)
FSM->>USB: Write request
loop poll
FSM->>MSG: Poll & decode incoming frames
alt decode ok and matches expected
MSG-->>FSM: response ptr
FSM-->>Caller: return response
else decode ok but unexpected
MSG-->>FSM: unexpected id → failure
FSM-->>Caller: return failure
else decode error / cancel / timeout
MSG-->>FSM: set error flags
FSM-->>Caller: return failure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Buf (1.57.2)legacy/firmware/protob/messages-ethereum-eip712-onekey.protoFailure: no .proto files were targeted. This can occur if no .proto files are found in your input, --path points to files that do not exist, or --exclude-path excludes all files. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 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.
📒 Files selected for processing (20)
common/protob/messages-management.proto(1 hunks)common/protob/messages.proto(2 hunks)legacy/firmware/Makefile(2 hunks)legacy/firmware/ethereum_onekey.c(2 hunks)legacy/firmware/ethereum_onekey.h(2 hunks)legacy/firmware/ethereum_typed_data.h(1 hunks)legacy/firmware/fsm.c(1 hunks)legacy/firmware/fsm.h(2 hunks)legacy/firmware/fsm_msg_common.h(2 hunks)legacy/firmware/fsm_msg_ethereum_onekey.h(1 hunks)legacy/firmware/layout2.c(1 hunks)legacy/firmware/messages.c(4 hunks)legacy/firmware/messages.h(1 hunks)legacy/firmware/oled_text.c(3 hunks)legacy/firmware/oled_text.h(1 hunks)legacy/firmware/protob/Makefile(2 hunks)legacy/firmware/protob/messages-ethereum-eip712-onekey.options(1 hunks)legacy/firmware/protob/messages-ethereum-eip712-onekey.proto(1 hunks)legacy/gd32_hard/memory_app_1.8.0_gd32.ld(1 hunks)vendor/nanopb(1 hunks)
🧰 Additional context used
🪛 Buf (1.57.2)
common/protob/messages.proto
227-227: Enum value name "MessageType_EthereumTypedDataStructAckOneKey" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_ETHEREUM_TYPED_DATA_STRUCT_ACK_ONE_KEY".
(ENUM_VALUE_UPPER_SNAKE_CASE)
228-228: Enum value name "MessageType_EthereumTypedDataValueRequestOneKey" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_ETHEREUM_TYPED_DATA_VALUE_REQUEST_ONE_KEY".
(ENUM_VALUE_UPPER_SNAKE_CASE)
229-229: Enum value name "MessageType_EthereumTypedDataValueAckOneKey" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_ETHEREUM_TYPED_DATA_VALUE_ACK_ONE_KEY".
(ENUM_VALUE_UPPER_SNAKE_CASE)
⏰ 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: Gen check
- GitHub Check: Style check
🔇 Additional comments (5)
legacy/firmware/protob/Makefile (1)
20-20: Good call adding the new protoGlad to see the OneKey EIP‑712 proto wired into the build list. Looks solid.
legacy/gd32_hard/memory_app_1.8.0_gd32.ld (2)
49-49: LGTM!Ensures minimum 16K heap. Good sanity check.
44-46: Verify bootloader flag region is not overlapped by heap
- The heap spans from
endto_ram_end - 16K(0x2002C000). The bootloader flag at 0x20010000 sits inside this range ifendis before it. Ensureend >= 0x20010000or explicitly reserve the flag address before enabling the heap.- Line 48: Remove the tautological
ASSERT ((_heap_end + 16K) <= _ram_end).vendor/nanopb (1)
1-1: Ack bumpSubmodule pointer update noted. No further feedback.
legacy/firmware/fsm.c (1)
217-253: Surface decode errors fromcall()When
msg_decode_error_occurredflips true we drop straight out of the loop, leave the flag set, and never emit a Failure. The host never learns why the sync exchange stopped, and the next call still sees the stale flag and exits immediately. Please clear the flag and send an explicit error response before returning.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
legacy/firmware/oled_text.c (1)
209-236: Newlines are incorrectly counted as separate lines.The
line_count++at line 217 runs before the newline check at line 218. This means a newline character increments the counter even though it draws nothing. For example,"Hello\nWorld"returns 3 instead of 2.Move the increment after the newline check:
uint8_t draw_string_wrap(int x, int y, const char *text, uint8_t font) { const char *p = text; uint8_t height = font_get_height(); int cursor_x = x; int cursor_y = y; int char_width = 0, line_width = 0; uint8_t line_count = 0; while (*p) { - line_count++; if (*p == '\r' || *p == '\n') { cursor_x = 0; cursor_y += height; p++; continue; } + line_count++; const char *next_line = get_next_line(p, OLED_WIDTH - cursor_x, &line_width, font); while (p < next_line) { char_width = get_char_width(p, font); draw_char(cursor_x, cursor_y, p, font); cursor_x += char_width; p = utf8_next(p); } cursor_y += height; cursor_x = 0; } return line_count; }
♻️ Duplicate comments (4)
legacy/firmware/protob/Makefile (1)
9-10: Trim the escaped line continuation.Keep the comments as real comments. Drop the trailing
\on Line 8 and the extra\signs on Lines 9–10 so the assignment ends cleanly.- TxAckInput TxAckOutput TxAckPrev TxAckPaymentRequest \ - # EthereumSignTypedData EthereumTypedDataStructRequest EthereumTypedDataStructAck \ - # EthereumTypedDataValueRequest EthereumTypedDataValueAck \ + TxAckInput TxAckOutput TxAckPrev TxAckPaymentRequest + # EthereumSignTypedData EthereumTypedDataStructRequest EthereumTypedDataStructAck + # EthereumTypedDataValueRequest EthereumTypedDataValueAcklegacy/firmware/ethereum_onekey.c (1)
1717-1757: Check display_info_init() before using display_info
display_info_init()allocates the backing array and reports failure via its return value. We ignore that and immediately indexdisplay_info.items, so a low-memory failure turns into a NULL dereference. The same pattern appears at the seconddisplay_info_init()below. Please bail out (and send a failure response) when init returns false.legacy/firmware/ethereum_typed_data.h (2)
145-159: Fix DisplayInfo capacity overflow
items_capacityis auint8_t. Once you double past 0x80 the math wraps to zero,reallocshrinks the array, and the next write corrupts memory. Please promote the counters (andnew_capacity) to a wider type or clamp growth so the arithmetic never wraps.
227-304: Guard entry_types_count before appending
entry_typesholds only eight slots, yet every ARRAY member blindly appends and bumpsentry_types_count. A struct with more than eight array members (or nested arrays) walks past the buffer and smashes the envelope. Add a hard capacity check in every path that writes toentry_types(dependent/domain/primary) and fail cleanly when the limit is hit.
📜 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.
📒 Files selected for processing (20)
common/protob/messages-management.proto(1 hunks)common/protob/messages.proto(2 hunks)legacy/firmware/Makefile(2 hunks)legacy/firmware/ethereum_onekey.c(2 hunks)legacy/firmware/ethereum_onekey.h(2 hunks)legacy/firmware/ethereum_typed_data.h(1 hunks)legacy/firmware/fsm.c(1 hunks)legacy/firmware/fsm.h(2 hunks)legacy/firmware/fsm_msg_common.h(2 hunks)legacy/firmware/fsm_msg_ethereum_onekey.h(1 hunks)legacy/firmware/layout2.c(1 hunks)legacy/firmware/messages.c(4 hunks)legacy/firmware/messages.h(1 hunks)legacy/firmware/oled_text.c(3 hunks)legacy/firmware/oled_text.h(1 hunks)legacy/firmware/protob/Makefile(2 hunks)legacy/firmware/protob/messages-ethereum-eip712-onekey.options(1 hunks)legacy/firmware/protob/messages-ethereum-eip712-onekey.proto(1 hunks)legacy/gd32_hard/memory_app_1.8.0_gd32.ld(1 hunks)vendor/nanopb(1 hunks)
🧰 Additional context used
🪛 Buf (1.57.2)
common/protob/messages.proto
227-227: Enum value name "MessageType_EthereumTypedDataStructAckOneKey" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_ETHEREUM_TYPED_DATA_STRUCT_ACK_ONE_KEY".
(ENUM_VALUE_UPPER_SNAKE_CASE)
228-228: Enum value name "MessageType_EthereumTypedDataValueRequestOneKey" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_ETHEREUM_TYPED_DATA_VALUE_REQUEST_ONE_KEY".
(ENUM_VALUE_UPPER_SNAKE_CASE)
229-229: Enum value name "MessageType_EthereumTypedDataValueAckOneKey" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_ETHEREUM_TYPED_DATA_VALUE_ACK_ONE_KEY".
(ENUM_VALUE_UPPER_SNAKE_CASE)
⏰ 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: Gen check
- GitHub Check: Style check
- GitHub Check: Defs check
🔇 Additional comments (5)
legacy/firmware/oled_text.h (1)
22-22: LGTM!The signature change from
voidtouint8_taligns with the implementation and lets callers know how many lines were rendered.legacy/firmware/ethereum_onekey.h (1)
26-50: Header glue looks right.
The new include and prototype match the added workflow.legacy/firmware/fsm_msg_common.h (1)
123-133: Capability list stays consistent.
Count now matches the nine populated entries and keeps the new flag reachable.legacy/firmware/fsm.h (1)
34-164: Dispatcher update looks good.
The FSM header now exposes the new typed-data handler alongside the proto include.legacy/firmware/layout2.c (1)
218-226: Exporting the helper makes sense.
Droppingstaticexposeslayout_index_countfor the new typed-data UI pieces.
Summary by CodeRabbit
New Features
Chores