-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(cli): add acc-address by id cli and grpc gateway api #12199
feat(cli): add acc-address by id cli and grpc gateway api #12199
Conversation
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.
nice work!
1. Removed conv from str to bytes and bytes to string
@gsk967 can you look into the failing tests? |
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! Great work!
Looks like x/auth/migrations/v046/store_test.go
is empty though? We probably want a simple test or two there.
@alexanderbez @likhita-809 add state migration test cases , Please look into changes. |
x/auth/keeper/migrations.go
Outdated
} | ||
|
||
// V45_SetAccount implements V45_SetAccount | ||
// set the account without map to accAddr to accNumber |
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.
// set the account without map to accAddr to accNumber | |
// set the account without map to accAddr to accNumber. | |
// | |
// NOTE: This is used for testing purposes only. |
(cherry picked from commit 4385325)
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.
I left some suggestions for API improvements
@@ -24,6 +24,11 @@ service Query { | |||
option (google.api.http).get = "/cosmos/auth/v1beta1/accounts/{address}"; | |||
} | |||
|
|||
// AccountAddressByID returns account address based on account id | |||
rpc AccountAddressByID(QueryAccountAddressByIDRequest) returns (QueryAccountAddressByIDResponse) { |
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.
AccountAddressByID
seems like a weird name to me. Do people use the term "ID"?
I would call it AddressByAccountNumber
or AddressByAccNum
|
||
// QueryAccountAddressByIDRequest is the request type for AccountAddressByID rpc method | ||
message QueryAccountAddressByIDRequest{ | ||
int64 id = 1; |
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.
why is this int64, when account_number is uint64 everywhere?
@@ -24,6 +24,11 @@ service Query { | |||
option (google.api.http).get = "/cosmos/auth/v1beta1/accounts/{address}"; | |||
} | |||
|
|||
// AccountAddressByID returns account address based on account id |
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.
This is missing a Since proto comment
|
||
// QueryAccountAddressByIDResponse is the response type for AccountAddressByID rpc method | ||
message QueryAccountAddressByIDResponse { | ||
string account_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; |
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.
Would rename this field to address
simply, which makes it consistent with some other places
If we can't rename the proto fields (too late), then we can at least update the CLI command name and gRPC routes. |
Description
Closes: #12155
This pull request contains the address-by-id cli cmd and grpc gateway api request for account address retrieval by id
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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 change