Conversation
WalkthroughThis pull request makes comprehensive updates to the Cardano-related modules. Protocol definitions and message types have been extended with new enums and fields (for example, CardanoDRep and CVote registration values). The updates include renaming governance registration to CVote registration, adding certificate fields (deposit and drep), and refining address validation, signing, and helper utilities. Similar changes are applied to TRON and CLI modules with revised imports and parameter handling. Changes
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (17)
core/src/apps/cardano/seed.py (2)
87-87: Remove redundant comment.The comment "this is true now" adds no value.
88-92: Simplify assert block.Multiple equality checks can be combined.
-assert ( - len(BYRON_ROOT) == len(SHELLEY_ROOT) - and len(MULTISIG_ROOT) == len(SHELLEY_ROOT) - and len(MINTING_ROOT) == len(SHELLEY_ROOT) -) +assert len(BYRON_ROOT) == len(SHELLEY_ROOT) == len(MULTISIG_ROOT) == len(MINTING_ROOT)python/src/trezorlib/messages.py (4)
588-590: Inconsistent line spacing after enum valuesExtra blank line after
VOTE_DELEGATION = 9STAKE_REGISTRATION_CONWAY = 7 STAKE_DEREGISTRATION_CONWAY = 8 VOTE_DELEGATION = 9 -
3607-3608: Missing docstring for classCardanoTxAuxiliaryDataSupplement
3667-3667: Typo in field namecvote_registration_signatureShould be
vote_registration_signature
3679-3679: Typo in field namecvote_registration_signatureShould be
vote_registration_signaturecore/src/apps/cardano/helpers/paths.py (1)
3-3: Remove unused importRemove
unhardenor use it.core/src/apps/cardano/sign_tx/multisig_signer.py (1)
19-20: Improve variable name clarity.
_assert_tx_init_condis unclear. Rename toassert_transaction_condition.core/src/apps/cardano/sign_tx/pool_owner_signer.py (2)
30-41: Add comments to validation conditions.Each condition in the tuple needs explanation.
86-88: Split error message.Message is too long. Split into two sentences.
- "Stakepool registration transaction can only contain the pool owner witness request" + "Invalid witness request. Only pool owner witness is allowed in stakepool registration."core/src/apps/common/cbor.py (1)
54-54: Rename ambiguous variable 'l'.Variable name 'l' is hard to read.
-def _header(typ: int, l: int) -> bytes: +def _header(typ: int, length: int) -> bytes:🧰 Tools
🪛 Ruff (0.8.2)
54-54: Ambiguous variable name:
l(E741)
python/src/trezorlib/cli/cardano.py (1)
160-164: Simplify chain code handling.Nested ternary is hard to read.
- "chain_code": ( - witness["chain_code"].hex() - if witness["chain_code"] is not None - else None - ), + "chain_code": witness["chain_code"].hex() if witness["chain_code"] else None,core/src/apps/cardano/addresses.py (1)
264-279: Move comments to function level.Inline comments about removed functions should be docstring comments.
- # _validate_shelley_address - - # _validate_size assert_cond( _MIN_ADDRESS_BYTES_LENGTH <= len(address_bytes) <= _MAX_ADDRESS_BYTES_LENGTH ) - - # _validate_bech32_hrp valid_hrp = _get_bech32_hrp(address_type, network_id) - # get_hrp bech32_hrp = address.rsplit(bech32.HRP_SEPARATOR, 1)[0] assert_cond(valid_hrp == bech32_hrp) - - # _validate_network_idcore/src/apps/cardano/README.md (1)
94-97: Grammar. Inconsistent singular and plural forms.core/src/apps/cardano/sign_tx/__init__.py (1)
20-20: Remove redundant import.CardanoSignTxFinished already imported in TYPE_CHECKING block.
- from trezor.messages import CardanoSignTxFinishedcore/src/apps/cardano/helpers/utils.py (1)
57-57: Fix typo in comment."concatenable" is not a word. Use "concatenatable" instead.
core/src/apps/cardano/sign_tx/plutus_signer.py (1)
60-61: Move imports to top of fileImports scattered across methods. Move them to the top.
Also applies to: 89-90, 98-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
common/protob/messages-cardano.proto(10 hunks)core/src/all_modules.py(1 hunks)core/src/apps/cardano/README.md(2 hunks)core/src/apps/cardano/addresses.py(14 hunks)core/src/apps/cardano/auxiliary_data.py(5 hunks)core/src/apps/cardano/byron_addresses.py(3 hunks)core/src/apps/cardano/certificates.py(9 hunks)core/src/apps/cardano/get_address.py(1 hunks)core/src/apps/cardano/get_native_script_hash.py(2 hunks)core/src/apps/cardano/get_public_key.py(3 hunks)core/src/apps/cardano/helpers/account_path_check.py(5 hunks)core/src/apps/cardano/helpers/bech32.py(2 hunks)core/src/apps/cardano/helpers/credential.py(7 hunks)core/src/apps/cardano/helpers/hash_builder_collection.py(3 hunks)core/src/apps/cardano/helpers/paths.py(2 hunks)core/src/apps/cardano/helpers/protocol_magics.py(1 hunks)core/src/apps/cardano/helpers/utils.py(5 hunks)core/src/apps/cardano/layout.py(28 hunks)core/src/apps/cardano/native_script.py(3 hunks)core/src/apps/cardano/seed.py(5 hunks)core/src/apps/cardano/sign_tx/__init__.py(2 hunks)core/src/apps/cardano/sign_tx/multisig_signer.py(3 hunks)core/src/apps/cardano/sign_tx/ordinary_signer.py(4 hunks)core/src/apps/cardano/sign_tx/plutus_signer.py(4 hunks)core/src/apps/cardano/sign_tx/pool_owner_signer.py(3 hunks)core/src/apps/cardano/sign_tx/signer.py(49 hunks)core/src/apps/common/cbor.py(6 hunks)core/src/apps/common/keychain.py(1 hunks)core/src/apps/common/paths.py(8 hunks)core/src/apps/tron/sign_message.py(1 hunks)core/src/apps/tron/sign_tx.py(1 hunks)core/src/trezor/enums/CardanoCertificateType.py(1 hunks)core/src/trezor/enums/CardanoDRepType.py(1 hunks)core/src/trezor/enums/CardanoTxAuxiliaryDataSupplementType.py(1 hunks)core/src/trezor/enums/__init__.py(2 hunks)core/src/trezor/messages.py(9 hunks)python/src/trezorlib/cardano.py(11 hunks)python/src/trezorlib/cli/cardano.py(10 hunks)python/src/trezorlib/messages.py(12 hunks)
✅ Files skipped from review due to trivial changes (3)
- core/src/trezor/enums/CardanoTxAuxiliaryDataSupplementType.py
- core/src/trezor/enums/CardanoDRepType.py
- core/src/apps/cardano/helpers/protocol_magics.py
👮 Files not reviewed due to content moderation or server errors (4)
- core/src/trezor/enums/CardanoCertificateType.py
- core/src/apps/cardano/get_native_script_hash.py
- core/src/apps/cardano/helpers/bech32.py
- core/src/apps/cardano/sign_tx/ordinary_signer.py
🧰 Additional context used
🪛 Ruff (0.8.2)
core/src/apps/cardano/byron_addresses.py
71-71: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
core/src/apps/cardano/get_address.py
42-42: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
core/src/apps/cardano/native_script.py
43-48: Combine if branches using logical or operator
Combine if branches
(SIM114)
150-163: Combine if branches using logical or operator
Combine if branches
(SIM114)
core/src/apps/common/cbor.py
54-54: Ambiguous variable name: l
(E741)
core/src/apps/cardano/certificates.py
67-68: Use a single if statement instead of nested if statements
(SIM102)
core/src/apps/cardano/sign_tx/signer.py
336-337: Use a single if statement instead of nested if statements
(SIM102)
341-342: Use a single if statement instead of nested if statements
(SIM102)
350-351: Use a single if statement instead of nested if statements
(SIM102)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Defs check
- GitHub Check: Style check
🔇 Additional comments (28)
core/src/apps/cardano/helpers/hash_builder_collection.py (2)
26-26: LGTM!
93-104: LGTM!core/src/apps/cardano/helpers/credential.py (1)
56-57: LGTM!Good use of local caching to improve performance.
Also applies to: 103-105, 180-181, 223-226
core/src/apps/common/keychain.py (1)
153-154: LGTM!Type ignore comments are appropriate here.
Also applies to: 163-163
core/src/apps/common/cbor.py (1)
8-11: LGTM!Clean imports and good addition of tagged set support.
Also applies to: 48-51, 308-310
core/src/all_modules.py (1)
1026-1027: LGTM!The new Cardano enums are correctly added.
Also applies to: 1030-1031
core/src/trezor/messages.py (1)
27-29: LGTM!The Cardano protocol updates for CVote registration and DRep support are well-structured and consistent.
Also applies to: 2124-2141, 2149-2150, 2190-2204, 2206-2232, 2235-2241, 2321-2328
common/protob/messages-cardano.proto (9)
59-62: LGTM.
64-69: LGTM.
77-80: LGTM.
82-86: LGTM.
349-358: LGTM.
369-372: LGTM.
533-539: LGTM.
412-415: LGTM.
478-482: LGTM.core/src/apps/cardano/README.md (1)
292-297: LGTM.core/src/apps/cardano/auxiliary_data.py (1)
1-332: No issues foundcore/src/apps/cardano/certificates.py (1)
1-400: No issues found🧰 Tools
🪛 Ruff (0.8.2)
67-68: Use a single
ifstatement instead of nestedifstatements(SIM102)
core/src/apps/cardano/layout.py (1)
1-1068: No issues foundcore/src/apps/common/paths.py (5)
364-374: LGTM!
377-388: LGTM!
391-392: LGTM!
395-401: LGTM!
421-435: LGTM!core/src/trezor/enums/__init__.py (3)
555-563: LGTM!
564-568: LGTM!
579-582: LGTM!
3 participants
Summary by CodeRabbit
• New Features
- Enhanced Cardano protocol support with new certificate types for stake registration (Conway) and vote delegation, plus refined delegation representation.
- Updated vote key registration aligned with current CIP standards and improved address validation.
- Expanded CLI options for address retrieval, transaction signing, and public key display.
• Documentation
- Clarified staking key derivation paths and auxiliary data guidance to better inform users.