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

atheeshp · 2022-11-07T12:44:48Z

Description

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...

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...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…c-id-type

CHANGELOG.md

x/auth/keeper/grpc_query.go

proto/cosmos/auth/v1beta1/query.proto

amaury1093 · 2022-11-09T13:55:59Z

proto/cosmos/auth/v1beta1/query.proto

@@ -197,10 +197,17 @@ 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;


we can add the proto deprecated annotation here

x/auth/keeper/grpc_query_test.go

…c-id-type

…into ap/change-acc-id-type

codecov · 2022-11-10T11:45:15Z

Codecov Report

Merging #13780 (3a39765) into main (d8391cb) will increase coverage by 0.00%.
The diff coverage is 55.55%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13780   +/-   ##
=======================================
  Coverage   56.24%   56.24%           
=======================================
  Files         664      664           
  Lines       56480    56484    +4     
=======================================
+ Hits        31766    31769    +3     
- Misses      22143    22144    +1     
  Partials     2571     2571

Impacted Files	Coverage Δ
x/auth/client/cli/query.go	`19.55% <0.00%> (-0.13%)`	⬇️
x/auth/keeper/grpc_query.go	`57.14% <100.00%> (-1.30%)`	⬇️
x/auth/keeper/account.go	`71.66% <0.00%> (-5.00%)`	⬇️
x/staking/simulation/operations.go	`75.91% <0.00%> (+1.37%)`	⬆️
crypto/keys/internal/ecdsa/privkey.go	`83.01% <0.00%> (+1.88%)`	⬆️

x/auth/keeper/grpc_query.go

x/auth/client/cli/query.go

alexanderbez · 2022-11-12T04:25:54Z

x/auth/keeper/grpc_query.go

+
+	var accId uint64
+	switch {
+	case req.AccountId > 0 && req.Id > 0 && req.AccountId != uint64(req.Id):


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.

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

We start account numbers at 1?

I'm not sure, but I'm getting the response for 0 too by using the same query.

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.

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.

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)

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.

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.

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.

…c-id-type

…into ap/change-acc-id-type

…c-id-type

amaury1093

LGTM, just a small proposal to have shorter code

x/auth/keeper/grpc_query.go

likhita-809

few nits

x/auth/keeper/grpc_query.go

CHANGELOG.md

proto/cosmos/auth/v1beta1/query.proto

Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>

…into ap/change-acc-id-type

…c-id-type

x/auth/keeper/grpc_query.go

…c-id-type

sonarcloud · 2022-11-21T11:44:18Z

[Cosmos SDK] SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

50.0% Coverage
0.0% Duplication

julienrbrt · 2022-11-30T07:31:40Z

@Mergifyio backport release/v0.47.x

mergify · 2022-11-30T07:32:25Z

backport release/v0.47.x

✅ Backports have been created

#14084 chore: added new field account-id with uint64 in AccountAddressByID (backport #13780) has been created for branch release/v0.47.x

…D` (#13780) * add changes * Update CHANGELOG.md * Update CHANGELOG.md * review changes * add test * fix static check * fix lint * fix lint * fix lint * review changes * review changes * review changes * review changes * fix tests * Update CHANGELOG.md Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com> * Update proto/cosmos/auth/v1beta1/query.proto Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com> * review changes * lint * review changes Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com> (cherry picked from commit 42597ee)

…D` (backport #13780) (#14084)

atheeshp added 2 commits November 7, 2022 18:08

add changes

82ef03d

Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…

bd53cb1

…c-id-type

github-actions bot added C:CLI C:x/auth labels Nov 7, 2022

atheeshp commented Nov 7, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

335db34

atheeshp commented Nov 7, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

4b944b8

atheeshp changed the title ~~added new field account-id with uint64 in AccountAddressByID~~ chore: added new field account-id with uint64 in AccountAddressByID Nov 7, 2022

github-advanced-security bot found potential problems Nov 7, 2022

View reviewed changes

x/auth/keeper/grpc_query.go Fixed Show fixed Hide fixed

x/auth/keeper/grpc_query.go Fixed Show fixed Hide fixed

amaury1093 reviewed Nov 9, 2022

View reviewed changes

atheeshp added 4 commits November 10, 2022 15:17

Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…

ce167ea

…c-id-type

Merge branch 'ap/change-acc-id-type' of github.com:cosmos/cosmos-sdk …

94d4cf2

…into ap/change-acc-id-type

review changes

2289441

add test

7f468bb

atheeshp requested a review from amaury1093 November 10, 2022 11:59

fix static check

a272769

github-advanced-security bot found potential problems Nov 10, 2022

View reviewed changes

x/auth/keeper/grpc_query.go Fixed Show fixed Hide fixed

x/auth/keeper/grpc_query.go Fixed Show fixed Hide fixed

fix lint

dd4d69f

github-advanced-security bot found potential problems Nov 10, 2022

View reviewed changes

x/auth/keeper/grpc_query.go Fixed Show fixed Hide fixed

x/auth/keeper/grpc_query.go Fixed Show fixed Hide fixed

atheeshp and others added 3 commits November 10, 2022 18:19

fix lint

98b9113

fix lint

7e09506

Merge branch 'main' into ap/change-acc-id-type

3bc2c5e

atheeshp marked this pull request as ready for review November 10, 2022 12:56

atheeshp requested a review from a team as a code owner November 10, 2022 12:56

alexanderbez reviewed Nov 12, 2022

View reviewed changes

atheeshp added 3 commits November 14, 2022 15:41

Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…

9592596

…c-id-type

Merge branch 'ap/change-acc-id-type' of github.com:cosmos/cosmos-sdk …

21d8396

…into ap/change-acc-id-type

review changes

8a622ea

atheeshp requested a review from amaury1093 November 16, 2022 04:49

amaury1093 assigned amaury1093 and alexanderbez Nov 16, 2022

atheeshp added 2 commits November 17, 2022 12:30

Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…

5875b96

…c-id-type

review changes

bdbaaa2

amaury1093 approved these changes Nov 17, 2022

View reviewed changes

x/auth/keeper/grpc_query.go Outdated Show resolved Hide resolved

review changes

c270cec

likhita-809 approved these changes Nov 17, 2022

View reviewed changes

x/auth/keeper/grpc_query.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

proto/cosmos/auth/v1beta1/query.proto Outdated Show resolved Hide resolved

atheeshp and others added 8 commits November 17, 2022 18:14

fix tests

03b5bd0

Update CHANGELOG.md

2b32933

Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>

Update CHANGELOG.md

037b354

Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>

Update proto/cosmos/auth/v1beta1/query.proto

78d46b9

Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>

review changes

34130b5

Merge branch 'ap/change-acc-id-type' of github.com:cosmos/cosmos-sdk …

93b888e

…into ap/change-acc-id-type

Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…

c3b198c

…c-id-type

lint

1d680e7

alexanderbez approved these changes Nov 17, 2022

View reviewed changes

x/auth/keeper/grpc_query.go Outdated Show resolved Hide resolved

atheeshp and others added 3 commits November 17, 2022 21:19

review changes

718c752

Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/change-ac…

faee1f8

…c-id-type

Merge branch 'main' into ap/change-acc-id-type

3a39765

atheeshp enabled auto-merge (squash) November 21, 2022 11:30

atheeshp merged commit 42597ee into main Nov 21, 2022

atheeshp deleted the ap/change-acc-id-type branch November 21, 2022 11:50

mergify bot mentioned this pull request Nov 30, 2022

chore: added new field account-id with uint64 in AccountAddressByID (backport #13780) #14084

Merged

tac0turtle pushed a commit that referenced this pull request Nov 30, 2022

chore: added new field account-id with uint64 in `AccountAddressByI…

3f3c6bb

…D` (backport #13780) (#14084)

0xstepit mentioned this pull request Jul 1, 2024

chore(cachemulti): add Copy method evmos/cosmos-sdk#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

atheeshp commented Nov 7, 2022

amaury1093 Nov 9, 2022

codecov bot commented Nov 10, 2022 •

edited

Loading

alexanderbez Nov 12, 2022

atheeshp Nov 12, 2022 •

edited

Loading

alexanderbez Nov 12, 2022

atheeshp Nov 14, 2022

alexanderbez Nov 14, 2022

amaury1093 Nov 14, 2022

atheeshp Nov 15, 2022

alexanderbez Nov 16, 2022

amaury1093 Nov 16, 2022 •

edited

Loading

alexanderbez Nov 16, 2022

amaury1093 left a comment

likhita-809 left a comment

sonarcloud bot commented Nov 21, 2022

julienrbrt commented Nov 30, 2022

mergify bot commented Nov 30, 2022

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

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

Conversation

atheeshp commented Nov 7, 2022

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

codecov bot commented Nov 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

atheeshp Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

likhita-809 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 21, 2022

julienrbrt commented Nov 30, 2022

mergify bot commented Nov 30, 2022

✅ Backports have been created

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

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

codecov bot commented Nov 10, 2022 •

edited

Loading

atheeshp Nov 12, 2022 •

edited

Loading

amaury1093 Nov 16, 2022 •

edited

Loading