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.

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

feat: Wipe wallet group #1331

Merged
merged 16 commits into from
Feb 28, 2024
Merged

feat: Wipe wallet group #1331

merged 16 commits into from
Feb 28, 2024

Conversation

BrodyHughes
Copy link
Member

@BrodyHughes BrodyHughes commented Feb 1, 2024

Fixes BX-1254
Figma link: https://www.figma.com/file/hzfwKOddfRmeVO4KUsG9wM/branch/bQjlkhAqO2w3nk7NJF5EKS/Rainbow-BX?type=design&node-id=15871-3647&mode=design&t=XOaSEV6pKSICFPvc-0

What changed (plus any additional context for devs):

  • added the ability to remove an entire wallet group via wallet details
  • should only show up for HD wallets atm
  • watched are uneffected
  • will follow up with the ability to unpair a ledger group next

Screen recordings / screenshots

POW:

Screen.Recording.2024-02-02.at.3.20.13.PM.mov

Copy link

github-actions bot commented Feb 1, 2024

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-feedae3dfc0d88b1784ee793f527cb914ee812de.zip
screenshots

Copy link

github-actions bot commented Feb 2, 2024

Here's the packed extension for this build:
rainbowbx-5fb8f260b50a7a52548571ffec7cf9e6b4115c09.zip

@BrodyHughes BrodyHughes changed the title [WIP] Brody/wallet wipe wallet group feat: Wipe wallet group Feb 2, 2024
Copy link

linear bot commented Feb 2, 2024

@BrodyHughes BrodyHughes marked this pull request as ready for review February 2, 2024 21:24
Copy link

github-actions bot commented Feb 2, 2024

Here's the packed extension for this build:
rainbowbx-1ad569401ce627877afab25839eb1b61bd9e3407.zip

Copy link
Collaborator

@DanielSinclair DanielSinclair left a comment

Choose a reason for hiding this comment

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

UI looks great! Noticed an issue where the Wallet Group would re-appear in the Wallets & Keys menu initially after completing the flow. The group would shift from "Backed Up" to the "Not backed up" state. Then when completing the flow again, it would be fully removed
Screenshot 2024-02-05 at 12 59 25 AM

Copy link

github-actions bot commented Feb 5, 2024

Here's the packed extension for this build:
rainbowbx-4423a174136f6daf85ea7d086219617ea45b72f7.zip

@BrodyHughes
Copy link
Member Author

UI looks great! Noticed an issue where the Wallet Group would re-appear in the Wallets & Keys menu initially after completing the flow. The group would shift from "Backed Up" to the "Not backed up" state. Then when completing the flow again, it would be fully removed Screenshot 2024-02-05 at 12 59 25 AM

cannot repro.

@DanielSinclair
Copy link
Collaborator

@BrodyHughes Found another issue where dapps connections persist after we remove a seed phrase, leading to these keychain errors. This might be true for "hide wallet" and "remove wallet" features too. I think we'll need to implement a watcher or logic to automatically disconnect dapps in these scenarios Screenshot 2024-02-06 at 9 47 02 PM

Copy link

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-f946144e6267445dde150092817e6d695b32f118.zip

Copy link

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-6b521740eeb89cf53daac8abefb611aeb893114d.zip

}

export default function WarningInfo({
iconAndCopyList,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this don't have to be a prop either, seems like a very specific component no need to abstract it, or change the name and abstract to use in the other similar screens?

Copy link
Contributor

@greg-schrammel greg-schrammel left a comment

Choose a reason for hiding this comment

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

🐐🐐🐐

Copy link

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-be0e80e6a429a2760fb7624e7b22ed6968b9fb8f.zip
screenshots

Copy link

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-b8fdde379981219567011c3f10208ff60e5c8045.zip
screenshots

@@ -22,25 +18,18 @@ import * as wallet from '../../../handlers/wallet';
const t = (s: string) =>
i18n.t(s, { scope: 'settings.privacy_and_security.wallets_and_keys' });

async function handleWipeWallet() {
Copy link
Member Author

Choose a reason for hiding this comment

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

could maybe inline this now bc it's way smaller now with your refactor. idk what your preference is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like creating functions outside the component when possible, so it won't be recreated on every rerender, but it's not a huge performance gain here I'd go with whatever you think is more readable, up to you

Copy link

Here's the packed extension for this build:
rainbowbx-1f5704b22cb042d26f30fc0915b9bd1fea2a5db6.zip

Copy link

Here's the packed extension for this build:
rainbowbx-57ad9b9c620916a0af2091ec50c4216b52547eb1.zip

Copy link

Here's the packed extension for this build:
rainbowbx-4ea5e330eec1937eb73c0af879d9e7b0f50c1e04.zip

Copy link
Member Author

@BrodyHughes BrodyHughes left a comment

Choose a reason for hiding this comment

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

tested this with greg's changes and it lgtm

Copy link
Collaborator

@DanielSinclair DanielSinclair 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 to me!

@DanielSinclair DanielSinclair added this pull request to the merge queue Feb 28, 2024
Merged via the queue into master with commit 2757cb6 Feb 28, 2024
17 checks passed
@DanielSinclair DanielSinclair deleted the brody/wallet-wipe-wallet-group branch February 28, 2024 21:12
Copy link

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-2757cb61c4cefc637da48e029bc2d858e9867787.zip

greg-schrammel added a commit that referenced this pull request Mar 27, 2024
Co-authored-by: Daniel Sinclair <[email protected]>
Co-authored-by: gregs <[email protected]>
@BrodyHughes BrodyHughes restored the brody/wallet-wipe-wallet-group branch November 27, 2024 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants