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

goal: added "wallet rename" #6161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tasosbit
Copy link

Added a goal wrapper to the KMD rename wallet API

Usage: goal wallet rename [current wallet name] [new wallet name]

@tasosbit tasosbit mentioned this pull request Oct 31, 2024
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 3.22581% with 30 lines in your changes missing coverage. Please review.

Project coverage is 56.25%. Comparing base (eff5fb4) to head (b1f4201).

Files with missing lines Patch % Lines
cmd/goal/wallet.go 4.34% 22 Missing ⚠️
libgoal/wallets.go 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6161      +/-   ##
==========================================
- Coverage   56.29%   56.25%   -0.04%     
==========================================
  Files         494      494              
  Lines       69958    69989      +31     
==========================================
- Hits        39381    39373       -8     
- Misses      27907    27940      +33     
- Partials     2670     2676       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Was it tested? consider adding a new e2e expect test

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

I'm inclined to accept even without expect script test. I find those expect scripts painful to read/write, and to me this is essentially "UI", we need not be as concerned about perfect correctness here as we try to be on the protocol. This change amounts to little more than a prepackaged curl command.

Comment on lines +218 to +224
if wid == nil {
reportErrorf(errorCouldntFindWallet, string(walletName))
}

if err != nil {
reportErrorf(errorCouldntRenameWallet, err)
}
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 you should check err first, so you get the more detailed error message.

Comment on lines +230 to +232
if bytes.Equal(walletName, newWalletName) {
reportErrorf(errorCouldntRenameWallet, "new name is identical to current name")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like you might as well do this test before interacting with kmd at all.

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

Successfully merging this pull request may close these issues.

3 participants