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

Wallet rename #11652

Merged
merged 79 commits into from
Jan 12, 2024
Merged

Wallet rename #11652

merged 79 commits into from
Jan 12, 2024

Conversation

SuperJMN
Copy link
Collaborator

@SuperJMN SuperJMN commented Oct 9, 2023

This PR allows renaming a wallet by going into Wallet Settings

WalletWasabi.Fluent.Desktop_qgvbjQVA2c.mp4

I'm trying to keep things as simple as I can with this.
Will require thorough testing.

Closes #3527

@SuperJMN SuperJMN marked this pull request as draft October 9, 2023 09:05
@turbolay
Copy link
Collaborator

I had an idea while reading the code, I'm sharing as maybe it's interesting (maybe it isn't):

Instead of introducing Id/Name as you are doing, we could validate the new name in the UI and schedule the execution of a batch script after the software's termination that will rename the file directly in the OS. We display to the user, "The wallet will be renamed next time you'll relaunch the app".

It's obviously a hacky solution, but it avoids changing the reliance on the name as an ID (which maybe is not even good in the first place soo....)

@SuperJMN SuperJMN marked this pull request as ready for review October 11, 2023 20:38
@SuperJMN SuperJMN self-assigned this Oct 11, 2023
@SuperJMN SuperJMN mentioned this pull request Oct 17, 2023
Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

the Rename button is still clickable while it is not visible

@soosr
Copy link
Collaborator

soosr commented Nov 6, 2023

Backup

Taking a look at the backup logic, it doesn't care if there is already a different wallet with the same file name. It just overrides it.
So when the user renames a wallet, you can simply rename the corresponding file in the backup directory (if there is any).

https://github.com/zkSNACKs/WalletWasabi/blob/7325103413d7db5fb8d4bd444d806ccfae062f49/WalletWasabi/Wallets/WalletManager.cs#L313-L321

UX

I think we should introduce a separate dialog for renaming the wallet. Similar to how the user can edit the label of an address. With this you could validate the input and only enable the Continue button, when the name is valid, also the user would still have the opportunity the cancel the operation in a friendlier way.

So in settings:

Wallet name             [Rename]

Which would pop a compact dialog.

Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

fwiw: I just also tested the RPC with this PR, works fine.
also tested renaming in the GUI and after that the RPC only recognizes the new wallet name

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

This doesn't work for me on Win10.

@SuperJMN
Copy link
Collaborator Author

This doesn't work for me on Win10.

Thanks for the heads up @yahiheb ! Should work now :)

WalletWasabi/Wallets/IWallet.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

tACK

  • Renaming multiple wallets.
  • Renaming during CJ critical phase.
  • Same name not allowed.
  • Special characters.

Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tack 7765369 could not break it.

two nits:

  • In the UI, why the large spacing between Name and the rename button?
  • normally when launching Wasabi it will automatically select the latest used wallet. It doesn't do that when having renamed the wallet. I guess that's ok.

@molnard molnard merged commit 0b1a8fa into WalletWasabi:master Jan 12, 2024
6 of 7 checks passed
@soosr
Copy link
Collaborator

soosr commented Jan 15, 2024

In the UI, why the large spacing between Name and the rename button?

That is the standard layout. Title to the left, actions buttons to the right. It will align better when more setting is added (like it is in the app settings).

normally when launching Wasabi it will automatically select the latest used wallet. It doesn't do that when having renamed the wallet. I guess that's ok.

Bug. #12253

@SuperJMN SuperJMN deleted the features/wallet-rename branch January 15, 2024 10:46
@yahiheb
Copy link
Collaborator

yahiheb commented Jan 15, 2024

That is the standard layout. Title to the left, actions buttons to the right.

The rename button is neither to the right nor in the middle.

WS1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename wallet
10 participants