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

Remove sync with extension from settings #2044

Merged
merged 12 commits into from
Jan 12, 2021
Merged

Remove sync with extension from settings #2044

merged 12 commits into from
Jan 12, 2021

Conversation

rickycodes
Copy link
Contributor

@rickycodes rickycodes commented Dec 4, 2020 •
edited by omnat
Loading

Description

Remove sync and update QR scan feedback as per bellow:

Checklist

  • Remove "Sync with extension" option from Settings
  • Error handling when user scans the Sync with mobile QR code from extension wallet

Copy suggestion: “In order to Sync your MetaMask mobile wallet with your extension wallet, please re-install the app and choose the 'Import your extension wallet' option at onboarding.
Make sure your have your current wallet seed phrase safely backed up before uninstalling. Backup now (button that redirects to Settings > Security & Privacy)

UPDATE: This is going to be a different PR

  • Remove the ability to replace wallet (remove "Go back" button when user logs out)
    Consider adding copy somewhere to let user know that if they want to create/import new wallet, they need to re-install the app.

Issue

Resolves #1950

@rickycodes rickycodes force-pushed the feature/remove-sync-from-wallet branch from e5ebb79 to 1934176 Compare December 10, 2020 17:20
@rickycodes rickycodes marked this pull request as ready for review December 10, 2020 17:30
@rickycodes rickycodes requested a review from a team as a code owner December 10, 2020 17:30
@rickycodes rickycodes added needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Dec 10, 2020
estebanmino
estebanmino reviewed Dec 10, 2020
View reviewed changes
app/components/Views/Login/index.js Outdated
@@ -400,7 +407,8 @@ class Login extends PureComponent {

const mapStateToProps = state => ({
passwordSet: state.user.passwordSet,
selectedAddress: state.engine.backgroundState.PreferencesController.selectedAddress
selectedAddress: state.engine.backgroundState.PreferencesController.selectedAddress,
isDev: process.env?.NODE_ENV === DEVELOPMENT
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be moved as constant outside the component?

estebanmino
estebanmino approved these changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

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

👌

@rickycodes rickycodes force-pushed the feature/remove-sync-from-wallet branch from b250c93 to f898aa8 Compare December 10, 2020 19:00
@ibrahimtaveras00 ibrahimtaveras00 added needs-design Feature that requires UI/UX design and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Dec 10, 2020
ibrahimtaveras00
ibrahimtaveras00 suggested changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

@cjeria @omnat Possible re-design on the removal of "Go Back"
cc @rickycodes

@ibrahimtaveras00 ibrahimtaveras00 removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Dec 10, 2020
@ibrahimtaveras00
Copy link
Contributor

ibrahimtaveras00 commented Dec 10, 2020 •
edited by rickycodes
Loading

Also bump security up in the settings list

we handled this in #2042

@omnat
Copy link
Contributor

omnat commented Dec 10, 2020 •
edited
Loading

@cjeria syncing with Ibrahim on this one. We are talking how this direction might still make sense and doesn't require re-design. Happy to sync on it.
Let's also keep @jakehaugen in loop here as we discussed this in a Secure UI reading call too. Where a lot of this direction came up.

@cjeria
Copy link

cjeria commented Dec 10, 2020 •
edited
Loading

Here is a suggested design for restoring the wallet using seed phrase on the login screen that mirrors what the extension does today

image

Prototype

@cjeria
Copy link

cjeria commented Dec 14, 2020 •
edited
Loading

Here's another idea for removing the ability to restore from login and instead include direction for what do to if the user is locked out cc @omnat This copy could use refining.
image

@rickycodes rickycodes force-pushed the feature/remove-sync-from-wallet branch from b92c934 to c3278f2 Compare January 11, 2021 17:23
@rickycodes rickycodes added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-design Feature that requires UI/UX design labels Jan 11, 2021
@rickycodes rickycodes force-pushed the feature/remove-sync-from-wallet branch from 1f8bde3 to f42edf0 Compare January 12, 2021 22:41
ibrahimtaveras00
ibrahimtaveras00 approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

QA Passed 👍🏽

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 12, 2021
@rickycodes rickycodes merged commit cef4f30 into develop Jan 12, 2021
@rickycodes rickycodes deleted the feature/remove-sync-from-wallet branch January 12, 2021 23:00
@omnat omnat linked an issue Mar 4, 2021 that may be closed by this pull request
test case #2342
Closed
2 tasks
@omnat omnat removed a link to an issue Mar 4, 2021
test case #2342
Closed
2 tasks
rickycodes added a commit that referenced this pull request Jan 31, 2022
@rickycodes
Remove sync with extension from settings (#2044)
15f4c17 
* remove sync with extension from settings

* Update attempting_sync_from_wallet_error

* Update tests

* Update translation

* Update attempting_sync_from_wallet_error text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

@estebanmino estebanmino estebanmino approved these changes

Assignees

@cjeria cjeria

Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove ability to replace wallet
5 participants
@rickycodes @ibrahimtaveras00 @omnat @cjeria @estebanmino