Skip to content
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

fix(solana): fix Memo, multisig and polish instructions UI #3445

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

gabrielKerekes
Copy link
Contributor

This PR fixes two important issues with Solana, one medium issue and minor issues with the way some instructions are displayed:

  1. Memo instruction is currently treated as an uknown instruction i.e. it's basically being blind signed.

  2. Multisig token transfer instruction is also treated as a predefined instruction and multisig signers are not shown. The only warning the user getting that it's a multisig instruction is the Transaction requires X signers which increases the fee. warning.

  3. The The following instruction is a multisig instruction. message is shown after the instruction parameters are shown but before the accounts - it should be displayed even before the parameters.

  4. The order in which some properties are displayed has been changed and the wording of some of the properties has been updated.

Note: I would have regenerated the UI fixtures but I get different results for all of them although definitely not all of them should've changed. Not sure what's going on here. This happens for both Trezor T and R, I run the T emulator with the -a switch to stop animation.

@Hannsek
Copy link
Contributor

Hannsek commented Dec 6, 2023

For receiving to a multisig address, users should see this screen:
image
"Multisig" should also be in the "receive address" header:
image

@gabrielKerekes gabrielKerekes force-pushed the fix/solana-memo-and-multisig branch from dd9e319 to bcaa944 Compare December 6, 2023 17:24
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM

@Hannsek
Copy link
Contributor

Hannsek commented Dec 7, 2023

@gabrielKerekes Please post here the UI changes.

@gabrielKerekes
Copy link
Contributor Author

For receiving to a multisig address, users should see this screen:

We're currently not optimising the UI for general instructions.

@gabrielKerekes Please post here the UI changes.

Will try.

@gabrielKerekes
Copy link
Contributor Author

Hopefully this will help you - the failed folder contains the html files from the tests report.

failed:changed ui tests.zip

@matejcik
Copy link
Contributor

matejcik commented Dec 8, 2023

all seems good in CI, please add changelog (since this is going out in the next release so it's a change)

@gabrielKerekes
Copy link
Contributor Author

👋 Will there be a FW release this month or when can we expect this to be released? 🙏

@Hannsek
Copy link
Contributor

Hannsek commented Jan 8, 2024

Next release will happen together with March release of Suite -> 13.3. (EAP)

@matejcik matejcik merged commit 1f4f126 into trezor:main Jan 10, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants