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.

Sign up for GitHub

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

Jump to bottom

feat(core/ethereum): new ETH contract flow #4270

Merged
merged 1 commit into from
Oct 30, 2024
Merged

feat(core/ethereum): new ETH contract flow #4270

merged 1 commit into from
Oct 30, 2024

Conversation

ibz
Copy link
Contributor

@ibz ibz commented Oct 18, 2024 •
edited
Loading

Fixes #4251

@ibz ibz self-assigned this Oct 18, 2024
@ibz ibz force-pushed the ibz/20241118-eth-contracts branch 2 times, most recently from 2d720de to 793bde7 Compare October 18, 2024 13:44
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 18, 2024 •
edited
Loading

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) 2724
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@ibz
Copy link
Contributor Author

ibz commented Oct 21, 2024

Here is how the new flow looks (TS5):

image
image
image
image
image
image

@Hannsek
Copy link
Contributor

Hannsek commented Oct 21, 2024 •
edited
Loading

Thanks. The fourth screen looks a bit different. Please add two line bottom part. And maybe shorten the amount of data displayed on the screen. cc @lapohoda
image

How do the screen in the menu with data looks like?

Can you please run the CI in different languages? Does the text fit everywhere?

How about TS3?

@ibz ibz force-pushed the ibz/20241118-eth-contracts branch 7 times, most recently from 66fc602 to 39f7298 Compare October 23, 2024 15:54
@ibz
Copy link
Contributor Author

ibz commented Oct 23, 2024

How about TS3?

image

image

image

image

image

I know it's not 100% like Figma. Still have to do a couple of tweaks. But it's getting close...

@ibz ibz force-pushed the ibz/20241118-eth-contracts branch 3 times, most recently from 18d1c7e to 069a2c0 Compare October 24, 2024 09:40
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 24, 2024 •
edited
Loading

legacy UI changes device test(screens) main(screens)

@ibz ibz force-pushed the ibz/20241118-eth-contracts branch 4 times, most recently from 0856612 to 020bf4e Compare October 24, 2024 12:28
@ibz ibz marked this pull request as ready for review October 24, 2024 12:28
@ibz ibz requested review from obrusvit and removed request for prusnak and matejcik October 24, 2024 12:28
@ibz ibz force-pushed the ibz/20241118-eth-contracts branch 2 times, most recently from 24d8f0d to 1683e06 Compare October 24, 2024 15:05
@ibz ibz force-pushed the ibz/20241118-eth-contracts branch from 1683e06 to b5df4f2 Compare October 25, 2024 08:43
obrusvit
obrusvit requested changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

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

The flow looks nice on the example provided:

trezorctl ethereum sign-tx -n "m/44'/60'/0'/0/0" -d a9059cbb000000000000000000000000574bbb36871ba6b78e27f4b4dcfb76ea0091880b0000000000000000000000000000000000000000000000000000000000000123 -g 100 -G 100 -i 1 0xfc6b5d6af8a13258f7cbd0d39e11b35e01a32f93 0

However, the simple transfer is now modified and I'm not sure it's what we want. See a comment below.

Also, from the point of view of a reviewer, I suggest splitting the PR into more logical commits which would ease the review.
For example, a decision on improving of the confirm_action by introducing ConfirmActionMenu and ConfirmActionStrings should be done by two separate commits. You can always squash later.

core/src/apps/nem/multisig/layout.py Show resolved Hide resolved
core/src/apps/ethereum/sign_tx.py Outdated Show resolved Hide resolved
core/embed/rust/src/ui/model_mercury/layout.rs Show resolved Hide resolved
@ibz ibz force-pushed the ibz/20241118-eth-contracts branch from d79537a to 0d8f92f Compare October 25, 2024 11:21
obrusvit
obrusvit reviewed Oct 29, 2024
View reviewed changes
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

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

In general, I think we should go for more specialized rust/python functions rather than extending the existing functions with more arguments but I don't want to block the PR over this.

And as discussed in the past, updating fixtures.json before the review makes it difficult to review the UI changes locally :(

core/translations/en.json Outdated Show resolved Hide resolved
mmilata
mmilata approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Did not test but code-wise I do not see any problem 👍

Please don't forget to include fixtures for translations.

@mmilata mmilata added the translations Put this label on a PR to run tests in all languages label Oct 29, 2024
@ibz ibz force-pushed the ibz/20241118-eth-contracts branch from d21e844 to b283a4f Compare October 30, 2024 07:56
obrusvit
obrusvit approved these changes Oct 30, 2024
View reviewed changes
core/embed/rust/src/ui/model_tr/layout.rs
@@ -388,7 +417,9 @@ extern "C" fn new_confirm_properties(n_args: usize, args: *const Obj, kwargs: *m
paragraphs.into_paragraphs(),
TR::buttons__confirm.into(),
None,
false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rebase on main might cause conflict here, should be easy to resolve.

@ibz ibz force-pushed the ibz/20241118-eth-contracts branch 4 times, most recently from e79201b to fd30397 Compare October 30, 2024 12:37
@ibz ibz force-pushed the ibz/20241118-eth-contracts branch from fd30397 to aa4365e Compare October 30, 2024 14:28
@ibz ibz merged commit c300576 into main Oct 30, 2024
137 of 138 checks passed
@ibz ibz deleted the ibz/20241118-eth-contracts branch October 30, 2024 15:25
@bosomt
Copy link

bosomt commented Oct 31, 2024

QA OK

tested via:

trezorctl ethereum sign-tx -n "m/44'/60'/0'/0/0" -d a9059cbb000000000000000000000000574bbb36871ba6b78e27f4b4dcfb76ea0091880b0000000000000000000000000000000000000000000000000000000000000123 -g 100 -G 100 -i 1 0xfc6b5d6af8a13258f7cbd0d39e11b35e01a32f93 0

Info:

  • Suite version: desktop 24.11.0 (bef8f3c4378237d4cf63580d4ab50756f56e3ec1)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuiteDev/24.11.0 Chrome/128.0.6613.162 Electron/32.1.2 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T3T1 2.8.4 regular (revision 3c468fa)
  • Transport: BridgeTransport 3.0.0-bundled.24.11.0

@ibz
Copy link
Contributor Author

ibz commented Nov 1, 2024 •
edited
Loading

Can you please run the CI in different languages? Does the text fit everywhere?

image

@ibz
Copy link
Contributor Author

ibz commented Nov 1, 2024

pt

image

@ibz
Copy link
Contributor Author

ibz commented Nov 1, 2024

es

image

@ibz
Copy link
Contributor Author

ibz commented Nov 1, 2024 •
edited
Loading

PS: I know that this is different from Figma, where there is a shorter version of this string in TS3 / TT. I took a shortcut and have just one string in all models. I will add the shorter version and this will be fixed. Just FYI. IMO it is not a show stopper. @Hannsek

@Hannsek
Copy link
Contributor

Hannsek commented Nov 1, 2024

Right, please create an issue for the fix and link it here. 🙏🏻

@ibz
Copy link
Contributor Author

ibz commented Nov 1, 2024

es "Contrato de interacción" too long...

image

@ibz ibz mentioned this pull request Nov 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obrusvit obrusvit obrusvit approved these changes

@mmilata mmilata mmilata approved these changes

Assignees

@ibz ibz

Labels
translations Put this label on a PR to run tests in all languages
Projects
Firmware
Status: ✅ Approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EVM call contract (send token) flow
5 participants
@ibz @Hannsek @bosomt @mmilata @obrusvit