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

chore: added new field account-id with uint64 in AccountAddressByID #13780

Merged
merged 32 commits into from
Nov 21, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
82ef03d
add changes
atheeshp Nov 7, 2022
bd53cb1
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…
atheeshp Nov 7, 2022
335db34
Update CHANGELOG.md
atheeshp Nov 7, 2022
4b944b8
Update CHANGELOG.md
atheeshp Nov 7, 2022
ce167ea
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…
atheeshp Nov 10, 2022
94d4cf2
Merge branch 'ap/change-acc-id-type' of github.com:cosmos/cosmos-sdk …
atheeshp Nov 10, 2022
2289441
review changes
atheeshp Nov 10, 2022
7f468bb
add test
atheeshp Nov 10, 2022
a272769
fix static check
atheeshp Nov 10, 2022
dd4d69f
fix lint
atheeshp Nov 10, 2022
98b9113
fix lint
atheeshp Nov 10, 2022
7e09506
fix lint
atheeshp Nov 10, 2022
3bc2c5e
Merge branch 'main' into ap/change-acc-id-type
atheeshp Nov 10, 2022
9592596
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…
atheeshp Nov 14, 2022
21d8396
Merge branch 'ap/change-acc-id-type' of github.com:cosmos/cosmos-sdk …
atheeshp Nov 14, 2022
8a622ea
review changes
atheeshp Nov 14, 2022
6302d29
review changes
atheeshp Nov 16, 2022
2cf1d2c
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…
atheeshp Nov 16, 2022
5875b96
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…
atheeshp Nov 17, 2022
bdbaaa2
review changes
atheeshp Nov 17, 2022
c270cec
review changes
atheeshp Nov 17, 2022
03b5bd0
fix tests
atheeshp Nov 17, 2022
2b32933
Update CHANGELOG.md
atheeshp Nov 17, 2022
037b354
Update CHANGELOG.md
atheeshp Nov 17, 2022
78d46b9
Update proto/cosmos/auth/v1beta1/query.proto
atheeshp Nov 17, 2022
34130b5
review changes
atheeshp Nov 17, 2022
93b888e
Merge branch 'ap/change-acc-id-type' of github.com:cosmos/cosmos-sdk …
atheeshp Nov 17, 2022
c3b198c
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…
atheeshp Nov 17, 2022
1d680e7
lint
atheeshp Nov 17, 2022
718c752
review changes
atheeshp Nov 17, 2022
faee1f8
Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…
atheeshp Nov 18, 2022
3a39765
Merge branch 'main' into ap/change-acc-id-type
atheeshp Nov 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#13236](https://github.com/cosmos/cosmos-sdk/pull/13236) Integrate Filter Logging
* [#13528](https://github.com/cosmos/cosmos-sdk/pull/13528) Update `ValidateMemoDecorator` to only check memo against `MaxMemoCharacters` param when a memo is present.
* [#13651](https://github.com/cosmos/cosmos-sdk/pull/13651) Update `server/config/config.GetConfig` function.
* [#13780](https://github.com/cosmos/cosmos-sdk/pull/13780) `id` (type of int64) in `AccountAddressByID` grpc query is now deprecated, update to use account-id(type of uint64) to request `AccountAddressByID`.
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
* [#13781](https://github.com/cosmos/cosmos-sdk/pull/13781) Remove `client/keys.KeysCdc`.
* [#13803](https://github.com/cosmos/cosmos-sdk/pull/13803) Add an error log if iavl set operation failed.
* [#13802](https://github.com/cosmos/cosmos-sdk/pull/13802) Add --output-document flag to the export CLI command to allow writing genesis state to a file.
Expand Down
325 changes: 194 additions & 131 deletions api/cosmos/auth/v1beta1/query.pulsar.go

Large diffs are not rendered by default.

11 changes: 10 additions & 1 deletion proto/cosmos/auth/v1beta1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,19 @@ message AddressStringToBytesResponse {
//
// Since: cosmos-sdk 0.46.2
message QueryAccountAddressByIDRequest {
// Deprecated, use account_id instead
//
// 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;
int64 id = 1 [deprecated = true];

// account_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.
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
//
// Since: cosmos-sdk 0.47
uint64 account_id = 2;
}

// QueryAccountAddressByIDResponse is the response type for AccountAddressByID rpc method
Expand Down
11 changes: 10 additions & 1 deletion x/auth/client/cli/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,17 @@ func GetAccountAddressByIDCmd() *cobra.Command {
return err
}

accNumUint, err := strconv.ParseUint(args[0], 10, 64)
atheeshp marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}

queryClient := types.NewQueryClient(clientCtx)
res, err := queryClient.AccountAddressByID(cmd.Context(), &types.QueryAccountAddressByIDRequest{Id: accNum})
res, err := queryClient.AccountAddressByID(cmd.Context(), &types.QueryAccountAddressByIDRequest{
Id: accNum,
AccountId: accNumUint,
})

if err != nil {
return err
}
Expand Down
14 changes: 12 additions & 2 deletions x/auth/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,21 @@ func (ak AccountKeeper) AccountAddressByID(c context.Context, req *types.QueryAc
}

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

var accId uint64
switch {
case req.AccountId > 0 && req.Id > 0 && req.AccountId != uint64(req.Id):
Fixed Show fixed Hide fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I'm not sure how much value this case adds. IMO we should instead enforce one or the other value is passed. If both are passed, i.e. both are non-zero, then simply error.

Copy link
Contributor Author

@atheeshp atheeshp Nov 12, 2022

Choose a reason for hiding this comment

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

IMO we should instead enforce one or the other value is passed

Unforunately we can't do this since 0 is also a valid case here to query by account-number. and the default values for both int64 & uint64 are 0s

Copy link
Contributor

Choose a reason for hiding this comment

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

We start account numbers at 1?

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'm not sure, but I'm getting the response for 0 too by using the same query.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I see that they start with 0. It' really unfortunate we have to have this sort of case. I would almost prefer to completely omit/ignore the legacy field all together.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also agree with ignoring the legacy field. Maybe we can throw an error if it's not zero? So that users have immediate feedback they need to change something on their side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are keeping it as deprecated means, may be it should not throw error unitl the field gets removed right? (not sure just a thought)

Copy link
Contributor

Choose a reason for hiding this comment

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

The question really is:

Are we backporting this to < 0.47.x? If not, then just ignore the legacy field and remove this case all together. If we are backporting to 0.46.x and 0.45.x, then we need to handle the use of the legacy field and I suppose this is fine.

Copy link
Contributor

@amaury1093 amaury1093 Nov 16, 2022

Choose a reason for hiding this comment

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

I vote no backport.

Edit: but again, I also don't like to ignore that old field. Imagina a dapp pinging that endpoint, and suddently (after node upgrade) it returns a different response. I propose to error if the legacy field is populated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is a zero/default value 0 is technically valid (the very first account). So there is no notion of "set" vs "unset" unfortunately. Thats why I suggested ignoring it.

return nil, status.Errorf(codes.InvalidArgument, "different values passed for id (%d) & account-id (%d)", req.Id, req.AccountId)
case req.AccountId > 0:
accId = req.AccountId
case req.Id > 0:
accId = uint64(req.Id)
Fixed Show fixed Hide fixed
Fixed Show fixed Hide fixed
}

ctx := sdk.UnwrapSDKContext(c)
address := ak.GetAccountAddressByID(ctx, uint64(req.GetId()))
address := ak.GetAccountAddressByID(ctx, accId)
if len(address) == 0 {
return nil, status.Errorf(codes.NotFound, "account address not found with account number %d", req.Id)
}
Expand Down
28 changes: 28 additions & 0 deletions x/auth/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,22 @@ func (suite *KeeperTestSuite) TestGRPCQueryAccountAddressByID() {
false,
func(res *types.QueryAccountAddressByIDResponse) {},
},
{
"invalid: account-id, id are not same",
func() {
req = &types.QueryAccountAddressByIDRequest{AccountId: 0, Id: 1}
},
false,
func(res *types.QueryAccountAddressByIDResponse) {},
},
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
{
"invalid: account-id, id are not same",
func() {
req = &types.QueryAccountAddressByIDRequest{AccountId: 1, Id: -1}
},
false,
func(res *types.QueryAccountAddressByIDResponse) {},
},
{
"account address not found",
func() {
Expand All @@ -182,6 +198,18 @@ func (suite *KeeperTestSuite) TestGRPCQueryAccountAddressByID() {
false,
func(res *types.QueryAccountAddressByIDResponse) {},
},
{
"valid account-id",
func() {
account := suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr)
suite.accountKeeper.SetAccount(suite.ctx, account)
req = &types.QueryAccountAddressByIDRequest{AccountId: account.GetAccountNumber()}
},
true,
func(res *types.QueryAccountAddressByIDResponse) {
suite.Require().NotNil(res.AccountAddress)
},
},
{
"valid request",
func() {
Expand Down
175 changes: 108 additions & 67 deletions x/auth/types/query.pb.go

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

Loading