Tron: Improve Transfer flow#6883
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change improves the Tron TRX transfer confirmation flow by introducing structured account details handling throughout signing and layouts. A new confirm_trx_transfer delegates to layout-level confirm_tron_send, which now shows recipient (with account info) then a summary via confirm_tron_summary. sign_tx centralizes TX field validation in _validate_tx_fields, constructs account_details early, adjusts signing order (serialize/hash before key derivation), and threads account_details into process_contract. Multiple UI layouts (bolt, caesar, delizia, eckhart) add info_items/info_title to confirm_address and implement the two-step address+summary flow. A single-line changelog entry was added. Sequence DiagramsequenceDiagram
actor User
participant sign_tx as sign_tx.py
participant layout as apps/tron/layout.py
participant ui as UI Layouts
User->>sign_tx: Sign Tron Transaction
sign_tx->>sign_tx: _validate_tx_fields()
sign_tx->>sign_tx: Construct account_details
sign_tx->>sign_tx: Build & serialize raw tx
sign_tx->>sign_tx: Compute tx hash
sign_tx->>sign_tx: Derive signing node
alt TransferContract
sign_tx->>layout: process_contract(contract, fee_limit, account_details)
layout->>ui: confirm_trx_transfer(contract, account_details)
ui->>ui: confirm_address(address, info_items=account_details)
ui->>User: Display recipient with account details
User->>ui: Approve address
ui->>ui: confirm_tron_summary(amount, fee, account_details)
ui->>User: Display transaction summary
User->>ui: Approve summary
else Unknown Smart Contract
sign_tx->>layout: process_contract(contract, fee_limit, account_details)
layout->>ui: confirm_tron_summary(title, None, formatted_fee_limit)
ui->>User: Display fee summary
User->>ui: Approve
end
sign_tx->>sign_tx: Create signature
sign_tx->>User: Transaction signed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/trezor/ui/layouts/caesar/__init__.py (1)
846-867:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
confirm_address()still drops the new TRON context.Lines 856-857 add
info_items/info_title, but Line 859 still routes throughconfirm_blob(), so Caesar never renders the account details thatconfirm_tron_send()now passes in. That leaves this model short of the richer TRX recipient review introduced by this PR.🤖 Prompt for 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. In `@core/src/trezor/ui/layouts/caesar/__init__.py` around lines 846 - 867, confirm_address currently calls confirm_blob without forwarding the new TRON-specific context (info_items and info_title), so Caesar never renders the account details; update confirm_address to pass info_items and info_title (and preserve footer if needed) through to confirm_blob (keeping the existing br_name/br_code/verb/chunkify behavior) so the TRON recipient review provided by confirm_tron_send is rendered. Ensure you reference confirm_address and confirm_blob when making the change and keep existing defaults for br_name/br_code.
🤖 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 `@core/src/trezor/ui/layouts/bolt/__init__.py`:
- Line 1680: The label for the Tron summary uses the wrong translation key:
replace the use of TR.send__total_amount with TR.words__amount where
amount_label is set (the amount_label assignment in the bolt layout code in
__init__.py) so the UI shows "Amount" for the transfer summary when amount is
present; keep the conditional (if amount else "") unchanged.
In `@core/src/trezor/ui/layouts/caesar/__init__.py`:
- Around line 1795-1806: Replace the hard-coded English labels in the
approve/revoke branch with translation keys so non-English builds use localized
strings: instead of setting title, action_subtitle, value_subtitle, and the
summary_view literal strings inside the is_revoke/else block, use the
appropriate TR keys (e.g., TR.revoke__title / TR.revoke__action_subtitle /
TR.revoke__value_subtitle or TR.approve__title / TR.approve__action_subtitle /
TR.approve__value_subtitle) and make summary_view reference TR keys (e.g.,
TR.words__token or a new TR key for "Amount allowance") so all four identifiers
(title, action_subtitle, value_subtitle, summary_view) read from TR rather than
hard-coded English.
In `@core/src/trezor/ui/layouts/delizia/__init__.py`:
- Around line 1703-1723: confirm_tron_send currently calls confirm_address
without surfacing the "check with source" instruction; update the
confirm_tron_send implementation to pass TR.address__check_with_source into the
confirm_address call so the recipient step shows the source-check hint (either
by adding it as a subtitle/hint argument if confirm_address supports one, or by
inserting a prominent info_items entry like (TR.address__check_with_source,
None, False) before the account info). Ensure the change references the
confirm_tron_send function and the confirm_address call so the
TR.address__check_with_source text is displayed on the recipient screen.
- Around line 1691-1696: The amount is being labeled with TR.words__recipient in
the _confirm_summary call; change the amount_label argument to the correct
amount label (replace TR.words__recipient with the appropriate amount label,
e.g., TR.words__amount) so the transfer amount is shown under the “Amount”
heading instead of “Recipient”.
---
Outside diff comments:
In `@core/src/trezor/ui/layouts/caesar/__init__.py`:
- Around line 846-867: confirm_address currently calls confirm_blob without
forwarding the new TRON-specific context (info_items and info_title), so Caesar
never renders the account details; update confirm_address to pass info_items and
info_title (and preserve footer if needed) through to confirm_blob (keeping the
existing br_name/br_code/verb/chunkify behavior) so the TRON recipient review
provided by confirm_tron_send is rendered. Ensure you reference confirm_address
and confirm_blob when making the change and keep existing defaults for
br_name/br_code.
🪄 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: 909df7e8-149a-4f54-8cea-3a147a5c8744
📒 Files selected for processing (7)
core/.changelog.d/6520.fixedcore/src/apps/tron/layout.pycore/src/apps/tron/sign_tx.pycore/src/trezor/ui/layouts/bolt/__init__.pycore/src/trezor/ui/layouts/caesar/__init__.pycore/src/trezor/ui/layouts/delizia/__init__.pycore/src/trezor/ui/layouts/eckhart/__init__.py
PrisionMike
force-pushed
the
prisionmike/update-trx-transfer
branch
2 times, most recently
from
8c5ef4d to
8d119db
Compare
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 `@core/src/apps/tron/sign_tx.py`:
- Around line 235-239: The current confirm_blob call decodes the signed bytes
with bytes(msg.data).decode("utf-8", "replace"), which can mutate invalid
sequences; change it to render the exact bytes: check if bytes(msg.data) is
valid UTF-8 and if so pass the UTF-8 string, otherwise pass a hex representation
(or other lossless encoding) to confirm_blob (same call site where
chunkify=False and br_name="tron/note") so users always see an exact, non-lossy
view of the signed bytes.
🪄 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: c81b74bb-b9a0-4f6c-a493-1a64c800eb9c
📒 Files selected for processing (7)
core/.changelog.d/6520.fixedcore/src/apps/tron/layout.pycore/src/apps/tron/sign_tx.pycore/src/trezor/ui/layouts/bolt/__init__.pycore/src/trezor/ui/layouts/caesar/__init__.pycore/src/trezor/ui/layouts/delizia/__init__.pycore/src/trezor/ui/layouts/eckhart/__init__.py
✅ Files skipped from review due to trivial changes (1)
- core/.changelog.d/6520.fixed
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/apps/tron/layout.py
- core/src/trezor/ui/layouts/caesar/init.py
- core/src/trezor/ui/layouts/delizia/init.py
PrisionMike
force-pushed
the
prisionmike/update-trx-transfer
branch
from
8d119db to
f8320fd
Compare
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)
core/src/trezor/ui/layouts/caesar/__init__.py (1)
856-867:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
confirm_addressdropsinfo_items/info_title, so TRON recipient details never render
confirm_tron_sendpasses account info (Line 1782-Line 1786), butconfirm_addressignores those new params and always callsconfirm_blob. This silently breaks the intended “recipient + source/account details” flow.Suggested fix
def confirm_address( @@ ) -> Awaitable[None]: + if info_items: + return confirm_value( + title=subtitle or title, + value=address, + description=description, + br_name=br_name or "confirm_address", + br_code=br_code, + verb=verb, + info_items=info_items, + chunkify=chunkify, + chunkify_info=chunkify, + ) return confirm_blob( br_name or "confirm_address", subtitle or title, address, description,🤖 Prompt for 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. In `@core/src/trezor/ui/layouts/caesar/__init__.py` around lines 856 - 867, confirm_address currently drops info_items and info_title so calls from confirm_tron_send lose recipient/source details; update confirm_address to accept and forward the info_items and info_title parameters into its confirm_blob call (preserving existing args like br_name, subtitle/title, address, description, verb, br_code, chunkify) so recipient/account details render; reference confirm_tron_send, confirm_address, confirm_blob and the parameters info_items and info_title when making the change.
🤖 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 `@core/src/trezor/ui/layouts/eckhart/__init__.py`:
- Around line 1762-1766: The account name value account_details[0] can be None
but is unconditionally inserted into the StrPropertyType items in
confirm_tron_summary and confirm_tron_send; change both functions
(confirm_tron_summary, confirm_tron_send) to only append the account item when
account_details[0] (the account string) is non-empty/truthy (same guard pattern
used in _get_account_info_items), i.e., check if account_details[0] before
adding (TR.words__account_colon, account_details[0], False) /
(TR.words__account, account_details[0], False) so you never pass None into the
UI item list.
- Around line 1772-1775: The confirm_tron_summary call is coercing optional
parameters amount and amount_label to empty strings which loses the None
semantic expected by _confirm_summary/trezorui_api.confirm_summary; change the
call in confirm_tron_summary so it passes amount and amount_label as-is (do not
use `amount or ""` or `amount_label or ""`), leaving fee and fee_label coercions
intact, so trezorui_api.confirm_summary receives None when appropriate.
---
Outside diff comments:
In `@core/src/trezor/ui/layouts/caesar/__init__.py`:
- Around line 856-867: confirm_address currently drops info_items and info_title
so calls from confirm_tron_send lose recipient/source details; update
confirm_address to accept and forward the info_items and info_title parameters
into its confirm_blob call (preserving existing args like br_name,
subtitle/title, address, description, verb, br_code, chunkify) so
recipient/account details render; reference confirm_tron_send, confirm_address,
confirm_blob and the parameters info_items and info_title when making the
change.
🪄 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: a05a87d5-ab5d-4d5f-95b6-e2b3d8997bce
📒 Files selected for processing (7)
core/.changelog.d/6520.fixedcore/src/apps/tron/layout.pycore/src/apps/tron/sign_tx.pycore/src/trezor/ui/layouts/bolt/__init__.pycore/src/trezor/ui/layouts/caesar/__init__.pycore/src/trezor/ui/layouts/delizia/__init__.pycore/src/trezor/ui/layouts/eckhart/__init__.py
✅ Files skipped from review due to trivial changes (1)
- core/.changelog.d/6520.fixed
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/apps/tron/layout.py
- core/src/apps/tron/sign_tx.py
- core/src/trezor/ui/layouts/delizia/init.py
- core/src/trezor/ui/layouts/bolt/init.py
PrisionMike
added
the
translations
PrisionMike
force-pushed
the
prisionmike/update-trx-transfer
branch
3 times, most recently
from
ed4f7a9 to
fe6d516
Compare
|
@PrisionMike why does this test |
|
It seems like It goes into the menu via |
|
@coderabbitai Please read both of @obrusvit 's comment and create an issue for it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
I'll share the issue URL once the script completes. 🏁 Script executed: Length of output: 54
Caesar layout: _paginate_and_confirm behaves incorrectly, bypassing transaction confirmation It documents both of |
obrusvit
left a comment
obrusvit
left a comment
There was a problem hiding this comment.
One nit, otherwise LGTM.
PrisionMike
force-pushed
the
prisionmike/update-trx-transfer
branch
from
fe6d516 to
b2c5778
Compare
- Better transfer layout - account details shown - small code refactoring
PrisionMike
force-pushed
the
prisionmike/update-trx-transfer
branch
from
b2c5778 to
edfa0b8
Compare
2 participants
Fixes #6520