refactor(core): improved Stellar transaction signing interface for a more streamlined user experience.

[overcat]

Key Changes

• Warning for TX Source Mismatch: We'll now show a warning when the transaction source doesn't match your Trezor account.
• Improved Memo Alert: The alert page for when a memo isn't set has been optimized.
• Enhanced Payment UI: The user interface for payment operations has been improved.
• Enhanced Create Account UI: The user interface for the "Create Account" operation has been optimized.
• Recipient #{output_index}: If a tx contains multiple payment-related operations, we will use Recipient #{output_index} to label them.

You can find the design here, though the final design has some slight differences which you can see in the follow-up discussions.

[overcat]

A Few Questions

1. Do I need to manually add translation files? I'm guessing not, but when do we typically update those?

2. Do I need to manually update ./tests/ui_tests/fixtures.json? It seems like ./tests/update_fixtures.py might not work in my own repository. Perhaps I'll only be able to use it after the CI runs in this repository?

Hi @matejcik, could you please help me with this? I haven't contributed code here for a long time.

[Hannsek]

cc @obrusvit

[matejcik]

Do I need to manually add translation files? I'm guessing not, but when do we typically update those?

you need to add English strings. you can ignore the other languages

Do I need to manually update ./tests/ui_tests/fixtures.json? It seems like ./tests/update_fixtures.py might not work in my own repository. Perhaps I'll only be able to use it after the CI runs in this repository?

nobody does that manually.
if you run tests e.g. via make test_emu_ui_multicore in core directory, you should be able to run tests/update_fixtures.py local to update them. if there's a problem with that, please report what you see.

[overcat]

I've made some adjustments to meet Stellar's requirements, but this has impacted certain interfaces for Ethereum and Solana.

Solana before and after:

Ethereum before and after:

I'm not sure why we're not consistent with delizia's implementation here, especially since the subtitle isn't ignored there.

trezor-firmware/core/src/trezor/ui/layouts/delizia/__init__.py

Lines 635 to 655 in 5194e1e

	def confirm_address(
	title: str,
	address: str,
	subtitle: str | None = None,
	description: str | None = None,
	verb: str | None = None,
	warning_footer: str | None = None,
	chunkify: bool = True,
	br_name: str | None = None,
	br_code: ButtonRequestType = BR_CODE_OTHER,
	) -> Awaitable[None]:
	return confirm_value(
	title,
	address,
	description or "",
	br_name or "confirm_address",
	br_code,
	subtitle=subtitle,
	verb=(verb or TR.buttons__confirm),
	chunkify=chunkify,
	)

[Hannsek]

This is fine. Thanks for pointing this out.

[obrusvit]

Hello @overcat, I rebased on top of main and force-pushed so that CI can run.

• I did minor fixes in d79e5c6
• It's possible that ui_fixtures.json will have to be updated again, I will take care of it.

From the discussion in the issue, the last thing remaining in the UI is the cancellation from the warning screen, am I right? I'll take a look whether we can do it easily.

Thank you.

[obrusvit]

One more thing @overcat, do you think you could provide trezorctl commands which trigger the usecases shown in your videos? Thank you.

[overcat]

Hi @obrusvit

From the discussion in the issue, the last thing remaining in the UI is the cancellation from the warning screen, am I right? I'll take a look whether we can do it easily.

This should apply to all alert pages, not just Stellar's. So, would it be better to do this in a separate PR?

do you think you could provide trezorctl commands which trigger the usecases shown in your videos? Thank you.

Here's some test data; you can find more in https://github.com/trezor/trezor-firmware/blob/main/common/tests/fixtures/stellar/sign_tx.json (check xdr field).

trezorctl device load -m 'all all all all all all all all all all all all'
# Source not equals Trezor Account (show warning page)
trezorctl stellar sign-transaction 'AAAAAgAAAADTSPIr3MGTm9sMI5oJJ/GuXE2KFkCxLma1cUfqm1LKzQAAAGQAAAAAAAAD6AAAAAEAAAAAG4J3zQAAAABd5CqEAAAAAAAAAAEAAAAAAAAAAQAAAABdVWQkZrGFuEMVLp4hkVHbxYkgJ+xAEBpRe+1coDDC4AAAAAAAAAAAHc8WmAAAAAAAAAAA'
# Payment with non-native asset
trezorctl stellar sign-transaction 'AAAAAgAAAAAvIrnGLwi3dPPr5t1ufbk8PsLL3gJ5Vho9nFIluMMikgAAAGQAAAAAAAAD6AAAAAEAAAAAG4J3zQAAAABd5CqEAAAAAAAAAAEAAAAAAAAAAQAAAABdVWQkZrGFuEMVLp4hkVHbxYkgJ+xAEBpRe+1coDDC4AAAAAFYAAAAAAAAACmElgLCDlg2XxI8Eth6HKRVDRDNIbCuDhjUTj5W/WoVAAAAAB3PFpgAAAAAAAAAAA=='
# All Send-like operations
trezorctl stellar sign-transaction 'AAAAAgAAAAAvIrnGLwi3dPPr5t1ufbk8PsLL3gJ5Vho9nFIluMMikgAAAfQAAAAAAAAD6AAAAAEAAAAAG4J3zQAAAABd5CqEAAAAAAAAAAUAAAAAAAAAAAAAAABdVWQkZrGFuEMVLp4hkVHbxYkgJ+xAEBpRe+1coDDC4AAAAAAF9eEAAAAAAAAAAAEAAAAAXVVkJGaxhbhDFS6eIZFR28WJICfsQBAaUXvtXKAwwuAAAAABWAAAAAAAAAAphJYCwg5YNl8SPBLYehykVQ0QzSGwrg4Y1E4+Vv1qFQAAAAAdzxaYAAAAAAAAAA0AAAABWAAAAAAAAABwi6oxX35cFm2EtGS/s4/WJXj+OtJyJ+dsy7ehecRRIQAAAAAdzxaYAAAAAF1VZCRmsYW4QxUuniGRUdvFiSAn7EAQGlF77VygMMLgAAAAAkFCQ0RFRkdISUpLTAAAAAAphJYCwg5YNl8SPBLYehykVQ0QzSGwrg4Y1E4+Vv1qFQAAAAAAAeJAAAAAAAAAAAAAAAACAAAAAVgAAAAAAAAAcIuqMV9+XBZthLRkv7OP1iV4/jrScifnbMu3oXnEUSEAAAAAHc8WmAAAAABdVWQkZrGFuEMVLp4hkVHbxYkgJ+xAEBpRe+1coDDC4AAAAAJBQkNERUZHSElKS0wAAAAAKYSWAsIOWDZfEjwS2HocpFUNEM0hsK4OGNROPlb9ahUAAAAAAAHiQAAAAAAAAAAAAAAACAAAAABdVWQkZrGFuEMVLp4hkVHbxYkgJ+xAEBpRe+1coDDC4AAAAAAAAAAA'
# Text Memo
trezorctl stellar sign-transaction 'AAAAAgAAAAAvIrnGLwi3dPPr5t1ufbk8PsLL3gJ5Vho9nFIluMMikgAAAGQAAAAAAAAD6AAAAAEAAAAAG4J3zQAAAABd5CqEAAAAAQAAAA1IZWxsbywgd29ybGQhAAAAAAAAAQAAAAAAAAABAAAAAF1VZCRmsYW4QxUuniGRUdvFiSAn7EAQGlF77VygMMLgAAAAAAAAAAAdzxaYAAAAAAAAAAA='

[Hannsek]

This should apply to all alert pages, not just Stellar's. So, would it be better to do this in a separate PR?

I thought it is an issue only with this pr, but as you said, it is problem with all warning screens. So, let's not block this pr with this.

[ibz]

Rebased on main and force pushed...

[ibz]

BTW, I am taking this over so we can get unstuck. Thank you, @overcat for the great job!!!

After rebasing and dealing with conflicts, I am doing a final review - including a review of the UI changes.

I have to say that generally the UI looks much better than before!

In some cases it feels like a slight downgrade, for example:

Was replaced with:

I honestly liked seeing the fact that an account is created, not just a generic Recipient #1 - what do you think @Hannsek ? Should I in this case show Create account #1 instead of Recipient #1 in the title?

Another question, for @overcat

Is this really a "maximum fee" or is it an exact fee amount that we are dealing with? Since we changed the design, I would not have expected a change in the semantics... The two are not the same!

But generally the UI feels more consistent and in line with other coins, so I think having this merged will be a plus.

PS: I will look into the warning cancellation after this gets merged, since it is a separate issue...

[overcat]

Hi @ibz, it's maximum fee. See https://stellar.org/blog/developers/surge-pricing-on-stellar-faq

CAP5 introduces “dutch auction” fee payments which ensure that the operation fee you submit is the maximum you will pay and the actual amount is the lowest possible.

[ibz]

LGTM.

I removed some strings that are now unused.

One thing to note is that we used to warn when using testnet for a transaction, now we don't.

I suppose it is not a risk of losing funds, because at worst what happens is somebody sends testnet tokens without realizing they are testnet... not the other way around.

[ibz]

Renamed to timebounds

[ibz]

I will squash, force push, and get it ready to be merged...

[overcat]

One thing to note is that we used to warn when using testnet for a transaction, now we don't.

I suppose it is not a risk of losing funds, because at worst what happens is somebody sends testnet tokens without realizing they are testnet... not the other way around.

Yes, this design has been clarified here.

[ibz]

has been clarified

Right, I missed that part at the bottom about the testnet, but seems it's all clear. It was indeed an unnecessary complication for no added benefit.