Page MenuHomePhabricator

[Cashtab] Add ability to rename active wallet
ClosedPublic

Authored by kieran709 on Aug 3 2022, 14:44.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC2086478395e9: [Cashtab] Add ability to rename active wallet
Summary

Related to task T2132. Added logic to renameWallet in useWallet.js that checks if the wallet to be renamed is the currently active wallet, sets newWalletName for the currently active wallet. I did not include the delete wallet button for the currently active wallet, but this can be added if it is desired.

Test Plan

cd web/cashtab && npm start
navigate to the Configure screen
note that the saved wallets collapse is present even if there is only one saved wallet
Note that the currently active wallet now shows the add to address button & the edit name button
Edit the name of the currently active wallet
Observe that the active wallet's name has changed

From the wallet label component in Send, Airdrop, or Home screen, click the edit button,
Observe that the browser is routed to the Configure screen with the RenameWalletModal open,
Change the wallet name,
Observe that the wallet's name changes in the SW in local storage, the SW list on the configure screen, and in the wallet label component.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rename-current-wallet
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19765
Build 39249: Build Diffcashtab-tests
Build 39248: arc lint + arc unit

Event Timeline

kieran709 edited the summary of this revision. (Show Details)
bytesofman requested changes to this revision.Aug 3 2022, 16:45
bytesofman added inline comments.
web/cashtab/src/components/Configure/Configure.js
1586

why is wallet necessary now when it was not previously?

web/cashtab/src/hooks/useWallet.js
783

Consider the impact this will have if the user is changing the name of a saved wallet 🚨

You probably need a specific function for changing the name of the active wallet. It could be a simple wrapper, i.e. it could change wallet.name and also call renameWallet, which will update the data in the savedWallets object, which is necessary

Changed logic in renameWallet to account for the activeWallet's name being changed, the active wallet & the copy of the activeWallet in the SW list are updated with this new logic.

  1. Rename the exisitng renameWallet function to renameSavedWallet
  2. Create a new function renameActiveWallet in useWallet.js
  3. Pass wallet to this function as a parameter (along with oldName and newName)
  4. The new renameActiveWallet function should:
    • Verify that no savedWallet has the same name, just like renameSavedWallet does
    • Update savedWallets[i].name just like renameSavedWallet does
    • Also update wallet.name and save to localforage
    • setWallet(wallet) -- this will update the context of the active wallet

In addition to this, we will also want the 'edit' icon next to the wallet name on all screens, not just on the settings page. So, we don't need to change any logic about when the savedWallets dropdown appears.

web/cashtab/src/hooks/useWallet.js
749 ↗(On Diff #34744)

Good practice is to keep a function isolated to its parameters. Here, we are pulling in wallet from context.

This is "the same place" that Configure.js grabs it from, but it would be cleaner to deliver wallet as a parameter to this function.

At this point, though, I think we are creating enough complication in the difference between the logic of renaming a saved wallet and renaming the active wallet that there should be two distinct functions.

Responded to review feedback & updated snapshots. Changes planned.

kieran709 planned changes to this revision.EditedAug 30 2022, 17:43

need to add edit button to the wallet name throughout the app to maintain SW list rendering logic on the configure screen.

Added edit button to WalletLabel component which redirects to /configure and triggers the rename wallet modal.

Good work. Well thought through diff that understands the moving parts and solves the problem. A few clean-up items in the inline comments.

I'm seeing this error in the dev console when I follow these steps.

  1. npm start
  2. Click the 'edit' icon on the home page
  3. See this error in the dev console

image.png (503×568 px, 107 KB)

web/cashtab/src/components/Common/WalletLabel.js
4 ↗(On Diff #34775)

This should be done in CustomIcons.js

Update: I see this is how it's currently imported into Configure.js, along with some others

Please create a task to move all icon imports like this into CustomIcons.js and assign it to yourself. Don't fix it in this diff.

web/cashtab/src/components/Configure/Configure.js
796 ↗(On Diff #34775)

We shouldn't have two if blocks with mutually exclusive conditions.

That's what an else {} block is for.

This revision now requires changes to proceed.Aug 31 2022, 21:45
kieran709 edited the test plan for this revision. (Show Details)

Updated snapshots & rebased to master. Fixed console error, imported Edit icon from CustomIcons and refactored changeWalletName per review feedback.

This revision is now accepted and ready to land.Sep 1 2022, 20:45