Skip to content

[FIX] Fix SRP reveal screen#5980

Merged
cortisiko merged 5 commits intorelease/6.2.0from
fix/720-srp-confirm-screen
Mar 17, 2023
Merged

[FIX] Fix SRP reveal screen#5980
cortisiko merged 5 commits intorelease/6.2.0from
fix/720-srp-confirm-screen

Conversation

@Cal-L
Copy link
Copy Markdown
Contributor

@Cal-L Cal-L commented Mar 17, 2023 •
edited
Loading

Description

This PR fixes both the padding issue + interaction of the SRP reveal screen bug found in 6.2.0 regression
https://app.zenhub.com/workspaces/mobile-release-regression-6249e5242464b70013315a98/issues/gh/metamask/mobile-planning/720

Screenshots/Recordings

Reach out to @Cal-L for videos

Testing
There are three main scenarios that the reveal credentials screen affects (the first 3 scenarios. 3 and 4 are essentially the same scenario)

  • Scenario: Should be able to recover SRP when something errors out on the Root boundary

    • Given I am on the Login screen
    • And I enter the password and log in
    • When the app errors out and displays the Root error boundary
    • Then I should be able to follow the flow to reveal the SRP

  • Scenario: Should be able to recover SRP when something errors out on the Activity boundary

    • Given I am on the Wallet view
    • And I open the drawer and tap on Activity
    • When the app errors out and displays the ActivityView error boundary
    • Then I should be able to follow the flow to reveal the SRP

  • Scenario: Should be able to reveal SRP from the settings flow

    • Given I am on the Settings view
    • And I open Security & Privacy
    • When I tap on Reveal Secret Recovery Phrase
    • And I pass the quiz
    • Then I should be able to follow the flow to reveal the SRP

  • Scenario: Should be able to reveal private key from the settings flow

    • Given I am on the Settings view
    • And I open Security & Privacy
    • When I tap on Show private key
    • Then I should be able to follow the flow to reveal my private key

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@Cal-L Cal-L added type-bug Something isn't working needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) regression-RC DEPRECATED: Please use "regresssion-RC-x.y.z" label instead team-mobile-client release-6.2.0 Issue or pull request that will be included in release 6.2.0 labels Mar 17, 2023
@Cal-L Cal-L requested a review from a team as a code owner March 17, 2023 03:45
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 17, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

tommasini
tommasini approved these changes Mar 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

Thanks for pinging me to review this!

Just reinforced the need for testing on outside the navigation object screen

LGTM!!

sethkfman
sethkfman approved these changes Mar 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@Cal-L Cal-L force-pushed the fix/720-srp-confirm-screen branch from f937a69 to 98e89c3 Compare March 17, 2023 17:49
@Cal-L
Copy link
Copy Markdown
Contributor Author

Cal-L commented Mar 17, 2023

Just rebased

@Cal-L Cal-L removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 17, 2023
@cortisiko cortisiko removed the regression-RC DEPRECATED: Please use "regresssion-RC-x.y.z" label instead label Mar 17, 2023
@Cal-L
Copy link
Copy Markdown
Contributor Author

Cal-L commented Mar 17, 2023

@chrisleewilcox @cortisiko This issue might also be related to another one that Kat posted in Slack. If we have the bandwidth, could we quickly verify it as well?
https://consensys.slack.com/archives/G8RSKCNCD/p1676917311594709

@sethkfman sethkfman added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 17, 2023
@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 17, 2023
cortisiko
cortisiko approved these changes Mar 17, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

🌮 🌮 🌮 🌮

@cortisiko cortisiko added QA Passed QA testing has been completed and passed and removed QA in Progress QA has started on the feature. labels Mar 17, 2023
@cortisiko
Copy link
Copy Markdown
Member

cortisiko commented Mar 17, 2023

@Cal-L for the issue you linked, we need to triage it in our next bug triaging session. The ticket for the issue you linked is here

@cortisiko cortisiko merged commit d484d36 into release/6.2.0 Mar 17, 2023
@cortisiko cortisiko deleted the fix/720-srp-confirm-screen branch March 17, 2023 23:38
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2023
@gauthierpetetin gauthierpetetin added the team-mobile-ux Mobile UX team label Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cortisiko cortisiko cortisiko approved these changes

@tommasini tommasini tommasini approved these changes

+1 more reviewer

@sethkfman sethkfman sethkfman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

QA Passed QA testing has been completed and passed release-6.2.0 Issue or pull request that will be included in release 6.2.0 team-mobile-ux Mobile UX team type-bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Cal-L @cortisiko @sethkfman @tommasini @gauthierpetetin