-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat(legacy): add misc features #124
Conversation
WalkthroughThis pull request updates several submodules, JSON token definitions, protocol messages, and UI elements. It adds new Ethereum networks and tokens, enhances Solana message signing through new proto definitions and functions, and improves Cardano transaction handling. Several internationalization files now support German, and utility functions gain extra validation and buffer handling. Firmware functions across ADA, PSBT, and layout modules are refined with extra checks and updated signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as Firmware
participant H as HDNode
C->>F: Send offchain message signing request
F->>F: Validate message (solana_sanitize_offchain_message)
F->>F: Prepare message (prepare_message)
F->>H: Request signature (ed25519_sign/hdnode_sign)
H-->>F: Return signature
F->>C: Deliver SolanaMessageSignature
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 17
🔭 Outside diff range comments (2)
legacy/firmware/ada.c (2)
183-354
: 🧹 Nitpick (assertive)Large function for deriving address bytes.
The implementation is thorough, but it's quite long. Consider splitting it into smaller sub-functions for clarity and maintainability.
1065-1914
: 🧹 Nitpick (assertive)Extensive additions for inline datum, reference scripts, certificate logic, minting, etc.
These expansions greatly improve flexibility and cover new transaction features. However, complexity has grown. Consider grouping logic into separate modules. Also ensure thorough tests for corner cases like partial minted amounts or invalid script bits.🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 1065-1065: The function 'txHashBuilder_addInlineDatumChunk' is never used.
(unusedFunction)
[style] 1083-1083: The function 'txHashBuilder_addReferenceScriptChunk' is never used.
(unusedFunction)
[style] 1380-1380: The function 'txHashBuilder_addCertificate' is never used.
(unusedFunction)
[style] 1413-1413: The function 'txHashBuilder_addWithdrawal' is never used.
(unusedFunction)
[style] 1460-1460: The function 'txHashBuilder_addAuxiliaryData' is never used.
(unusedFunction)
[style] 1515-1515: The function 'txHashBuilder_addMint' is never used.
(unusedFunction)
[style] 1549-1549: The function 'txHashBuilder_addRequiredSigner' is never used.
(unusedFunction)
[style] 1752-1752: The function '_processs_tx_init' is never used.
(unusedFunction)
[style] 1843-1843: The function 'cardano_txack' is never used.
(unusedFunction)
[style] 1861-1861: The function 'cardano_txwitness' is never used.
(unusedFunction)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (44)
common/defs/ethereum/chains
(1 hunks)common/defs/evm_tokens/1.json
(1 hunks)common/defs/evm_tokens/177.json
(1 hunks)common/defs/evm_tokens/9798.json
(1 hunks)common/defs/support.json
(4 hunks)common/protob/messages-solana.proto
(3 hunks)common/protob/messages.proto
(1 hunks)legacy/buttons.h
(1 hunks)legacy/firmware/ada.c
(19 hunks)legacy/firmware/ada.h
(6 hunks)legacy/firmware/chinese.c
(3 hunks)legacy/firmware/ethereum_networks.c
(2 hunks)legacy/firmware/ethereum_onekey.h
(1 hunks)legacy/firmware/ethereum_tokens.c
(3 hunks)legacy/firmware/ethereum_tokens_onekey.c.mako
(0 hunks)legacy/firmware/ethereum_tokens_onekey.h.mako
(1 hunks)legacy/firmware/fsm.c
(1 hunks)legacy/firmware/fsm.h
(1 hunks)legacy/firmware/fsm_msg_ada.h
(2 hunks)legacy/firmware/fsm_msg_coin.h
(1 hunks)legacy/firmware/fsm_msg_common.h
(1 hunks)legacy/firmware/fsm_msg_solana.h
(1 hunks)legacy/firmware/gettext.c
(2 hunks)legacy/firmware/i18n/i18n.c
(1 hunks)legacy/firmware/i18n/i18n.h
(1 hunks)legacy/firmware/i18n/keys.h
(1 hunks)legacy/firmware/i18n/locales/de.inc
(1 hunks)legacy/firmware/i18n/locales/en.inc
(1 hunks)legacy/firmware/i18n/locales/es.inc
(1 hunks)legacy/firmware/i18n/locales/ja.inc
(1 hunks)legacy/firmware/i18n/locales/pt_br.inc
(1 hunks)legacy/firmware/i18n/locales/zh_cn.inc
(1 hunks)legacy/firmware/i18n/locales/zh_tw.inc
(1 hunks)legacy/firmware/layout2.c
(5 hunks)legacy/firmware/layout2.h
(1 hunks)legacy/firmware/menu_list.c
(1 hunks)legacy/firmware/protob/messages-solana.options
(1 hunks)legacy/firmware/psbt/psbt.c
(12 hunks)legacy/firmware/psbt/psbt.h
(6 hunks)legacy/firmware/solana.c
(2 hunks)legacy/firmware/solana.h
(2 hunks)legacy/script/i18n.py
(2 hunks)legacy/util.c
(3 hunks)legacy/util.h
(3 hunks)
💤 Files with no reviewable changes (1)
- legacy/firmware/ethereum_tokens_onekey.c.mako
🧰 Additional context used
🪛 Buf (1.47.2)
common/protob/messages-solana.proto
38-38: Field named %!q(MISSING) should not be required.
(FIELD_NOT_REQUIRED)
56-56: Field named %!q(MISSING) should not be required.
(FIELD_NOT_REQUIRED)
67-67: Field named %!q(MISSING) should not be required.
(FIELD_NOT_REQUIRED)
81-81: Field named %!q(MISSING) should not be required.
(FIELD_NOT_REQUIRED)
89-89: Field named %!q(MISSING) should not be required.
(FIELD_NOT_REQUIRED)
common/protob/messages.proto
387-387: Enum value name "MessageType_SolanaSignOffChainMessage" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_SOLANA_SIGN_OFF_CHAIN_MESSAGE".
(ENUM_VALUE_UPPER_SNAKE_CASE)
388-388: Enum value name "MessageType_SolanaMessageSignature" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_SOLANA_MESSAGE_SIGNATURE".
(ENUM_VALUE_UPPER_SNAKE_CASE)
389-389: Enum value name "MessageType_SolanaSignUnsafeMessage" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_SOLANA_SIGN_UNSAFE_MESSAGE".
(ENUM_VALUE_UPPER_SNAKE_CASE)
🪛 Cppcheck (2.10-2)
legacy/firmware/solana.c
[style] 215-215: The function 'solana_sign_offchain_message' is never used.
(unusedFunction)
[style] 256-256: The function 'solana_sanitize_offchain_message' is never used.
(unusedFunction)
[style] 297-297: The function 'solana_sign_unsafe_message' is never used.
(unusedFunction)
legacy/util.c
[style] 73-73: The function 'int2str' is never used.
(unusedFunction)
[style] 253-253: The function 'is_printable' is never used.
(unusedFunction)
[style] 257-257: The function 'init_buffer_reader' is never used.
(unusedFunction)
[style] 263-263: The function 'init_buffer_writer' is never used.
(unusedFunction)
[style] 268-268: The function 'read_bytes' is never used.
(unusedFunction)
[style] 276-276: The function 'write_bytes' is never used.
(unusedFunction)
legacy/firmware/ada.c
[style] 446-446: The function 'txHashBuilder_addInput' is never used.
(unusedFunction)
[style] 800-800: The function 'txHashBuilder_addOutput' is never used.
(unusedFunction)
[style] 954-954: The function 'txHashBuilder_addAssetGroup' is never used.
(unusedFunction)
[style] 989-989: The function 'txHashBuilder_addToken' is never used.
(unusedFunction)
[style] 1065-1065: The function 'txHashBuilder_addInlineDatumChunk' is never used.
(unusedFunction)
[style] 1083-1083: The function 'txHashBuilder_addReferenceScriptChunk' is never used.
(unusedFunction)
[style] 1380-1380: The function 'txHashBuilder_addCertificate' is never used.
(unusedFunction)
[style] 1413-1413: The function 'txHashBuilder_addWithdrawal' is never used.
(unusedFunction)
[style] 1515-1515: The function 'txHashBuilder_addMint' is never used.
(unusedFunction)
[style] 1549-1549: The function 'txHashBuilder_addRequiredSigner' is never used.
(unusedFunction)
[style] 1843-1843: The function 'cardano_txack' is never used.
(unusedFunction)
[style] 1861-1861: The function 'cardano_txwitness' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Defs check
- GitHub Check: Style check
🔇 Additional comments (102)
common/defs/ethereum/chains (1)
1-1
: Subproject Commit Update Looks GoodThe commit ID has been updated to
fa90d06165cf20bf1086546e10a2a76bbf056010
. This is a straightforward update with no change in functionality.common/defs/evm_tokens/9798.json (2)
1-4
: Schema Definition Check
The top-level JSON structure is clear and concise. The "name" and "chain" fields correctly identify the blockchain as "Data Trade Chain."
5-68
: Token Objects Review
Every token object is well defined with the expected fields: "id", "name", "symbol", "address", and "decimals." The formatting is consistent, and the values (especially for "decimals") look appropriate for their token types.common/defs/support.json (3)
782-782
: New token support added for HashKey Platform Token.The ERC20 token "HSK" is properly added with version 3.11.0, following alphabetical ordering in the list.
2045-2045
: New token support added for DTT.The Ethereum token "DTT" is correctly added with version 3.11.0, placed in proper alphabetical order among other Ethereum tokens.
2251-2251
: New token support added for S.The Ethereum token "S" is properly added with version 3.11.0, following the consistent pattern of other token entries.
legacy/firmware/i18n/i18n.h (1)
19-19
: German language support added correctly.The declaration for German language support follows the established pattern for other language declarations.
legacy/firmware/ethereum_onekey.h (1)
74-79
: New blockchain networks properly added.Added support for two new EVM-compatible networks: "Sonic" (chain ID 146) and "HashKey Chain" (chain ID 177). The implementation follows the existing pattern.
legacy/firmware/gettext.c (2)
25-26
: German language case added correctly.The German language support (case 6) has been properly implemented in the
gettextX
function.
50-50
: Good fix: added missing newline.Added missing newline at end of function, improving code consistency.
legacy/firmware/menu_list.c (1)
188-189
:❓ Verification inconclusive
German language option added to menu.
Added "Deutsch" entry following the same structure as other language entries.
I'd recommend verifying the German localization files exist and have been properly tested for UI display:
🏁 Script executed:
#!/bin/bash # Check if German language files are properly included echo "Checking for German language files..." find legacy -name "*de*" -type f | grep -i languageLength of output: 138
German Localization: Manual Verification Needed
- The German language option has been added in
legacy/firmware/menu_list.c
(lines 188-189).- The automated search for German localization files did not yield any results.
- Please manually confirm that the necessary German language files exist and that the UI displays correctly.
legacy/script/i18n.py (2)
7-7
: German language support added successfully.The addition of "de" to the SUPPORTED_LANGS tuple expands language support to include German.
19-19
: Nice work mapping German language.The entry 'de': 'Deutsch' correctly maps the language code to its display name.
legacy/firmware/i18n/locales/zh_cn.inc (1)
369-375
: New Solana-related strings look good.The added strings provide clear warnings about Solana raw message signing risks and related terminology in Simplified Chinese.
legacy/firmware/i18n/locales/de.inc (1)
1-376
: German language file implemented correctly.The new German language file follows the established structure and includes all necessary UI strings, including the Solana warning messages.
legacy/firmware/fsm.h (1)
169-170
:❓ Verification inconclusive
Solana message signing functions declared properly.
These function declarations add support for both off-chain and unsafe message signing in Solana. The declarations follow the project's naming conventions and parameter patterns.
🏁 Script executed:
#!/bin/bash # Check for the implementation of these newly declared functions grep -n "fsm_msgSolanaSignOffChainMessage\|fsm_msgSolanaSignUnsafeMessage" --include="*.c" -r .Length of output: 97
Please Verify Function Implementations
The Solana message signing functions are declared properly in
legacy/firmware/fsm.h
(lines 169–170). Their declarations follow the project’s naming conventions and parameter patterns. However, our initial search did not locate any corresponding definitions in.c
files.
- Confirm that
fsm_msgSolanaSignOffChainMessage
andfsm_msgSolanaSignUnsafeMessage
are implemented elsewhere or are scheduled for implementation.- If they are defined in non-
.c
files or intended for future addition, update the verification accordingly.legacy/firmware/fsm_msg_common.h (1)
192-199
: Good safety checks added for BUILD_ID handling.The static assertions ensure BUILD_ID is at least 7 characters long and the buffer is large enough. The code now correctly extracts just the last 7 characters from the full build ID. This prevents potential buffer overflow issues.
legacy/firmware/ethereum_tokens_onekey.h.mako (1)
10-10
: Token count definition now matches actual list size.Fixed the TOKENS_COUNT definition to exactly match the erc20_list length, removing the arbitrary +9 offset. This change makes the code more accurate and easier to maintain.
common/defs/evm_tokens/177.json (1)
1-30
: New HashKey Chain tokens added correctly.The file properly defines tokens for the HashKey Chain (chain ID 177) with the expected structure. All token entries have complete information including name, symbol, address, and decimal precision.
legacy/firmware/ethereum_networks.c (3)
7-7
: Network count updated to match additions.The NETWORKS_COUNT constant is properly increased from 8 to 10 to reflect the two new networks.
52-63
: Two new networks added with standard formatting.Added support for Sonic (chain_id 146) and HashKey Chain (chain_id 177) networks. Both use slip44 value 60.
67-67
: Comment clarification for Data Trade Chain.Improved comment to specify the full name "Data Trade Chain" for the DTT symbol.
common/protob/messages.proto (1)
387-389
: Well-implemented Solana message signing supportThe additions expand Solana functionality with three new message types that follow the existing pattern. The new types enable off-chain message signing and unsafe message handling capabilities.
🧰 Tools
🪛 Buf (1.47.2)
387-387: Enum value name "MessageType_SolanaSignOffChainMessage" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_SOLANA_SIGN_OFF_CHAIN_MESSAGE".
(ENUM_VALUE_UPPER_SNAKE_CASE)
388-388: Enum value name "MessageType_SolanaMessageSignature" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_SOLANA_MESSAGE_SIGNATURE".
(ENUM_VALUE_UPPER_SNAKE_CASE)
389-389: Enum value name "MessageType_SolanaSignUnsafeMessage" should be UPPER_SNAKE_CASE, such as "MESSAGE_TYPE_SOLANA_SIGN_UNSAFE_MESSAGE".
(ENUM_VALUE_UPPER_SNAKE_CASE)
legacy/firmware/i18n/locales/ja.inc (1)
369-375
: Good security warnings in localized stringsThe Japanese translations properly warn users about phishing and blind signing risks. The clear warnings help users make informed decisions about enabling Solana raw signing.
legacy/firmware/protob/messages-solana.options (1)
9-17
: Appropriate field size constraintsThe size limits for the new Solana message types look appropriate:
- Address counts match existing patterns (max 8)
- Public key and signature sizes align with cryptographic standards (32/64 bytes)
- Message size limits differ between regular (1212 bytes) and unsafe (2048 bytes) messages
Note the intentional difference between message size limits - unsafe messages allow larger payloads.
legacy/firmware/i18n/locales/zh_tw.inc (1)
369-375
: Consistent localization for security featuresThe Traditional Chinese translations correctly warn users about the risks of Solana raw signing. The translations maintain consistency with other language files while clearly explaining potential dangers.
legacy/firmware/fsm_msg_coin.h (1)
1058-1061
:❓ Verification inconclusive
Verify the fingerprint access change is consistent.
The code now checks
output->bip32_path_lookuped
instead of looping through multiple keypaths. Make sure this change maintains equivalent validation logic and doesn't skip necessary checks.
🏁 Script executed:
#!/bin/bash # Check for other instances where similar pattern changes might be needed grep -r "hd_keypaths\[.*\]\.key_origin\.fingerprint" --include="*.c" --include="*.h" .Length of output: 88
Action Required: Confirm Fingerprint Logic Consistency
The change to check
output->bip32_path_lookuped
instead of looping through keypaths appears sound. Our grep search did not reveal any other fingerprint accesses tied tohd_keypaths
. However, the lack of output means the result is inconclusive. Please perform a manual review of the related fingerprint logic to ensure that all necessary validations remain intact and no edge cases are missed.legacy/firmware/solana.h (1)
26-26
: Looks good - necessary include for new functions.The parser include supports the new Solana message handling capabilities.
legacy/firmware/i18n/locales/pt_br.inc (1)
369-375
: New Solana signing translations look good.These Portuguese translations correctly warn users about the risks of raw Solana signing. The warnings about phishing and unauthorized transactions are clear and consistent with other language files.
legacy/firmware/i18n/locales/en.inc (1)
369-375
: Clear security warnings for Solana raw signing.The English text effectively warns users about phishing risks and blind signing dangers. The messages are concise and emphasize that users should only proceed if they understand the risks and trust the source.
legacy/firmware/i18n/i18n.c (1)
3-10
: German language support added correctly.The changes properly integrate German language support by:
- Increasing
langs_len
to 7- Adding "de" and "Deutsch" to the language arrays
- Including the German locale file
Good use of clang-format directives to maintain formatting consistency.
legacy/firmware/fsm_msg_solana.h (1)
71-99
: Well-implemented Solana off-chain message signing.The function follows security best practices:
- Verifies device initialization
- Validates path parameters
- Sanitizes input messages
- Requires PIN verification
- Properly handles errors
The public key handling and signature generation look correct.
legacy/firmware/i18n/keys.h (1)
763-777
: New Solana raw signing definitions added.You've added key definitions for Solana raw signing functionality with appropriate warning messages about security risks. The constants follow existing naming conventions and provide clear user warnings about phishing and unauthorized transactions.
legacy/firmware/i18n/locales/es.inc (1)
369-375
: Spanish translations added for Solana raw signing.The Spanish translations correctly match the newly added English keys in keys.h. Proper localization ensures Spanish-speaking users understand the security implications of Solana raw signing.
legacy/firmware/ethereum_tokens.c (3)
8-8
: Token count updated.Token count increased from 33 to 38 to reflect the five new tokens being added.
209-217
: New HSK token added for ETH mainnet.You've added HSK token with proper decimals and address for chain_id 1 (Ethereum mainnet).
237-271
: Added token support for chain_id 177.You've added four new tokens for chain_id 177:
- WHSK (Wrapped HSK)
- WETH (Wrapped ETH)
- USDT
- WBTC (Wrapped BTC)
All tokens include proper decimal places and bytecode addresses.
legacy/firmware/layout2.h (1)
303-306
: Enhanced layoutSignMessage parameter list for better security.The function signature has been improved with:
- Changed
is_ascii
tois_printable
for broader character support- Added
item_name
anditem_value
parameters to show more context- Added
is_unsafe
flag to mark potentially dangerous operationsThese changes allow for better context display and security warnings during message signing.
legacy/firmware/layout2.c (1)
1159-1159
: Ensure edge cases are handled for non-ASCII data
Switching from ASCII-only checks tois_printable
might expand valid characters. Confirm multi-byte or extended characters won't cause unintended behavior.legacy/firmware/fsm.c (2)
553-560
: Ensure consistent naming and comments.
The change fromis_valid_ascii
tois_printable
is good. However, ensure any in-code comments or references to ASCII checks are updated accordingly. Also, confirm that this logic aligns with your security and formatting needs.
564-570
: Validate parameter usage.
You switched fromis_valid_ascii
tois_printable
. Make sure the new function fully handles all intended edge cases (e.g., non-ASCII characters, zero-length messages). Double-check any references in docs or other files.legacy/firmware/chinese.c (4)
334-335
: Validate newline parsing logic.
You're usingmemchr
to find\n
withinrowlen
, then incrementingrowcount
. Watch out for corner cases if the text has no newlines or if it's shorter thanrowlen
.Also applies to: 337-337
350-352
: Conditional long-press enabling.
enableLongPress(true)
is placed inside#if !EMULATOR
. Double-check if the emulator build requires alternative handling. Document the difference clearly.
409-410
: Disable long-press carefully.
EnsureenableLongPress(false)
only triggers if no other part of the code depends on long-press events at this point. This can avoid unexpected behavior for user interactions.
413-415
: Standard font usage.
You’re forcingFONT_STANDARD
here. Double-check if you need dynamic font selection. The final layout might differ if the user expects a different size or style.common/protob/messages-solana.proto (5)
8-13
: Good clarity for versioning.
IntroducingSolanaOffChainMessageVersion
helps future-proof the design. Keep doc comments short.
38-38
: Review field requirement.
Static analysis suggests this field “should not be required.” If it truly must be present in all messages, keep it. Otherwise, make it optional.🧰 Tools
🪛 Buf (1.47.2)
38-38: Field named %!q(MISSING) should not be required.
(FIELD_NOT_REQUIRED)
56-57
: Validate “required” signature.
Same hint from static tools. If the signature is always mandatory, that’s fine. If there’s any scenario it can be absent, consider optional.🧰 Tools
🪛 Buf (1.47.2)
56-56: Field named %!q(MISSING) should not be required.
(FIELD_NOT_REQUIRED)
59-71
: Solid addition for off-chain message signing.
The new request structure is helpful. Double-check the size constraints forapplication_domain
. Could be beneficial to explicitly define or enforce 32 bytes in code.🧰 Tools
🪛 Buf (1.47.2)
67-67: Field named %!q(MISSING) should not be required.
(FIELD_NOT_REQUIRED)
84-90
: Check required signature usage.
Make sure it’s always present. If not, switch to optional. Also good to include the signer's public key as optional.🧰 Tools
🪛 Buf (1.47.2)
89-89: Field named %!q(MISSING) should not be required.
(FIELD_NOT_REQUIRED)
legacy/firmware/psbt/psbt.c (12)
6-6
: No issues adding util.h.
This import eases access to buffer utilities and fits well with the surrounding usage.
182-182
: Good boundary check for vin_count.
This check prevents invalid input counts, using the new MAX_INPUTS constant. Nicely done.
194-194
: Good boundary check for vout_count.
Consistently using MAX_OUTPUTS avoids magic numbers. Looks good.
234-238
: Robust checks on tap_leaf_hash array size.
Validating num_hashes against the array limit is good to prevent out-of-bounds errors.
350-351
: Potential off-by-one risk.
Usingif (input->partial_sigs_len >= MAX_INPUTS - 1)
might block valid writes ifpartial_sigs_len
can matchMAX_INPUTS - 1
exactly. Verify the intended capacity.
385-393
: Key length check might need broader context.
Accepting only 34 or 66 bytes is specific. Ensure external code won't pass other valid lengths. The rest looks fine.
539-547
: Similar key length check for outputs.
Same concern as inputs: confirm external code won't require other lengths. The bip32 reading logic is otherwise solid.
671-684
: Ensure safety in bip32 serialization.
The code concatenates pubkey data and writes it. Double-check buffer bounds onkey[66]
. Everything else is clear.
861-862
: Deserialization for global unsigned TX.
These new checks are clear and handle errors well.
903-903
: Consistent usage of MAX_INPUTS/MAX_OUTPUTS.
Checks here are neat. Avoids overflow or large loops that degrade performance.Also applies to: 911-911
970-975
: Transaction hash verification logic is clear.
Serializing and rehashing prevents mismatch. Good flow.
869-877
: 🛠️ Refactor suggestionUnreachable code after return false.
The code returns false at line 867, but lines 868–881 attempt further actions. Consider removing unreachable lines or using break instead of return.- return false; - if (key_reader.length != 79) return false; + if (key_reader.length != 79) return false;Likely an incorrect or invalid review comment.
legacy/firmware/solana.c (3)
193-213
: New prepare_message function.
Writes domain, format, signers, and message length. Straightforward approach. Check for potential buffer overrun or misuse if message length is near the upper boundary.
215-255
: Offchain message signing.
Flow is logical: 1) get address 2) show prompt 3) prepare message 4) sign message. Good user confirmation step. The fallback for non-EMULATOR is also consistent.🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 215-215: The function 'solana_sign_offchain_message' is never used.
(unusedFunction)
256-293
: Offchain message sanitization.
Checks domain length, message length, version, then ASCII or UTF-8 validity. This is thorough. Nice job verifying message constraints.🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 256-256: The function 'solana_sanitize_offchain_message' is never used.
(unusedFunction)
legacy/util.c (5)
56-71
: Updated uint2str plus int2str.
Switching to 64-bit is good for bigger numbers. Implementation is compact. The reversing logic is standard. No issues.
193-200
: ASCII validation.
This is a simple range check. It's fine.
202-251
: UTF-8 validation.
Algorithm is well-detailed, covering overlong scenarios. This ensures safe text parsing. Good addition.
253-255
: Combined check for 'printable'.
Allows ASCII or valid UTF-8. Very convenient.🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 253-253: The function 'is_printable' is never used.
(unusedFunction)
257-262
: Buffer reader/writer logic.
Initialization and read/write checks are standard. Guards for overflows look correct. This is a clean approach.Also applies to: 263-267, 268-275, 276-287
🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 257-257: The function 'init_buffer_reader' is never used.
(unusedFunction)
legacy/util.h (6)
25-25
: Good addition of<stddef.h>
.
Helps definesize_t
usage consistently.
34-38
: Check boundary handling forBufferReader
.
Ensure reads won't exceed the buffer length.
40-44
: Watch for potential out-of-bounds writes inBufferWriter
.
Confirm buffers won't overflow when writing.
77-78
: Extended range for numeric string conversion.
Switching to 64-bit parameters prevents overflow issues.
86-87
: Well-definedinit_buffer_reader
.
Makes buffer handling clearer and maintainable.
88-90
: Reading and writing rely on correct size checks.
Verify partial operations or error handling for incomplete I/O.legacy/firmware/psbt/psbt.h (5)
6-7
: Constants for input/output array sizes.
Makes the code more flexible if usage grows.
34-35
: Adaptedvin
andvout
to macros.
Ensure no references to numeric constants remain elsewhere.
78-78
:partial_sigs[MAX_INPUTS - 1]
logic check.
Ensure you won't exceed array bounds ifMAX_INPUTS
changes.
82-82
: Singlebip32_path
replaces an array.
Reduces complexity. Confirm usage aligns with rest of code.
155-156
: Consistent usage ofinputs[MAX_INPUTS]
andoutputs[MAX_OUTPUTS]
.
Matches the new macro-based approach.legacy/firmware/ada.h (11)
36-36
:MAX_CHUNK_SIZE
declared.
Helps unify chunk handling, but validate performance with large data.
73-78
: Babbage output keys enumerated.
Clear definitions improve readability for new output fields.
79-82
: Babbage datum types enumerated.
Explicit 0/1 assignments help avoid confusion in usage.
106-106
: RenamedTX_HASH_BUILDER_IN_COLLATERAL_RETURN
.
Matches updated concepts for Babbage transactions.
114-123
: Expanded output state machine.
Gives clearer transitions for output data, assets, and references.
125-130
: New mint state machine.
Makes minting more consistent with the existing builder pattern.
146-151
: New counters inAdaSigner
.
Confirm they're decremented properly as chunks and tokens are handled.
152-152
:mintState
integration.
Ensures mint states flow with existing transaction states.
162-163
: Inline datum and reference script sizes tracked.
Great for chunk-based parsing. Just be mindful of memory constraints.
176-176
:state_transmute()
function introduced.
Check transitions carefully to avoid invalid states.
178-178
:txHashBuilder_addInput
now returnsbool
.
Adding error checks is good. Double-check existing callers.legacy/firmware/ada.c (6)
39-63
: Good job centralizing CBOR appending in macros and helper functions.
These macros and functions reduce repetition, making the code concise. Keep an eye on potential side-effect pitfalls, but your usage of do-while(0) is a solid approach.
133-134
: Straightforward check for mainnet vs. testnet.
Returning the result of the equality is clear. Looks good.
162-181
: Localizingcrc32
enhances encapsulation.
Marking itstatic
limits its scope, which helps with maintainability.🧰 Tools
🪛 Cppcheck (2.10-2)
[portability] 172-172: Shifting signed 32-bit value by 31 bits is implementation-defined behaviour
(shiftTooManyBitsSigned)
[error] 172-172: Signed integer overflow for expression '1<<31'.
(integerOverflow)
414-495
: Great new input/output handling routines.
These functions improve readability by isolating distinct steps: entering inputs, appending them, switching states, and validating addresses. Code flow is more structured now.🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 446-446: The function 'txHashBuilder_addInput' is never used.
(unusedFunction)
781-951
:get_address_bytes
andtxHashBuilder_addOutput
logic.
The functions systematically handle address types and data. The overall design is coherent, but watch for boundary checks on arrays and buffers.🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 800-800: The function 'txHashBuilder_addOutput' is never used.
(unusedFunction)
953-1063
: Asset group and token additions.
Combining asset groups into the transaction is well-structured. Make sure the code is tested against edge cases (e.g., zero tokens).🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 954-954: The function 'txHashBuilder_addAssetGroup' is never used.
(unusedFunction)
[style] 989-989: The function 'txHashBuilder_addToken' is never used.
(unusedFunction)
legacy/firmware/fsm_msg_ada.h (4)
140-151
: Witness request and host acknowledgment
Directly callingcardano_txwitness
andcardano_txack
streamlines the flow. The quick layoutHome fallback on failure is a good user experience.
162-201
: Input, output, asset, token, certificate, and withdrawal messages
You now pass messages to the new builder functions and transmute state. This keeps the logic modular and reduces duplication.
209-225
: Unsupported pool queries
Sending an error and returning is appropriate since these operations are not yet implemented.
226-263
: Mint, required signer, inline datum, reference script, reference input
All additions properly call their builder counterparts, then refresh the UI. The safe exit on unsupported features is correct.
Summary by CodeRabbit
New Features
Improvements