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): use new ETH send flow also for staking #4143

Merged
merged 3 commits into from
Sep 3, 2024
Merged

feat(core): use new ETH send flow also for staking #4143

merged 3 commits into from
Sep 3, 2024

Conversation

ibz
Copy link
Contributor

@ibz ibz commented Aug 30, 2024

No description provided.

@ibz ibz self-assigned this Aug 30, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 30, 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)
T2B1 Safe 3 test(screens) main(screens) test(screens) main(screens) 2724
T3T1 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@ibz ibz force-pushed the ibz/20240830-stake branch 3 times, most recently from 3f14cc0 to 6e42d4c Compare September 2, 2024 13:06
@ibz ibz marked this pull request as ready for review September 2, 2024 14:51
@ibz ibz requested review from obrusvit and removed request for prusnak and matejcik September 2, 2024 14:51
@ibz ibz linked an issue Sep 3, 2024 that may be closed by this pull request
Send ETH (EVM) #3858
Closed
obrusvit
obrusvit reviewed Sep 3, 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.

Tests with input flows for ETH signtx and staking are still skipped. Otherwise, it looks good. We should take a look at the parameter set of the flows, calling a function with 18 arguments does not seem good.

core/src/trezor/ui/layouts/mercury/__init__.py Show resolved Hide resolved
core/src/trezor/ui/layouts/mercury/__init__.py Show resolved Hide resolved
@ibz
Copy link
Contributor Author

ibz commented Sep 3, 2024

We should take a look at the parameter set of the flows, calling a function with 18 arguments does not seem good.

I know, this flow grew too complex suddenly... It'll soon also make coffee for you after you click "Sign". I need to break it down into separate "versions".

obrusvit
obrusvit approved these changes Sep 3, 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.

Please wait for CI - update fixtures.json
Run make style.

@obrusvit obrusvit added this to the T3T1 milestone Sep 3, 2024
@obrusvit obrusvit added T3T1 translations Put this label on a PR to run tests in all languages labels Sep 3, 2024
@Hannsek
Copy link
Contributor

Hannsek commented Sep 3, 2024

I cannot see the diffs :/

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 3, 2024

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

@ibz
Copy link
Contributor Author

ibz commented Sep 3, 2024

I cannot see the diffs :/

They should be visible now. They were not before, because there were no tests enabled. After @obrusvit fixed the input flows and enabled the relevant tests, the screens appeared. But they are not really a diff, because they are new screens, so there is nothing to compare to...

@Hannsek
Copy link
Contributor

Hannsek commented Sep 3, 2024

New screens? We had staking before. Sorry, but I am confused now. @obrusvit can you clarify please?

@obrusvit
Copy link
Contributor

obrusvit commented Sep 3, 2024

New screens? We had staking before. Sorry, but I am confused now. @obrusvit can you clarify please?

We didn't have UI tests for staking on TS5 since TS5 release until now.
We had functional tests for the actual signing of the staking operations but the UI flow was tested manually only.

This PR enables the UI tests of staking for TS5 and implements them properly. That's why the screens are new.

@ibz ibz merged commit 423f159 into main Sep 3, 2024
119 of 121 checks passed
@ibz ibz deleted the ibz/20240830-stake branch September 3, 2024 15:59
@Hannsek
Copy link
Contributor

Hannsek commented Sep 3, 2024

Perfect, thanks for clarifying.

@bosomt
Copy link

bosomt commented Sep 4, 2024

QA OK

  • Device: Trezor T3T1 2.8.2 regular (revision d7e1f0f)

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

Assignees

@ibz ibz

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

Successfully merging this pull request may close these issues.

Send ETH (EVM)
4 participants
@ibz @Hannsek @obrusvit @bosomt