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

Accounts Revamp Fixes: "Overall" Wallet Improvements #6736

Merged
merged 30 commits into from
Jul 29, 2020
Merged

Accounts Revamp Fixes: "Overall" Wallet Improvements #6736

merged 30 commits into from
Jul 29, 2020

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Jul 27, 2020 •
edited
Loading

Bug fix

What does this PR do? Why is it needed?

  • Changes the wallet dir default path to not be a hidden directory
  • add WalletPasswordsDirFlag to edit-config
  • It would be nice if the wallet's password path could be saved as part of the wallet's metadata. Typing this path over and over is super annoying.
  • Whenever I do not provide at least one flag out of the 4 gRPC configuration flags during create or edit-config, I am prompted for all 4 configuration values and I have to pass them again
  • Add a --wallet-password-file and a --account-password-file flag
  • I don't understand how to use the --password-flle flag based on the help available in the console.
  • I think it should say exported accounts instead of exported wallet. It would also be nice if the default path was dynamically adjusted to reflect the path from the --wallet-dir flag.
  • Why does the export command contain a --passwords-dir flag when the passwords directory is not exported? Maybe it should be? (Nishant: I dont think we should be exporting passwords in batch, imo just exporting the wallets should be ok. Also
  • There is a small difference in formatting between list for a non-HD wallet and an HD wallet. Accounts V2: Fix issues reported by rkapka #6725
  • Account slist with no wallet shouldn't panic
  • Panics in https://gist.github.com/prestonvanloon/48b134de6c000acaf01671249ac3aa94
  • Import creates a new wallet if none exists

Which issues(s) does this PR fix?

Part of #6715

Other notes for review

@rkapka rkapka mentioned this pull request Jul 27, 2020
Closed
35 tasks
@codecov
Copy link

codecov bot commented Jul 28, 2020 •
edited
Loading

Codecov Report

Merging #6736 into master will increase coverage by 2.19%.
The diff coverage is 73.59%.

@@            Coverage Diff             @@
##           master    #6736      +/-   ##
==========================================
+ Coverage   60.07%   62.27%   +2.19%     
==========================================
  Files         323      384      +61     
  Lines       27422    29823    +2401     
==========================================
+ Hits        16473    18571    +2098     
+ Misses       8733     8593     -140     
- Partials     2216     2659     +443

@rauljordan rauljordan self-assigned this Jul 28, 2020
@rauljordan rauljordan added AccountsV2 Ready For Review A pull request ready for code review labels Jul 28, 2020
@rauljordan rauljordan marked this pull request as ready for review July 28, 2020 21:49
@rauljordan rauljordan requested a review from a team as a code owner July 28, 2020 21:49
0xKiwi
0xKiwi reviewed Jul 28, 2020
View reviewed changes
validator/accounts/v2/accounts_import.go Outdated Show resolved Hide resolved
validator/accounts/v2/wallet.go
Comment on lines +52 to 54
walletDir string
accountsPath string
passwordsDir string
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the difference between walletDir and accountsPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the wallet path is the path of the prysm-wallet-v2, and the accounts path is deeper within that directory

validator/accounts/v2/wallet_create.go
@@ -15,53 +15,55 @@ import (
// CreateWallet from user input with a desired keymanager. If a
// wallet already exists in the path, it suggests the user alternatives
// such as how to edit their existing wallet configuration.
func CreateWallet(cliCtx *cli.Context) error {
func CreateWallet(cliCtx *cli.Context) (*Wallet, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this returned wallet used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used as part of account create or import

0xKiwi
0xKiwi reviewed Jul 28, 2020
View reviewed changes
validator/accounts/v2/accounts_create.go Outdated Show resolved Hide resolved
validator/flags/flags.go Outdated Show resolved Hide resolved
rauljordan and others added 3 commits July 28, 2020 17:58
@rauljordan @0xKiwi
Update validator/accounts/v2/accounts_create.go
cfc7a90 
Co-authored-by: Ivan Martinez <ivanthegreatdev@gmail.com>
@rauljordan
works
0c05bdb
@rauljordan
Merge branch 'wallet-improvements-overall' of github.com:prysmaticlab…
71093ed 
…s/prysm into wallet-improvements-overall
prestonvanloon
prestonvanloon requested changes Jul 28, 2020
View reviewed changes
validator/accounts/v2/accounts_create.go Outdated Show resolved Hide resolved
validator/accounts/v2/accounts_export.go Outdated
wallet, err := OpenWallet(cliCtx)
if err != nil {
if errors.Is(err, ErrNoWalletFound) {
return errors.Wrap(err, "no wallet found at path, create a new wallet with wallet-v2 create")
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is the export code. Instead of directing the user to create a wallet, maybe we could just inform them there is nothing to export?

validator/accounts/v2/accounts_import.go
if err != nil {
return errors.Wrap(err, "could not read keymanager kind for existing wallet")
ctx := context.Background()
wallet, err := createOrOpenWallet(cliCtx, func(cliCtx *cli.Context) (*Wallet, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you add a test case with where no wallet exists?

validator/accounts/v2/prompt.go Outdated Show resolved Hide resolved
validator/accounts/v2/wallet.go Show resolved Hide resolved
validator/accounts/v2/wallet_edit.go Outdated
return errors.Wrap(err, "could not unmarshal config")
}
log.Infof("Current configuration")
fmt.Printf("%s\n", cfg)
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm! Seems out of place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prints out the current configuration to stdout. Left a comment to clarify

validator/accounts/v2/wallet_edit.go Outdated Show resolved Hide resolved
@rauljordan rauljordan merged commit 9d08ba4 into master Jul 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the wallet-improvements-overall branch July 29, 2020 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xKiwi 0xKiwi 0xKiwi left review comments

@prestonvanloon prestonvanloon prestonvanloon approved these changes

@rkapka rkapka Awaiting requested review from rkapka

@farazdagi farazdagi Awaiting requested review from farazdagi

@nisdas nisdas Awaiting requested review from nisdas

@shayzluf shayzluf Awaiting requested review from shayzluf

@terencechain terencechain Awaiting requested review from terencechain

Assignees

@rauljordan rauljordan

Labels
Ready For Review A pull request ready for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rauljordan @0xKiwi @prestonvanloon @shayzluf