ADR 11: Generalize Gen Accounts implementation #5017

fedekunze · 2019-09-09T15:27:48Z

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

x/auth/simulation/genesis.go

x/auth/types/genesis.go

…dekunze/ADR-11-generalize-gen-accs

rhuairahrighairidh · 2019-09-10T15:05:33Z

I was working on this implementation for a couple of days, didn’t realise you were also @fedekunze .
What’s the procedure in cosmos-sdk to avoid duplicating work?

codecov · 2019-09-10T15:09:52Z

Codecov Report

Merging #5017 into master will increase coverage by 0.08%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##           master    #5017      +/-   ##
==========================================
+ Coverage   54.85%   54.94%   +0.08%     
==========================================
  Files         291      290       -1     
  Lines       17846    17805      -41     
==========================================
- Hits         9790     9783       -7     
+ Misses       7351     7316      -35     
- Partials      705      706       +1

fedekunze · 2019-09-10T16:19:59Z

I was working on this implementation for a couple of days, didn’t realize you were also @fedekunze .
What’s the procedure in cosmos-sdk to avoid duplicating work?

Sorry about that. Usually self-assigning to an open issue and opening a work-in-progress PR

alexanderbez · 2019-09-10T20:01:37Z

@fedekunze @rhuairahrighairidh what do you recommend in order to proceed?

fedekunze · 2019-09-10T20:32:29Z

@alexanderbez the one from Kava Labs is still WIP Kava-Labs#3. I'd suggest reviewing this one and be more in sync with who's doing what.

rhuairahrighairidh · 2019-09-10T21:39:19Z

that sounds good, this one is further ahead

x/auth/alias.go

x/auth/client/cli/genaccounts.go

x/auth/exported/exported.go

x/auth/genesis.go

x/auth/types/codec.go

x/auth/types/genesis.go

x/genutil/legacy/v0_36/migrate.go

x/supply/internal/types/account.go

x/supply/internal/types/codec.go

alexanderbez · 2019-09-11T16:48:51Z

Reviewed, thanks @fedekunze! I think this PR needs to be tested thoroughly against/with Gaia. In addition, the add-genesis command needs to be implemented in the app, not here. Furthermore, there should be an actual GenesisAccount implementation...also, probably in Gaia.

Co-Authored-By: Alexander Bezobchuk <[email protected]>

….com/cosmos/cosmos-sdk into fedekunze/ADR-11-generalize-gen-accs

alexanderbez

Left a few more tidbits. Also, I believe we should leave the existing legacy migration logic in x/genaccounts/legacy/* and we must also implementation a migration in x/auth.

x/supply/internal/types/account.go

x/genaccounts/legacy/v0_36/types.go

x/genaccounts/legacy/v0_36/migrate_test.go

rigelrozanski · 2019-09-11T21:20:34Z

So this isn't actually and ADR but an ADR implementation.. we should make that more clear in the title of this PR

rigelrozanski

Minor comments throughout. Although obviously need to resolve all CI issues before merge

x/auth/types/codec.go

rigelrozanski · 2019-09-11T21:25:55Z

docs/architecture/adr-011-generalize-genesis-accounts.md


 // Register the module account type with the auth module codec so it can decode module accounts stored in a genesis file
 func init() {
-    authTypes.RegisterAccountTypeCodec(ModuleAccount{}, "cosmos-sdk/ModuleAccount")
+    authtypes.RegisterAccountTypeCodec(ModuleAccount{}, "cosmos-sdk/ModuleAccount")


Additionally we should provided a function: SealAccountTypeCodec to be used after all auth types have been registered

I don't think this function is necessary. The application will call {module}.ModuleCdc.Seal() somewhere near the end in the app constructor (e.g. gov.ModuleCdc.Seal()).

rigelrozanski · 2019-09-11T21:27:58Z

simapp/test_helpers.go

 	db := dbm.NewMemDB()
 	app := NewSimApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil, true, 0)

 	// initialize the chain with the passed in genesis accounts
 	genesisState := NewDefaultGenesisState()
-	genesisState = genaccounts.SetGenesisStateInAppState(app.Codec(), genesisState, genaccounts.GenesisState(genAccs))
+
+	authGenesis := auth.NewGenesisState(auth.DefaultParams(), genAccs)


what are the default params here btw?

rigelrozanski · 2019-09-11T21:28:33Z

x/auth/exported/exported.go

@@ -57,3 +57,9 @@ type VestingAccount interface {
 	GetDelegatedFree() sdk.Coins
 	GetDelegatedVesting() sdk.Coins
 }
+
+// GenesisAccount defines a genesis account that embeds an Account with validation capabilities.
+type GenesisAccount interface {


nice! I'm a fan of this pattern

x/auth/genesis.go

x/auth/simulation/genesis.go

x/auth/types/account.go

x/supply/internal/types/account.go

alexanderbez

testedACK:

Fixed migration logic
Updated changelog
Added concrete simulation genesis account type
Added tests
Tested migration against gaia 2.0.0

…tion

ADR 11: Generalize Gen Accounts implementation

40ed336

fedekunze added WIP labels Sep 9, 2019

move CLI cmd to auth

90a3882

golangcibot reviewed Sep 10, 2019

View reviewed changes

x/auth/simulation/genesis.go Outdated Show resolved Hide resolved

x/auth/types/genesis.go Outdated Show resolved Hide resolved

fedekunze added 4 commits September 10, 2019 12:37

delete genaccount module

8e92e02

add some tests

e1e4993

more fixes

84f92dd

register codec

19fd7c8

fedekunze added R4R and removed WIP labels Sep 10, 2019

fedekunze marked this pull request as ready for review September 10, 2019 14:54

fedekunze requested review from alessio, alexanderbez, jackzampolin and rigelrozanski as code owners September 10, 2019 14:54

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into fe…

7085b9c

…dekunze/ADR-11-generalize-gen-accs

fedekunze requested review from zmanian, colin-axner and AdityaSripal September 10, 2019 14:59

karzak mentioned this pull request Sep 11, 2019

Decouple auth/vesting #4486

Closed

4 tasks

alexanderbez suggested changes Sep 11, 2019

View reviewed changes

alexanderbez mentioned this pull request Sep 11, 2019

update sims operations to use baseapp #4946

Merged

5 tasks

fedekunze and others added 4 commits September 11, 2019 18:54

Apply suggestions from code review

0b6bbb5

Co-Authored-By: Alexander Bezobchuk <[email protected]>

address comments from review

e6bf7c4

Merge branch 'fedekunze/ADR-11-generalize-gen-accs' of https://github…

ac7aaef

….com/cosmos/cosmos-sdk into fedekunze/ADR-11-generalize-gen-accs

remove genaccounts command

e9d215a

alexanderbez suggested changes Sep 11, 2019

View reviewed changes

x/supply/internal/types/account.go Show resolved Hide resolved

x/genaccounts/legacy/v0_36/types.go Outdated Show resolved Hide resolved

x/genaccounts/legacy/v0_36/migrate_test.go Outdated Show resolved Hide resolved

fedekunze mentioned this pull request Sep 11, 2019

Genesis Account Command + Types cosmos/gaia#122

Merged

5 tasks

fedekunze added 3 commits September 11, 2019 20:42

keep legacy genaccount migrations

5a69363

update genutil migration

6c4b702

format

4e5e1a2

rigelrozanski approved these changes Sep 11, 2019

View reviewed changes

karzak reviewed Sep 11, 2019

View reviewed changes

x/supply/internal/types/account.go Show resolved Hide resolved

fedekunze added 2 commits September 12, 2019 12:01

migrate

6288793

iterator

fe125bf

karzak mentioned this pull request Sep 12, 2019

Separate vesting from auth, add custom vesting schedules #5040

Merged

3 tasks

alexanderbez added 10 commits September 12, 2019 12:22

Update godoc

1198733

Update godoc

f07fabc

Remove RegisterAccountTypeCodec call from supply

2979564

Merge branch 'master' into fedekunze/ADR-11-generalize-gen-accs

3636574

re-add RegisterAccountTypeCodec

da91323

Update godoc

42b04ee

Add sim genesis concrete type

7cef4d1

Fix migration

ae1569f

Add ref for #5041

3d924db

Add changelog entries

1d16d34

alexanderbez approved these changes Sep 12, 2019

View reviewed changes

alexanderbez merged commit ed63666 into master Sep 12, 2019

alexanderbez deleted the fedekunze/ADR-11-generalize-gen-accs branch September 12, 2019 19:24

alexanderbez mentioned this pull request Sep 13, 2019

Genesis Account Migration #5046

Closed

4 tasks

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

Merge PR cosmos#5017: ADR 011: Generalize Genesis Accounts Implementa…

3916131

…tion

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR 11: Generalize Gen Accounts implementation #5017

ADR 11: Generalize Gen Accounts implementation #5017

fedekunze commented Sep 9, 2019

rhuairahrighairidh commented Sep 10, 2019

codecov bot commented Sep 10, 2019 •

edited

Loading

fedekunze commented Sep 10, 2019

alexanderbez commented Sep 10, 2019

fedekunze commented Sep 10, 2019

rhuairahrighairidh commented Sep 10, 2019

alexanderbez commented Sep 11, 2019

alexanderbez left a comment

rigelrozanski commented Sep 11, 2019

rigelrozanski left a comment

rigelrozanski Sep 11, 2019

alexanderbez Sep 12, 2019

rigelrozanski Sep 11, 2019

rigelrozanski Sep 11, 2019

alexanderbez left a comment

ADR 11: Generalize Gen Accounts implementation #5017

ADR 11: Generalize Gen Accounts implementation #5017

Conversation

fedekunze commented Sep 9, 2019

rhuairahrighairidh commented Sep 10, 2019

codecov bot commented Sep 10, 2019 • edited Loading

Codecov Report

fedekunze commented Sep 10, 2019

alexanderbez commented Sep 10, 2019

fedekunze commented Sep 10, 2019

rhuairahrighairidh commented Sep 10, 2019

alexanderbez commented Sep 11, 2019

alexanderbez left a comment

Choose a reason for hiding this comment

rigelrozanski commented Sep 11, 2019

rigelrozanski left a comment

Choose a reason for hiding this comment

rigelrozanski Sep 11, 2019

Choose a reason for hiding this comment

alexanderbez Sep 12, 2019

Choose a reason for hiding this comment

rigelrozanski Sep 11, 2019

Choose a reason for hiding this comment

rigelrozanski Sep 11, 2019

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 10, 2019 •

edited

Loading