-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(cast): display public_key on wallet creation with "new" and "new-mnemonic" sub-commands #10600
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
base: master
Are you sure you want to change the base?
Conversation
232b8f5
to
90ca4e6
Compare
…-mnemonic" subcommands The method used to obtain public_key with the wallet is taken from "public-key" subcommand. Update tests to match the new outputs: - match pubkey - update redactions for pubkey and to match various spacings - use raw data for assertion in "wallet_mnemonic_from_entropy" to avoid conflict with redactions
90ca4e6
to
ffeefa2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, I think this makes sense (breaking change) @zerosnacks wdyt?
I think it is a bit verbose and for most use cases the level of detail seems unnecessary @mablr would you mind expanding on where you use the public keys? We already have the |
This feature was initially requested by @coffee-converter. @zerosnacks I agree with you and I shared the same concerns in comments on the related issue: #9748 (comment) & #9748 (comment). Some of the information is outdated, but the main idea is in line with your remark. After receiving feedback from @mattsse #9748 (comment), who assigned me to the issue, I finally implemented this feature. |
Hey @mablr, I see - thanks for the pointer! To my understanding @mattsse refers to In my opinion, I don't see a huge benefit to having this be logged, even at higher verbosity. I would be in favor of closing the ticket as |
I think you're right, limiting the pubkey display to a higher verbosity level (e.g. >1) is a good deal. |
Avoid breaking changes Some unit tests added for better coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, displaying on verbose lgtm! pending @zerosnacks review before merging
Motivation
Resolve #9748
Solution
The method used to obtain public_key with the wallet is taken from "public-key" sub-command.
Update tests to match the new outputs:
EDIT:
PR Checklist
Added Documentation-v
flag)