-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(client/keys): support display discreetly for keys add
#18663
feat(client/keys): support display discreetly for keys add
#18663
Conversation
WalkthroughWalkthroughThe changes involve enhancing security and usability features in a Go application. A new flag for discreet mnemonic display has been added, and the handling of mnemonic backup has been reworked. A function to print sensitive information discreetly has been introduced. Additionally, improvements have been made to querying governance proposals and providing address codecs in the client context. The Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- go.mod
- go.sum
Files selected for processing (2)
- client/keys/add.go (1 hunks)
- client/keys/utils.go (2 hunks)
Additional comments: 4
client/keys/add.go (3)
357-363: The implementation of the
displayDiscreetly
function call seems correct, and it is appropriately handling the error by returning a formatted error message if the function fails. This aligns with the PR's objective to discreetly display the mnemonic phrase.357-363: Please verify that the
displayDiscreetly
function is declared in theclient/keys/utils.go
file and is part of the public API as intended by the PR objectives.357-363: Please verify that the test
Test_runAddCmdBasic
exists and has been updated to cover the new functionality of discreetly displaying the mnemonic phrase.client/keys/utils.go (1)
- 76-88: The implementation of
displayDiscreetly
function appears to correctly handle the discreet display of sensitive information. However, ensure that error handling is consistent with the rest of the codebase, especially for thefmt.Scanln()
call which might need to handle specific cases like EOF or interrupted input.
981a197: you should run |
Thanks, I think I should update go.mod in simapp/go.mod instead of ./go.mod |
The script should do it right. If you have a go.work somewhere, please delete it and delete simapp/gomod2nix.toml and re-run the script. The diff shouldn't be that big. |
Ok,thank you very much🙏. There is indeed a go.work file, I will delete it and retry again. |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- simapp/go.mod
- simapp/go.sum
- simapp/gomod2nix.toml
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- client/keys/utils.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- client/keys/utils.go
Additional comments: 1
CHANGELOG.md (1)
- 59-59: The changes listed in the hunk for
CHANGELOG.md
correctly reflect the improvements and features added to the codebase as described in the summary. The links to the pull requests provide a clear reference for further details on each change.
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.
tACK: https://asciinema.org/a/ngiFJX2BjNMiGc2uQ9zIfggRt
Nice feature, maybe it will break some scripts however, can we add a flag to disable it?
keys add
keys add
Co-authored-by: Julien Robert <julien@rbrt.fr>
Sorry for mis-operate... |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- client/keys/add.go (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Additional comments: 5
client/keys/add.go (5)
39-39: The addition of the
flagIndiscreet
constant aligns with the PR's objective to enhance privacy and security during the mnemonic display process.333-335: The replacement of the
noBackup
flag withshowMnemonic
andshowMnemonicDiscreetly
variables is consistent with the PR's objective to provide a discreet mnemonic display option.348-351: The modification of the
printCreate
function to handle theshowMnemonicDiscreetly
variable aligns with the PR's objective to allow for a discreet display of the mnemonic.363-366: The use of the
displayDiscreetly
function withinprintCreate
whenshowMnemonicDiscreetly
is true is consistent with the PR's objective to discreetly display the mnemonic.330-345: The updates to the
runAddCmd
function to handle theshowMnemonicDiscreetly
variable align with the PR's objective to introduce a discreet mnemonic display feature.
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.
utACK!
client/keys/utils.go
Outdated
if _, err = fmt.Scanln(); err != nil { | ||
return err | ||
} | ||
return nil |
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.
nit: since we're not wrapping the error with context (maybe we should), then you can just simply return the result
if _, err = fmt.Scanln(); err != nil { | |
return err | |
} | |
return nil | |
return fmt.Scanln() |
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.
@alexanderbez Hi, sir. As fmt.Scanln
signature is func Scanln(a ...any) (n int, err error)
, but I just wanna return "error or not" for func displayDiscreetly
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.
tACK! +1 for bez comments.
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- client/keys/add.go (9 hunks)
- client/keys/utils.go (2 hunks)
Additional comments: 10
client/keys/add.go (7)
39-39: The addition of the
flagIndiscreet
constant is consistent with the PR's objective to introduce discreet mnemonic display.88-88: The PR summary mentioned the removal of
flagNoBackup
, but it is still present. Please verify if this flag should be removed or if the summary is incorrect.334-342: The logic for handling the display of mnemonics has been updated to use
showMnemonic
andshowMnemonicIndiscreetly
. Ensure that the new logic correctly implements the discreet display of mnemonics as intended.349-349: The
printCreate
function has been modified to includeshowMnemonic
andshowMnemonicIndiscreetly
parameters. Confirm that these parameters are used correctly within the function to handle mnemonic display as per the new feature.369-371: The
printDiscreetly
function is called here, but its implementation is not provided. Please ensure that the function exists and correctly implements discreet printing as intended.364-371: The logic for discreet and indiscreet mnemonic display in the
printCreate
function appears to be correctly implemented. Ensure that the discreet path does not leak sensitive information to the terminal's main screen.366-366: The error handling for mnemonic display in the
printCreate
function is clear and provides context, which is good for debugging purposes.client/keys/utils.go (3)
8-8: The addition of the
termenv
package is appropriate for the functionality being introduced.76-88: The
printDiscreetly
function correctly implements the discreet display of sensitive information using an alternate screen, which aligns with the PR's objective to enhance security and privacy.76-88: Verify that the existing test
Test_runAddCmdBasic
adequately covers the newprintDiscreetly
function, as claimed in the PR.
Could you please fix the conflicts? |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- go.mod
- x/accounts/go.mod
Files selected for processing (1)
- client/keys/add.go (9 hunks)
Additional comments: 7
client/keys/add.go (7)
39-39: The addition of the
flagIndiscreet
constant is consistent with the PR's objective to support discreet mnemonic display.88-88: The
flagIndiscreet
flag is added to the command flags with a clear description. It's important to ensure that the flag's behavior is correctly implemented in the rest of the code.334-336: The
showMnemonic
andshowMnemonicIndiscreetly
variables are introduced to replace thenoBackup
variable. This change is in line with the PR's objective to provide more granular control over mnemonic display.342-342: The
showMnemonicIndiscreetly
variable is explicitly set tofalse
when recovering a key, which is a good security practice to prevent accidental display of sensitive information.349-349: The
printCreate
function signature is modified to acceptshowMnemonic
andshowMnemonicIndiscreetly
parameters. It's crucial to review the implementation of this function to ensure it handles these parameters correctly.364-371: The logic to discreetly print the mnemonic phrase is added. It's important to verify that the
printDiscreetly
function is implemented correctly inclient/keys/utils.go
and that it uses thetermenv
package as intended to prevent the mnemonic from being displayed on the terminal's main screen.374-374: The
default
case in the switch statement foroutputFormat
provides error handling for invalid formats. This is a good practice to ensure that the function can handle unexpected input gracefully.
* feat: secp256k1 public key constant time (cosmos#18026) Signed-off-by: bizk <santiago.yanzon1999@gmail.com> * chore: Fixed changelog duplicated items (cosmos#18628) * adr: Un-Ordered Transaction Inclusion (cosmos#18553) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * docs: lint ADR-070 (cosmos#18634) * fix(baseapp)!: postHandler should run regardless of result (cosmos#18627) * docs: fix typos in adr-007-specialization-groups.md (cosmos#18635) * chore: alphabetize labels (cosmos#18640) * docs(x/circuit): add note on ante handler (cosmos#18637) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * refactor(store/v2): updates from integration (cosmos#18633) * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(store/v2): snapshot manager (cosmos#18458) * chore(client/v2): fix typos in the README.md (cosmos#18657) * fix(baseapp): protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654) Co-authored-by: unknown unknown <unknown@unknown> * chore: fix several minor typos (cosmos#18660) * chore(tools/confix/cmd): fix typo in view.go (cosmos#18659) * refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655) * feat(accounts): use gogoproto API instead of protov2. (cosmos#18653) Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651) * build(deps): Bump actions/stale from 8 to 9 (cosmos#18656) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): fix typos & wording in docs (cosmos#18667) * chore: fix several typos. (cosmos#18666) * feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Marko <marko@baricevic.me> * feat(store/v2): add SetInitialVersion in SC (cosmos#18665) * feat(client/keys): support display discreetly for `keys add` (cosmos#18663) Co-authored-by: Julien Robert <julien@rbrt.fr> * ci: add misspell action (cosmos#18671) * chore: typos fix by misspell-fixer (cosmos#18683) Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> * chore: add v0.50.2 changelog to main (cosmos#18682) * build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor(bank): remove .String() calls (cosmos#18175) Co-authored-by: Facundo <facundomedica@gmail.com> * ci: use codespell instead of misspell-fixer (cosmos#18686) Co-authored-by: Marko <marbar3778@yahoo.com> * feat(gov): add proposal types and spam votes (cosmos#18532) * feat(accounts): use account number as state prefix for account state (cosmos#18664) Co-authored-by: unknown unknown <unknown@unknown> * chore: typos fixes by cosmos-sdk bot (cosmos#18689) Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: marbar3778 <marbar3778@yahoo.com> * feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688) * refactor: remove panic usage in keeper methods (cosmos#18636) * ci: rename pr name in misspell job (cosmos#18693) Co-authored-by: Marko <marko@baricevic.me> * build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/keys): support display discreetly for keys export (cosmos#18684) * feat(x/gov): better gov genesis validation (cosmos#18707) --------- Signed-off-by: bizk <santiago.yanzon1999@gmail.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Carlos Santiago Yanzon <27785807+bizk@users.noreply.github.com> Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: Akaonetwo <107335783+Akare123@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: dreamweaverxyz <153101746+dreamweaverxyz@users.noreply.github.com> Co-authored-by: Pioua <136521243+dzizazda@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cool-developer <51834436+cool-develope@users.noreply.github.com> Co-authored-by: leonarddt05 <139609434+leonarddt05@users.noreply.github.com> Co-authored-by: testinginprod <98415576+testinginprod@users.noreply.github.com> Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: Sukey <35202440+sukey2008@users.noreply.github.com> Co-authored-by: axie <152680487+azukiboy@users.noreply.github.com> Co-authored-by: Luke Ma <867273263@qq.com> Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com> Co-authored-by: 0xn4de <109149873+0xn4de@users.noreply.github.com> Co-authored-by: hattizai <150505746+hattizai@users.noreply.github.com> Co-authored-by: Devon Bear <itsdevbear@berachain.com> Co-authored-by: Marko <marko@baricevic.me> Co-authored-by: Halimao <1065621723@qq.com> Co-authored-by: Cosmos SDK <113218068+github-prbot@users.noreply.github.com> Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com> Co-authored-by: Facundo <facundomedica@gmail.com> Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>
Description
Closes: #18662
This PR add support for displaying mnemonic discreetly.
Testing and Verifying
This change is already covered by existing tests, such as Test_runAddCmdBasic.
Summary by CodeRabbit
New Features
flagIndiscreet
in the constants section.<appd> keys add
inclient/keys
.Improvements
<appd> q gov proposer
by directly querying a proposal instead of tx events inx/gov
.Bug Fixes
DecCoins
to prevent errors.Refactor
Documentation