feat(trezor-client): implement EIP-712 streaming typed data signing#7038
Open
ifdario wants to merge 2 commits into
Open
feat(trezor-client): implement EIP-712 streaming typed data signing#7038ifdario wants to merge 2 commits into
ifdario wants to merge 2 commits into
Conversation
Add ethereum_sign_typed_data() which implements the full EthereumSignTypedData streaming protocol (message type 464). This enables on-device EIP-712 signing on modern Trezor devices (Safe 3, Safe 5, Model T) which do not support the legacy EthereumSignTypedHash message. Also adds ethereum_sign_typed_hash() for Model One compatibility.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds EIP-712 typed-data signing support to the Rust Trezor client. It introduces two public methods on the Trezor client: ethereum_sign_typed_hash for signing a precomputed domain separator (with optional message hash) and ethereum_sign_typed_data for the full streaming EIP-712 protocol. The implementation includes helpers to map EIP-712 type strings to protobuf field descriptors and to encode JSON values (integers, signed ints, addresses, bytes, booleans, strings, arrays) into the binary format expected by the device. The Cargo feature enables serde_json for these APIs. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/trezor-client/src/client/ethereum.rs`:
- Around line 235-251: The PinMatrixRequest and PassphraseRequest handling must
route through the module's existing interaction/callback path instead of
returning UnsupportedNetwork or auto-answering with an empty passphrase: for
protos::MessageType::MessageType_PinMatrixRequest, parse the
protos::PinMatrixRequest via resp.into_message() and invoke the same interaction
callback used by other flows to collect the PIN from the host (or wait for
on-device entry), then send the collected PIN back with the appropriate
PinMatrixAck via self.call_raw; for
protos::MessageType::MessageType_PassphraseRequest, parse the
protos::PassphraseRequest via resp.into_message(), if pr._on_device() follow the
current on-device ack path (set_on_device(true) and call_raw), otherwise call
the shared interaction callback to obtain the host-entered passphrase and set it
on protos::PassphraseAck with set_passphrase(...) before calling self.call_raw;
ensure you reuse the exact callback/interaction machinery used for ButtonRequest
handling rather than hardcoding responses.
- Around line 379-425: The encoding functions currently silently drop or panic
on invalid/oversized inputs; change encode_uint_be, encode_int_be, and
encode_typed_value to return Result<Vec<u8>, Error> (or a suitable error type)
and validate inputs: for hex inputs in encode_uint_be/encode_int_be use
hex::decode? and propagate decode errors instead of unwrap_or_default, check
decoded.len() <= 32 and <= byte_len before copying to avoid
buf[start..start+len] panics, and return an Err on oversize; for decimal
parsing, reject values that don't fit the expected bit width (use
parse_type_size(type_name) to compute byte_len and ensure parsed u128/i128 or
bigint fits) rather than falling back to zero; update callers to propagate
errors. Ensure encode_typed_value also handles serde_json::Number by converting
safely (or using BigInt parsing) and returns Err on out-of-range inputs or
malformed hex.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52f9fab4-1c1f-4bb1-9c95-7b0ca5a64dac
⛔ Files ignored due to path filters (1)
rust/trezor-client/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
rust/trezor-client/Cargo.tomlrust/trezor-client/src/client/ethereum.rs
…tion handling - Clamp byte_len to 32 and hex input length to prevent panics - Handle hex::decode errors gracefully instead of unwrap_or_default - Document that PIN/passphrase handling matches handle_interaction()
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ethereum_sign_typed_data()implementing the fullEthereumSignTypedDatastreaming protocol (message type 464)ethereum_sign_typed_hash()for Model One backward compatibility (message type 470)serde_jsonas optional dependency gated behind theethereumfeatureuint256and other large integer types correctly (up to 32 bytes)Closes #7037
Context
The Rust
trezor-clienthas protobuf bindings for the EIP-712 streaming protocol but no implementation to drive it. Modern Trezor devices (Safe 3, Safe 5, Model T) require this protocol and reject the legacyEthereumSignTypedHashwithFailure_UnexpectedMessage.The implementation handles the full streaming conversation:
EthereumSignTypedDatawith primary typeEthereumTypedDataStructRequestwith type definitionsEthereumTypedDataValueRequestwith encoded valuesButtonRequestandPassphraseRequestduring the flowEthereumTypedDataSignatureTest plan