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

Factored out onboard and communication UI modules #98

Merged
merged 9 commits into from
Nov 25, 2022
Merged

Factored out onboard and communication UI modules #98

merged 9 commits into from
Nov 25, 2022

Conversation

italo-sampaio
Copy link
Collaborator

@italo-sampaio italo-sampaio commented Nov 1, 2022 •
edited
Loading

  • Factored out onboard-related operations
  • Factored out communication module
  • Incidentally removed unused rx parameter from unlock and pin modules

@italo-sampaio italo-sampaio force-pushed the enhancement/101-add-ui-seed-unit-tests branch from fe0dc79 to be3a4e3 Compare November 7, 2022 15:18
@italo-sampaio italo-sampaio changed the title Factored out seed UI module Factored out onboard and communication UI modules Nov 7, 2022
@italo-sampaio italo-sampaio marked this pull request as draft November 7, 2022 17:03
amendelzon
amendelzon requested changes Nov 8, 2022
View reviewed changes
Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

Looking great so far! I think there's a few things we need to tackle before we can think of getting this merged in, but we're not far off!

ledger/src/ui/src/bolos_ux.c Outdated Show resolved Hide resolved
ledger/src/ui/src/communication.c Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.c Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.c Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.c Show resolved Hide resolved
ledger/src/ui/src/onboard.c Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.h Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.h Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.h Outdated Show resolved Hide resolved
amendelzon
amendelzon requested changes Nov 10, 2022
View reviewed changes
Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

Almost there! There's a few minor changes we need to do re safety and we're practically ready for unit tests!

ledger/src/ui/src/onboard.h Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.c Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.c Outdated Show resolved Hide resolved
amendelzon
amendelzon reviewed Nov 11, 2022
View reviewed changes
ledger/src/ui/src/pin.c Outdated Show resolved Hide resolved
amendelzon
amendelzon previously approved these changes Nov 11, 2022
View reviewed changes
Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

This looks great! Really liked the pin module refactor, things are more readable now IMHO. Thanks again for tackling this!

@italo-sampaio
Factored out UI communication and onboard modules
964df1f 
- Factored out onboard operations
- Factored out communication operations
- Removed unused parameters from unlock and pin modules
- Reworked pin module
@italo-sampaio italo-sampaio force-pushed the enhancement/101-add-ui-seed-unit-tests branch from 7433d4b to 964df1f Compare November 14, 2022 10:56
amendelzon
amendelzon requested changes Nov 14, 2022
View reviewed changes
Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

Minor (but some important) additional comments.

ledger/src/ui/src/onboard.c Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.c Outdated Show resolved Hide resolved
ledger/src/ui/src/onboard.h Outdated Show resolved Hide resolved
@italo-sampaio italo-sampaio marked this pull request as ready for review November 14, 2022 15:05
@amendelzon amendelzon marked this pull request as draft November 16, 2022 14:39
amendelzon
amendelzon previously approved these changes Nov 16, 2022
View reviewed changes
Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

Looking great! Ready for unit tests!

@italo-sampaio italo-sampaio force-pushed the enhancement/101-add-ui-seed-unit-tests branch 2 times, most recently from 58d8e40 to f260f31 Compare November 17, 2022 13:29
@italo-sampaio italo-sampaio marked this pull request as ready for review November 17, 2022 13:35
@italo-sampaio italo-sampaio force-pushed the enhancement/101-add-ui-seed-unit-tests branch from f260f31 to 7c00d07 Compare November 17, 2022 13:48
@italo-sampaio italo-sampaio force-pushed the enhancement/101-add-ui-seed-unit-tests branch from b747e95 to 99712d5 Compare November 22, 2022 14:49
@italo-sampaio
Rework on UI unit test to change from function call list to a more be…
c591751 
…haviour based approach

- Updated communication tests
- Updated onboard tests
- Updated pin tests
- Updated unlock tests
@italo-sampaio italo-sampaio force-pushed the enhancement/101-add-ui-seed-unit-tests branch 2 times, most recently from dfdd146 to 4108443 Compare November 25, 2022 11:15
@italo-sampaio
More adjustments after code review
58afed0 
- Reworked retries test to just assert that the returned value is valid
- Changed some function names for clarity
- Removed retries counter logic from mock
- Added mock variables to assert that wipe and unlock operations are done while the device is locked
- Added a test case to assert that pin capping is applied
@italo-sampaio italo-sampaio force-pushed the enhancement/101-add-ui-seed-unit-tests branch from 4108443 to 58afed0 Compare November 25, 2022 11:44
amendelzon
amendelzon approved these changes Nov 25, 2022
View reviewed changes
Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

Looks good, retested and everything works properly on the physical device. Thanks for tackling this one!

@amendelzon amendelzon merged commit c47b585 into master Nov 25, 2022
@amendelzon amendelzon deleted the enhancement/101-add-ui-seed-unit-tests branch November 25, 2022 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amendelzon amendelzon amendelzon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@italo-sampaio @amendelzon