-
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: add --qrcode flag to keys show command #18557
Conversation
WalkthroughThe code changes introduce a new feature to display a payment QR code in the terminal for a given key name or address, enhancing the Changes
Assessment against linked issues
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: 5
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- go.mod
- go.sum
Files selected for processing (3)
- client/keys/show.go (5 hunks)
- client/keys/show_test.go (1 hunks)
- testutil/ioutil.go (1 hunks)
Additional comments: 3
client/keys/show.go (2)
4-9: The import of the
qrterminal
package is correctly placed in the import block, following the convention of grouping third-party imports together. However, ensure that theerrors
package is not being used anymore since it's replaced byerrorsmod
fromcosmossdk.io/errors
. If it's not used, it should be removed to avoid unnecessary imports.31-35: The addition of the
flagQRCode
constant is consistent with the implementation of the new feature. It follows the established naming conventions and is well-documented.testutil/ioutil.go (1)
- 44-50:
The change to comment out
c.SetOut(io.Discard)
will cause the command's standard output to no longer be discarded during tests. This could lead to unintended side effects if the output is not handled or asserted properly in tests. Verify that this change is intentional and that all tests still behave as expected without discarding the standard output.
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.
lgtm, thank you! But I wouldn't call that a "payment qrcode". Maybe just address qrcode or smth. |
Okay, let's call it address qrcode😀 |
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 ignored due to filter (2)
- simapp/go.mod
- simapp/go.sum
Files selected for processing (1)
- client/keys/show_test.go (1 hunks)
Additional comments: 1
client/keys/show_test.go (1)
- 174-176:
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: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/keys/show.go (5 hunks)
Additional comments: 3
client/keys/show.go (3)
34-34:
The introduction of theflagQRCode
constant is consistent with the naming conventions and purpose of the pull request.51-54:
The flag descriptions forFlagPublicKey
,FlagDevice
,flagMultiSigThreshold
, andflagQRCode
are clear and provide the necessary information to the user.99-105:
The retrieval of flag values forisShowAddr
,isShowPubKey
,isShowDevice
, andisShowQRCode
is consistent with the existing pattern and correctly implemented.
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.
lgtm! 🚀
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.
Awesome, really cool. Thanks @levisyin
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
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)
- CHANGELOG.md (1 hunks)
- client/keys/show.go (5 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 43-43: The changelog entries are correctly formatted, include the necessary links to the pull requests, and accurately describe the new features and changes.
…smos-sdk into feat/payment-qrcode
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.
cool feature!
Lgtm!!
Description
Closes: #18556
simd keys show account1 --address=true --qrcode=true
Author Checklist
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
--qrcode
flag for thekeys show
command to display keys as a payment QR code.client.toml
, eliminating the need for the--from
flag during transactions.Enhancements
key_rotation_fee
parameter in thex/staking
module for fee calculation during key rotations.SubmitTestTx
helper method intestutil
for broadcasting test transactions in end-to-end tests.Tests
keys show
command with new test cases for QR code display and key management scenarios.Documentation