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

Accounts V2: Fix issues reported by rkapka #6725

Merged
merged 25 commits into from
Jul 28, 2020
Merged

Conversation

0xKiwi
Copy link
Contributor

@0xKiwi 0xKiwi commented Jul 26, 2020

What type of PR is this?
Bug fix and cleanup

What does this PR do? Why is it needed?
Part of #6723, this solves some of the issues reported by rkapka. Issues includes:
--skip-mnemonic-confirm being on accounts-v2 create help
--password-file flag not being allowed in runtime
Allowing "y" or "Y" for prompt confirmation entry.
Cleaning up errors for invalid words entered for mnemonics, or just entirely invalid mnemonics
Fixing formatting of accounts list from derived vs. non-hd
Solves some formatting and test issues for logs
Adds an error if you try to import non-HD accounts into a HD wallet

Also fixes a bug with password file entry

@0xKiwi 0xKiwi requested a review from a team as a code owner July 26, 2020 08:34
@0xKiwi 0xKiwi added Ready For Review A pull request ready for code review AccountsV2 labels Jul 26, 2020
@codecov
Copy link

codecov bot commented Jul 26, 2020

Codecov Report

Merging #6725 into master will increase coverage by 2.12%.
The diff coverage is 73.31%.

@@            Coverage Diff             @@
##           master    #6725      +/-   ##
==========================================
+ Coverage   60.07%   62.19%   +2.12%     
==========================================
  Files         323      384      +61     
  Lines       27422    29465    +2043     
==========================================
+ Hits        16473    18327    +1854     
+ Misses       8733     8544     -189     
- Partials     2216     2594     +378     

@rkapka
Copy link
Contributor

rkapka commented Jul 26, 2020

The referenced issue has been merged into #6715.

fmt.Printf("%s %s\n", au.BrightCyan("[created at]").Bold(), humanize.Time(unixTimestamp))

fmt.Printf("%s | %s\n", au.BrightGreen(accountNames[i]).Bold(), humanize.Time(unixTimestamp))
fmt.Printf("%s %#x\n", au.BrightMagenta("[validating public key]").Bold(), pubKeys[i])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should abstract this logging into a shared function if possible

return errors.Wrap(err, "could not check if wallet directory exists")
}
if errors.Is(err, ErrWalletExists) {
return ErrWalletExists
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just returning the wallet exists error here so it doesn't pass through could not check if wallet directory exists

@prylabs-bulldozer prylabs-bulldozer bot merged commit ee1addd into master Jul 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-v2-runtime branch July 28, 2020 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants