refactor!: make ExportGenesis return proto.Message #12700

facundomedica · 2022-07-22T22:14:45Z

Description

This PR replaces the usage of json.RawMessage for proto.Message in most places where genesis states are being passed.

AppModuleBasic

- DefaultGenesis(codec.JSONCodec) json.RawMessage
+ DefaultGenesis() proto.Message

AppModuleGenesis

- InitGenesis(sdk.Context, codec.JSONCodec, json.RawMessage) []abci.ValidatorUpdate
- ExportGenesis(sdk.Context, codec.JSONCodec) json.RawMessage
+ InitGenesis(sdk.Context, proto.Message) []abci.ValidatorUpdate
+ ExportGenesis(sdk.Context) proto.Message

Unresolved issues

`ModuleManager`'s still uses `map[string]json.RawMessage`

I need to figure out how to create a map[string]proto.Message that we can unmarshal values to. Maybe we need to define a new type for this somehow? I need more input on this issue

Closes: #12642

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)

…/import-export-genesis

…s/cosmos-sdk into facu/import-export-genesis

github-advanced-security

Found 13 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 13 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 13 potential problems in the proposed changes. Check the Files changed tab for more details.

codecov · 2022-07-29T20:18:58Z

Codecov Report

Merging #12700 (161ea0e) into main (cb31043) will decrease coverage by 0.00%.
The diff coverage is 55.76%.

@@            Coverage Diff             @@
##             main   #12700      +/-   ##
==========================================
- Coverage   56.12%   56.12%   -0.01%     
==========================================
  Files         645      645              
  Lines       54660    54649      -11     
==========================================
- Hits        30680    30670      -10     
- Misses      21505    21506       +1     
+ Partials     2475     2473       -2

Impacted Files	Coverage Δ
simapp/state.go	`0.00% <0.00%> (ø)`
types/module/simulation.go	`0.00% <ø> (ø)`
types/utils.go	`63.85% <0.00%> (-15.25%)`	⬇️
x/feegrant/module/module.go	`15.00% <0.00%> (+0.36%)`	⬆️
x/params/module.go	`20.96% <0.00%> (ø)`
x/upgrade/module.go	`28.35% <0.00%> (ø)`
simapp/test_helpers.go	`22.86% <33.33%> (ø)`
x/evidence/module.go	`45.20% <55.55%> (-0.75%)`	⬇️
x/group/module/module.go	`51.25% <66.66%> (+0.63%)`	⬆️
x/auth/module.go	`52.05% <71.42%> (+0.05%)`	⬆️
... and 20 more

alexanderbez · 2022-07-30T02:10:20Z

@marbar3778 should we make a note of something in an upgrading doc?

Great work @facundomedica!

tac0turtle · 2022-07-31T11:40:45Z

@marbar3778 should we make a note of something in an upgrading doc?

Yes a short explanation in the upgrading doc is needed

simapp/genesis.go

ValarDragon

The cleanup looks great!

types/utils.go

amaury1093 · 2022-08-01T11:38:40Z

runtime/builder.go

@@ -20,8 +20,8 @@ type AppBuilder struct {

 // DefaultGenesis returns a default genesis from the registered
 // AppModuleBasic's.
-func (a *AppBuilder) DefaultGenesis() map[string]json.RawMessage {
-	return a.app.basicManager.DefaultGenesis(a.app.cdc)
+func (a *AppBuilder) DefaultGenesis() map[string]proto.Message {


Will we break the AppModule interface again once we remove gogoproto?

How hard would it be to use the api/ genesis state today?

pulsar is not production ready from my understanding. We shouldn't force people to use it yet. the interface break is fairly small in the future, if we document then people will be fine with it

simapp/genesis.go

types/module/module.go

amaury1093

LGTM

amaury1093 · 2022-08-02T12:35:47Z

types/module/module.go

+		// to which we then unmarshal the genesis' file JSON into
+		genType := reflect.TypeOf(m.Modules[moduleName].DefaultGenesis())
+		if genType == nil {
+			continue


I would panic here instead TBH. It's feedback to the app developers that their DefaultGenesis is incorrect.

I believe there were some modules returning empty structs like this: []byte(`{}`) and I translated those to nil. So I'm gonna double check that

For example x/auth/vesting and x/upgrade returned this:

func (AppModuleBasic) DefaultGenesis(_ codec.JSONCodec) json.RawMessage { return []byte("{}") }

types/module/module.go

@@ -31,8 +31,10 @@
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"


aaronc

This breaks how the orm does genesis. I'm sorry but let's please close

aaronc · 2022-08-02T13:10:56Z

The orm will be unusable with this API change. It relies on raw json being usable. This is not just an API change but removes functionality other code depends on.

If people really want something like this, just have two extension interfaces (current genesis and proto only genesis) and let AppManager type cast.

facundomedica · 2022-08-03T13:24:39Z

I'm trying out different strategies, so far the main blocker is that 2 of the methods are part of AppModuleBasic:

	DefaultGenesis(codec.JSONCodec) json.RawMessage
	ValidateGenesis(codec.JSONCodec, client.TxEncodingConfig, json.RawMessage) error

And these are used everywhere. I think I can make it work but this would disrupt the module pattern described at the top of the types/module.go file :

The module pattern has been broken down by:
 - independent module functionality (AppModuleBasic)
 - inter-dependent module genesis functionality (AppModuleGenesis)
 - inter-dependent module simulation functionality (AppModuleSimulation)
 - inter-dependent module full functionality (AppModule)

alexanderbez · 2022-08-03T16:14:29Z

I thought we were just defining a new extension interface?

facundomedica · 2022-08-08T18:05:48Z

I've opened this replacement PR: #12828

initial progress

2e12fa1

github-actions bot added C:x/auth C:x/authz C:x/bank C:x/capability C:x/crisis C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/genutil genutil module issues C:x/gov C:x/group C:x/mint C:x/nft C:x/params C:x/slashing C:x/staking C:x/upgrade labels Jul 22, 2022

facundomedica and others added 3 commits July 22, 2022 19:16

small fix

4c3891f

DefaultGenesis, InitGenesis

e084984

Merge branch 'main' into facu/import-export-genesis

aabb540

github-actions bot added C:CLI C:Simulations labels Jul 25, 2022

facundomedica added 4 commits July 25, 2022 18:44

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…

c8498f4

…/import-export-genesis

fix legacy app

2b6b679

Merge branch 'facu/import-export-genesis' of https://github.com/cosmo…

7605079

…s/cosmos-sdk into facu/import-export-genesis

fix test

15e4cf7

github-advanced-security bot found potential problems Jul 25, 2022

View reviewed changes

facundomedica and others added 8 commits July 28, 2022 13:44

move upgrading notes

60e0995

Merge branch 'main' into facu/import-export-genesis

2a73e25

Merge branch 'main' into facu/import-export-genesis

f031937

Merge branch 'main' into facu/import-export-genesis

4e855e3

Merge branch 'main' into facu/import-export-genesis

8561535

Merge branch 'main' into facu/import-export-genesis

5be7220

Merge branch 'main' into facu/import-export-genesis

4f5dbb7

Merge branch 'main' into facu/import-export-genesis

c3fee89

alexanderbez approved these changes Jul 30, 2022

View reviewed changes

ValarDragon reviewed Jul 31, 2022

View reviewed changes

simapp/genesis.go Outdated Show resolved Hide resolved

ValarDragon approved these changes Jul 31, 2022

View reviewed changes

amaury1093 reviewed Aug 1, 2022

View reviewed changes

facundomedica added 5 commits August 1, 2022 12:54

remove GenesisState type

56e58ae

don't use tmjson in utils

f87781f

fixes

d304999

use reflect pkg to get genesis type

0181ccf

fix conflcts

161ea0e

amaury1093 approved these changes Aug 2, 2022

View reviewed changes

github-advanced-security bot found potential problems Aug 2, 2022

View reviewed changes

types/module/module.go

@@ -31,8 +31,10 @@

import (

"encoding/json"

"fmt"

"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

aaronc requested changes Aug 2, 2022

View reviewed changes

facundomedica mentioned this pull request Aug 8, 2022

refactor!: Accept both genesis as json.RawMessage and proto.Message #12828

Closed

19 tasks

facundomedica closed this Aug 8, 2022

facundomedica deleted the facu/import-export-genesis branch August 30, 2022 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: make ExportGenesis return proto.Message #12700

refactor!: make ExportGenesis return proto.Message #12700

facundomedica commented Jul 22, 2022 •

edited

Loading

github-advanced-security bot left a comment

github-advanced-security bot left a comment

github-advanced-security bot left a comment

codecov bot commented Jul 29, 2022 •

edited

Loading

alexanderbez commented Jul 30, 2022 •

edited

Loading

tac0turtle commented Jul 31, 2022

ValarDragon left a comment

amaury1093 Aug 1, 2022

tac0turtle Aug 1, 2022

amaury1093 left a comment

amaury1093 Aug 2, 2022

facundomedica Aug 2, 2022

facundomedica Aug 2, 2022

aaronc left a comment •

edited

Loading

aaronc commented Aug 2, 2022

facundomedica commented Aug 3, 2022

alexanderbez commented Aug 3, 2022

facundomedica commented Aug 8, 2022

refactor!: make ExportGenesis return proto.Message #12700

refactor!: make ExportGenesis return proto.Message #12700

Conversation

facundomedica commented Jul 22, 2022 • edited Loading

Description

Unresolved issues

ModuleManager's still uses map[string]json.RawMessage

Author Checklist

Reviewers Checklist

github-advanced-security bot left a comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2022 • edited Loading

Codecov Report

alexanderbez commented Jul 30, 2022 • edited Loading

tac0turtle commented Jul 31, 2022

ValarDragon left a comment

Choose a reason for hiding this comment

amaury1093 Aug 1, 2022

Choose a reason for hiding this comment

tac0turtle Aug 1, 2022

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Aug 2, 2022

Choose a reason for hiding this comment

facundomedica Aug 2, 2022

Choose a reason for hiding this comment

facundomedica Aug 2, 2022

Choose a reason for hiding this comment

aaronc left a comment • edited Loading

Choose a reason for hiding this comment

aaronc commented Aug 2, 2022

facundomedica commented Aug 3, 2022

alexanderbez commented Aug 3, 2022

facundomedica commented Aug 8, 2022

facundomedica commented Jul 22, 2022 •

edited

Loading

`ModuleManager`'s still uses `map[string]json.RawMessage`

codecov bot commented Jul 29, 2022 •

edited

Loading

alexanderbez commented Jul 30, 2022 •

edited

Loading

aaronc left a comment •

edited

Loading