fix: InitGenesis changes account numbers (upstream fix from Provenance) #13877
Conversation
x/auth/keeper/genesis.go
Outdated
| for _, acc := range accounts { | ||
| accNum := acc.GetAccountNumber() | ||
| for lastAccNum == nil || *lastAccNum < accNum { | ||
| n := ak.GetNextAccountNumber(ctx) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Well we're def. backporting to 0.47 (I added the label). Older versions I'm not sure. Which ever way, we can accommodate :)
There was a problem hiding this comment.
agree, lets at a minimum do 0.47. It would be nice to have the api make it more explicit it changes state.
There was a problem hiding this comment.
Ok so we rename it?
alexanderbez
added
the
backport/v0.47.x
Codecov Report
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
|
| @@ -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`. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's under Bug Fixes, the diff viewer is bad lol
…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)
…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>
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_collectoraccount 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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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