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

refactor: Improve and clarify API around AccountAddressById #13460

Merged
merged 9 commits into from Oct 6, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Expand Up @@ -143,8 +143,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (codec) [#12964](https://github.com/cosmos/cosmos-sdk/pull/12964) `ProtoCodec.MarshalInterface` now returns an error when serializing unregistered types and a subsequent `ProtoCodec.UnmarshalInterface` would fail.
* (x/staking) [#12973](https://github.com/cosmos/cosmos-sdk/pull/12973) Removed `stakingkeeper.RandomValidator`. Use `testutil.RandSliceElem(r, sk.GetAllValidators(ctx))` instead.
* (x/gov) [13160](https://github.com/cosmos/cosmos-sdk/pull/13160) Remove custom marshaling of proposl and voteoption.
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
* (x/auth) [13411](https://github.com/cosmos/cosmos-sdk/pull/13411) Change account id to use uint64 in `AccountAddressByID` grpc query.
* (types) [13430](https://github.com/cosmos/cosmos-sdk/pull/13430) Remove unused code `ResponseCheckTx` and `ResponseDeliverTx`
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
* (auth) [#13460](https://github.com/cosmos/cosmos-sdk/pull/13460) The `q auth address-by-id` CLI command has been renamed to `q auth address-by-acc-num` to be more explicit. However, the old `address-by-id` version is still kept as an alias, for backwards compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this go under CLI Breaking Changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually kept address-by-id as a CLI command alias, for backwards compat


### CLI Breaking Changes

Expand Down
33 changes: 22 additions & 11 deletions api/cosmos/auth/v1beta1/query.pulsar.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions api/cosmos/auth/v1beta1/query_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions api/cosmos/distribution/v1beta1/query_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 8 additions & 8 deletions api/cosmos/group/v1/types.pulsar.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion client/grpc/tmservice/query.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 15 additions & 2 deletions proto/cosmos/auth/v1beta1/query.proto
Expand Up @@ -30,7 +30,9 @@ service Query {
option (google.api.http).get = "/cosmos/auth/v1beta1/accounts/{address}";
}

// AccountAddressByID returns account address based on account id
// AccountAddressByID returns account address based on account number.
//
// Since: cosmos-sdk 0.46.2
rpc AccountAddressByID(QueryAccountAddressByIDRequest) returns (QueryAccountAddressByIDResponse) {
option (cosmos.query.v1.module_query_safe) = true;
option (google.api.http).get = "/cosmos/auth/v1beta1/address_by_id/{id}";
Expand Down Expand Up @@ -180,23 +182,34 @@ message AddressStringToBytesResponse {
}

// QueryAccountAddressByIDRequest is the request type for AccountAddressByID rpc method
//
// Since: cosmos-sdk 0.46.2
message QueryAccountAddressByIDRequest {
uint64 id = 1;
// id is the account number of the address to be queried. This field
// should have been an uint64 (like all account numbers), and will be
// updated to uint64 in a future version of the auth query.
int64 id = 1;
}

// QueryAccountAddressByIDResponse is the response type for AccountAddressByID rpc method
//
// Since: cosmos-sdk 0.46.2
message QueryAccountAddressByIDResponse {
string account_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
}

// QueryAccountInfoRequest is the Query/AccountInfo request type.
//
// Since: cosmos-sdk 0.47
message QueryAccountInfoRequest {

// address is the account address string.
string address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
}

// QueryAccountInfoResponse is the Query/AccountInfo response type.
//
// Since: cosmos-sdk 0.47
message QueryAccountInfoResponse {

// info is the account info which is represented by BaseAccount.
Expand Down
2 changes: 1 addition & 1 deletion store/types/commit_info.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 7 additions & 6 deletions x/auth/client/cli/query.go
Expand Up @@ -129,23 +129,24 @@ func GetAccountCmd() *cobra.Command {
// GetAccountAddressByIDCmd returns a query account that will display the account address of a given account id.
func GetAccountAddressByIDCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "address-by-id [id]",
Short: "Query for account address by account id",
Use: "address-by-acc-num [acc-num]",
Aliases: []string{"address-by-id"},
Short: "Query for an address by account number",
Args: cobra.ExactArgs(1),
Example: fmt.Sprintf("%s q auth address-by-id 1", version.AppName),
Example: fmt.Sprintf("%s q auth address-by-acc-num 1", version.AppName),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientQueryContext(cmd)
if err != nil {
return err
}

id, err := strconv.ParseUint(args[0], 10, 64)
accNum, err := strconv.ParseInt(args[0], 10, 64)
if err != nil {
return fmt.Errorf("id %s not a valid uint, please input a valid account-id", args[0])
return err
}

queryClient := types.NewQueryClient(clientCtx)
res, err := queryClient.AccountAddressByID(cmd.Context(), &types.QueryAccountAddressByIDRequest{Id: id})
res, err := queryClient.AccountAddressByID(cmd.Context(), &types.QueryAccountAddressByIDRequest{Id: accNum})
if err != nil {
return err
}
Expand Down
8 changes: 6 additions & 2 deletions x/auth/keeper/grpc_query.go
Expand Up @@ -24,10 +24,14 @@ func (ak AccountKeeper) AccountAddressByID(c context.Context, req *types.QueryAc
return nil, status.Errorf(codes.InvalidArgument, "empty request")
}

if req.Id < 0 {
return nil, status.Error(codes.InvalidArgument, "invalid account number")
}

ctx := sdk.UnwrapSDKContext(c)
address := ak.GetAccountAddressByID(ctx, req.Id)
address := ak.GetAccountAddressByID(ctx, uint64(req.GetId()))

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion

Potential integer overflow by integer type conversion
if len(address) == 0 {
return nil, status.Errorf(codes.NotFound, "account address not found with id %d", req.Id)
return nil, status.Errorf(codes.NotFound, "account address not found with account number %d", req.Id)
}

return &types.QueryAccountAddressByIDResponse{AccountAddress: address}, nil
Expand Down
10 changes: 9 additions & 1 deletion x/auth/keeper/grpc_query_test.go
Expand Up @@ -166,6 +166,14 @@ func (suite *KeeperTestSuite) TestGRPCQueryAccountAddressByID() {
expPass bool
posttests func(res *types.QueryAccountAddressByIDResponse)
}{
{
"invalid request",
func() {
req = &types.QueryAccountAddressByIDRequest{Id: -1}
},
false,
func(res *types.QueryAccountAddressByIDResponse) {},
},
{
"account address not found",
func() {
Expand All @@ -179,7 +187,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryAccountAddressByID() {
func() {
account := suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr)
suite.accountKeeper.SetAccount(suite.ctx, account)
req = &types.QueryAccountAddressByIDRequest{Id: account.GetAccountNumber()}
req = &types.QueryAccountAddressByIDRequest{Id: int64(account.GetAccountNumber())}
},
true,
func(res *types.QueryAccountAddressByIDResponse) {
Expand Down