fix: InitGenesis changes account numbers (upstream fix from Provenance) #13877

facundomedica · 2022-11-15T20:48:57Z

Description

Pretty much a copy-paste from @SpicyLemon 's work at provenance-io#228 + some tiny extras (the only difference is that we set the fee_collector account in tests).

Closes: #13033

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)

alexanderbez · 2022-11-15T21:24:50Z

x/auth/keeper/genesis.go

+	for _, acc := range accounts {
+		accNum := acc.GetAccountNumber()
+		for lastAccNum == nil || *lastAccNum < accNum {
+			n := ak.GetNextAccountNumber(ctx)


I was scratching my head at this thinking "this doesn't actually do anything". Then I took a look at GetNextAccountNumber and realized it's mutative. It really should be called NextAccountNumber or IncrAccountNumber IMO.

If we're backporting this to 0.47, I recommend we fix this API. If we're backporting to 0.47 and 0.46/0.45, then I recommend we create a second method that calls the existing so we don't break APIs.

Yeah, Get makes it sound like a read-only operation but it's doing:

bz = ak.cdc.MustMarshal(&gogotypes.UInt64Value{Value: accNumber + 1}) store.Set(types.GlobalAccountNumberKey, bz)

Not sure, if we are backporting to previous versions. @tac0turtle ?

Well we're def. backporting to 0.47 (I added the label). Older versions I'm not sure. Which ever way, we can accommodate :)

agree, lets at a minimum do 0.47. It would be nice to have the api make it more explicit it changes state.

Ok so we rename it?

julienrbrt

utACK. Pending the API renaming #13877 (comment).

x/auth/keeper/keeper_test.go

likhita-809

utACK

…into facu/fix-13033

codecov · 2022-11-16T16:14:36Z

Codecov Report

Merging #13877 (767fc24) into main (f82775b) will increase coverage by 0.00%.
The diff coverage is 53.84%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13877   +/-   ##
=======================================
  Coverage   56.56%   56.56%           
=======================================
  Files         638      638           
  Lines       54877    54904   +27     
=======================================
+ Hits        31040    31056   +16     
- Misses      21317    21323    +6     
- Partials     2520     2525    +5

Impacted Files	Coverage Δ
x/auth/keeper/account.go	`76.66% <0.00%> (+23.33%)`	⬆️
x/group/keeper/keeper.go	`56.07% <0.00%> (-0.18%)`	⬇️
x/group/module/abci.go	`20.00% <0.00%> (-30.00%)`	⬇️
x/group/msgs.go	`57.60% <0.00%> (+0.24%)`	⬆️
x/group/types.go	`47.57% <33.33%> (-1.22%)`	⬇️
x/auth/types/genesis.go	`59.29% <37.93%> (-7.38%)`	⬇️
x/group/internal/math/dec.go	`47.14% <60.00%> (+5.60%)`	⬆️
x/group/keeper/msg_server.go	`68.74% <66.66%> (+0.09%)`	⬆️
x/auth/keeper/genesis.go	`50.00% <100.00%> (+50.00%)`	⬆️
x/auth/keeper/keeper.go	`70.90% <100.00%> (ø)`
... and 8 more

julienrbrt · 2022-11-16T16:34:27Z

CHANGELOG.md

@@ -199,6 +200,7 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf
 * (x/gov) [#13728](https://github.com/cosmos/cosmos-sdk/pull/13728) Fix propagation of message events to the current context in `EndBlocker`.
 * (server) [#13778](https://github.com/cosmos/cosmos-sdk/pull/13778) Set Cosmos SDK default endpoints to localhost to avoid unknown exposure of endpoints.
 * [#13861](https://github.com/cosmos/cosmos-sdk/pull/13861) Allow `_` characters in tx event queries, i.e. `GetTxsEvent`.
+* (x/auth) [#13877](https://github.com/cosmos/cosmos-sdk/pull/13877) Handle missing account numbers during `InitGenesis`.


I think that's a bug fix, wrong category. But I can shuffle it later if you want, so you don't have to wait CI another time.

It's under Bug Fixes, the diff viewer is bad lol

@julienrbrt

…e) (#13877) * fix: InitGenesis changes account numbers (upstream fix from Provenance) * cl++ * rename GetNextAccountNumber to NextAccountNumber * suggestion from @julienrbrt * cl++ * fix readme * missing replace (cherry picked from commit 394f1b9)

@julienrbrt

…e) (#13877) (#13893) * fix: InitGenesis changes account numbers (upstream fix from Provenance) * cl++ * rename GetNextAccountNumber to NextAccountNumber * suggestion from @julienrbrt * cl++ * fix readme * missing replace (cherry picked from commit 394f1b9) Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>

fix: InitGenesis changes account numbers (upstream fix from Provenance)

8d9d96e

github-actions bot added the C:x/auth label Nov 15, 2022

cl++

63c228a

facundomedica marked this pull request as ready for review November 15, 2022 20:55

facundomedica requested a review from a team as a code owner November 15, 2022 20:55

Merge branch 'main' into facu/fix-13033

7c937d8

alexanderbez approved these changes Nov 15, 2022

View reviewed changes

alexanderbez added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Nov 15, 2022

julienrbrt approved these changes Nov 16, 2022

View reviewed changes

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

likhita-809 approved these changes Nov 16, 2022

View reviewed changes

facundomedica added 4 commits November 16, 2022 12:50

rename GetNextAccountNumber to NextAccountNumber

35c5388

suggestion from @julienrbrt

ae92123

cl++

1fec3c1

fix readme

1314d10

facundomedica requested review from alexanderbez and tac0turtle November 16, 2022 15:56

facundomedica and others added 3 commits November 16, 2022 12:56

Merge branch 'main' into facu/fix-13033

65dfa24

missing replace

6fb5d3f

Merge branch 'facu/fix-13033' of https://github.com/cosmos/cosmos-sdk …

767fc24

…into facu/fix-13033

tac0turtle approved these changes Nov 16, 2022

View reviewed changes

julienrbrt reviewed Nov 16, 2022

View reviewed changes

alexanderbez approved these changes Nov 16, 2022

View reviewed changes

facundomedica merged commit 394f1b9 into main Nov 16, 2022

facundomedica deleted the facu/fix-13033 branch November 16, 2022 17:13

mergify bot mentioned this pull request Nov 16, 2022

fix: InitGenesis changes account numbers (upstream fix from Provenance) (backport #13877) #13893

Merged

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

fix: InitGenesis changes account numbers (upstream fix from Provenance) #13877

fix: InitGenesis changes account numbers (upstream fix from Provenance) #13877

facundomedica commented Nov 15, 2022 •

edited

Loading

alexanderbez Nov 15, 2022

alexanderbez Nov 15, 2022 •

edited

Loading

facundomedica Nov 15, 2022

alexanderbez Nov 15, 2022

tac0turtle Nov 16, 2022

facundomedica Nov 16, 2022

alexanderbez Nov 16, 2022

facundomedica Nov 16, 2022

julienrbrt left a comment

likhita-809 left a comment

codecov bot commented Nov 16, 2022

julienrbrt Nov 16, 2022 •

edited

Loading

facundomedica Nov 16, 2022

fix: InitGenesis changes account numbers (upstream fix from Provenance) #13877

fix: InitGenesis changes account numbers (upstream fix from Provenance) #13877

Conversation

facundomedica commented Nov 15, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

alexanderbez Nov 15, 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

julienrbrt left a comment

Choose a reason for hiding this comment

likhita-809 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 16, 2022

Codecov Report

julienrbrt Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundomedica commented Nov 15, 2022 •

edited

Loading

alexanderbez Nov 15, 2022 •

edited

Loading

julienrbrt Nov 16, 2022 •

edited

Loading