feat: migrate x/capability to app wiring #12058

facundomedica · 2022-05-26T17:21:03Z

Description

Migrate x/capability to the new app wiring (dependency injection).

Tracking progress at #12069

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)

facundomedica · 2022-05-26T17:30:58Z

baseapp/baseapp.go

@@ -216,6 +216,9 @@ func (app *BaseApp) MountStores(keys ...storetypes.StoreKey) {
 		case *storetypes.TransientStoreKey:
 			app.MountStore(key, storetypes.StoreTypeTransient)

+		case *storetypes.MemoryStoreKey:


Not sure if adding this to baseapp is the way to go

What's the context here?

runtime/builder uses baseApps' MountStores. Without this addition we get this error Unrecognized store key type :*types.MemoryStoreKey.
I think that if we filter out any MemoryStoreKeys before calling this everything works just fine... I don't know enough about these things to know exactly what's up 😅

cosmos-sdk/runtime/builder.go

Lines 27 to 41 in 8eaff8f

// Build builds an *App instance.

func (a *AppBuilder) Build(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptions ...func(*baseapp.BaseApp)) *App {

for _, option := range a.app.baseAppOptions {

baseAppOptions = append(baseAppOptions, option)

}

// TODO: when the auth module is configured, fill-in txDecoder

bApp := baseapp.NewBaseApp(a.app.config.AppName, logger, db, nil, baseAppOptions...)

bApp.SetCommitMultiStoreTracer(traceStore)

bApp.SetVersion(version.Version)

bApp.SetInterfaceRegistry(a.app.interfaceRegistry)

bApp.MountStores(a.app.storeKeys...)

a.app.BaseApp = bApp

return a.app

}

nvm tried again removing it and it doesn't work

I'm not really sure about this code. I'd have to delegate to @aaronc here.

I think this addition is correct. IMHO the fact that the multi-store doesn't know how to map store keys -> store types is a really strange and error prone. So this patch is sort of needed, but really the multi-store design should be cleaned up a bit

facundomedica · 2022-05-26T19:40:18Z

x/capability/types/keys.go

@@ -14,7 +14,7 @@ const (
 	StoreKey = ModuleName

 	// MemStoreKey defines the in-memory store key
-	MemStoreKey = "mem_capability"
+	MemStoreKey = "memory:capability"


This is only used for testing, we might want to move it over. Same goes for StoreKey.

Why was this change necessary, just curious?

Because now the store keys are being registered through this function that has a fixed format:

cosmos-sdk/runtime/module.go

Lines 115 to 119 in 8eaff8f

func provideMemoryStoreKey(key container.ModuleKey, app appWrapper) *storetypes.MemoryStoreKey {

storeKey := storetypes.NewMemoryStoreKey(fmt.Sprintf("memory:%s", key.Name()))

registerStoreKey(app, storeKey)

return storeKey

}

I think this key is now deprecated and used only in tests because we don't use it anywhere else.

got it. If so, can we prefix the variable with Test or something? Or maybe just remove it completely and define it directly in tests?

I think we should move it over to test. We still need it to create the MemoryStoreKey pass it to the Keeper in tests. @aaronc this is what I pinged you about on Slack

We should maybe even have a copy of simapp/app.go before the app wiring (maybe simapp/legacy) that checks to make sure that way still works

I like that idea!

Ok, just opened #12068 to track that

And I will go back to MemStoreKey = "mem_capability" and create a new constant in the tests

I think it's okay to change the value of MemStoreKey just not to delete the const

Gotcha, will revert that last change then 👌

simapp/app.go

…/app-wiring-capability

facundomedica · 2022-05-26T20:57:26Z

All tests passing 🎉

alexanderbez · 2022-05-26T21:07:21Z

simapp/app.go

+		return msk
+	}
+
+	sk := app.UnsafeFindStoreKey(storeKey)


When would a mem key be found in the list of real store keys?

provideMemoryStoreKey calls registerStoreKey which appends the key to the list of store keys. Should we have a separate list for them?

cosmos-sdk/runtime/module.go

Lines 115 to 119 in 8eaff8f

func provideMemoryStoreKey(key container.ModuleKey, app appWrapper) *storetypes.MemoryStoreKey {

storeKey := storetypes.NewMemoryStoreKey(fmt.Sprintf("memory:%s", key.Name()))

registerStoreKey(app, storeKey)

return storeKey

}

alexanderbez · 2022-05-26T21:07:29Z

baseapp/baseapp.go

@@ -216,6 +216,9 @@ func (app *BaseApp) MountStores(keys ...storetypes.StoreKey) {
 		case *storetypes.TransientStoreKey:
 			app.MountStore(key, storetypes.StoreTypeTransient)

+		case *storetypes.MemoryStoreKey:


What's the context here?

alexanderbez · 2022-05-26T21:08:22Z

x/capability/types/keys.go

@@ -14,7 +14,7 @@ const (
 	StoreKey = ModuleName

 	// MemStoreKey defines the in-memory store key
-	MemStoreKey = "mem_capability"
+	MemStoreKey = "memory:capability"


Why was this change necessary, just curious?

…s/cosmos-sdk into facu/app-wiring-capability

This reverts commit ad191ea.

simapp/app.go

…s/cosmos-sdk into facu/app-wiring-capability

x/capability/keeper/keeper.go

…s/cosmos-sdk into facu/app-wiring-capability

blushi · 2022-06-02T10:37:08Z

@facundomedica could you fix the conflicts?

x/capability/module.go

 	defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)

 	am.keeper.InitMemStore(ctx)
+
+	if am.sealKeeper && !am.keeper.IsSealed() {


x/capability/module.go

 	defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)

 	am.keeper.InitMemStore(ctx)
+
+	if am.sealKeeper && !am.keeper.IsSealed() {
+		am.keeper.Seal()


api/cosmos/capability/module/v1/module.pulsar.go

+	protoiface "google.golang.org/protobuf/runtime/protoiface"
+	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
+	io "io"
+	reflect "reflect"


@aaronc

* feat: migrate x/capability to app wiring * added some fixes from Matt's PR + changed MemStoreKey * fix name * fix name * small change * rollback MemStoreKey change * Revert "rollback MemStoreKey change" This reverts commit ad191ea. * add suggestions by @aaronc * add IsSealed() * fix Co-authored-by: Aaron Craelius <aaron@regen.network>

feat: migrate x/capability to app wiring

cfee9c8

github-actions bot added the C:x/capability label May 26, 2022

facundomedica commented May 26, 2022

View reviewed changes

added some fixes from Matt's PR + changed MemStoreKey

96cfc00

facundomedica commented May 26, 2022

View reviewed changes

simapp/app.go Outdated Show resolved Hide resolved

facundomedica commented May 26, 2022

View reviewed changes

simapp/app.go Outdated Show resolved Hide resolved

facundomedica added 3 commits May 26, 2022 17:31

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

117673f

…/app-wiring-capability

fix name

7b560b1

fix name

5002e6a

facundomedica marked this pull request as ready for review May 26, 2022 20:57

facundomedica requested a review from a team as a code owner May 26, 2022 20:57

small change

5d81575

alexanderbez reviewed May 26, 2022

View reviewed changes

facundomedica added 2 commits May 26, 2022 19:17

Merge branch 'main' into facu/app-wiring-capability

f1094f5

Merge branch 'main' into facu/app-wiring-capability

229be85

facundomedica mentioned this pull request May 27, 2022

Create legacy simapp to test along new app wiring simapp #12068

Closed

4 tasks

facundomedica added 2 commits May 27, 2022 11:49

rollback MemStoreKey change

ad191ea

Merge branch 'facu/app-wiring-capability' of https://github.com/cosmo…

3e6a4d5

…s/cosmos-sdk into facu/app-wiring-capability

facundomedica mentioned this pull request May 27, 2022

migrate x/capability to use app wiring #12069

Closed

3 tasks

facundomedica and others added 5 commits May 27, 2022 12:25

Revert "rollback MemStoreKey change"

87a461d

This reverts commit ad191ea.

Merge branch 'main' into facu/app-wiring-capability

6f1ee5c

Merge branch 'main' into facu/app-wiring-capability

d63dbfd

Merge branch 'main' into facu/app-wiring-capability

15a83b0

Merge branch 'main' into facu/app-wiring-capability

efb0394

tac0turtle added WG: Framework labels May 31, 2022

facundomedica self-assigned this Jun 1, 2022

amaury1093 assigned blushi Jun 1, 2022

aaronc approved these changes Jun 1, 2022

View reviewed changes

simapp/app.go Outdated Show resolved Hide resolved

simapp/app.go Outdated Show resolved Hide resolved

facundomedica and others added 4 commits June 1, 2022 12:27

merge main

6606c18

Merge branch 'facu/app-wiring-capability' of https://github.com/cosmo…

8da73e5

…s/cosmos-sdk into facu/app-wiring-capability

add suggestions by @aaronc

27146e6

Merge branch 'main' into facu/app-wiring-capability

e9c81a9

facundomedica requested a review from aaronc June 1, 2022 17:42

aaronc reviewed Jun 1, 2022

View reviewed changes

x/capability/keeper/keeper.go Outdated Show resolved Hide resolved

facundomedica and others added 4 commits June 1, 2022 16:14

add IsSealed()

7ac22c1

Merge branch 'facu/app-wiring-capability' of https://github.com/cosmo…

14b798d

…s/cosmos-sdk into facu/app-wiring-capability

Merge branch 'main' into facu/app-wiring-capability

0ecb89f

Merge branch 'main' into facu/app-wiring-capability

aa33d9c

aaronc requested a review from kocubinski June 1, 2022 19:51

kocubinski approved these changes Jun 1, 2022

View reviewed changes

facundomedica added 2 commits June 2, 2022 10:07

fix conflicts

5bde56e

fix

30c207c

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

View reviewed changes

Merge branch 'main' into facu/app-wiring-capability

90fc1ef

facundomedica merged commit 17232fa into main Jun 2, 2022

facundomedica deleted the facu/app-wiring-capability branch June 2, 2022 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate x/capability to app wiring #12058

feat: migrate x/capability to app wiring #12058

facundomedica commented May 26, 2022 •

edited

Loading

facundomedica May 26, 2022

alexanderbez May 26, 2022

facundomedica May 26, 2022

facundomedica May 26, 2022

alexanderbez May 27, 2022

aaronc May 27, 2022

facundomedica May 26, 2022

alexanderbez May 26, 2022

facundomedica May 26, 2022

alexanderbez May 27, 2022

facundomedica May 27, 2022

alexanderbez May 27, 2022

facundomedica May 27, 2022 •

edited

Loading

facundomedica May 27, 2022

aaronc May 27, 2022

facundomedica May 27, 2022

facundomedica commented May 26, 2022

alexanderbez May 26, 2022

facundomedica May 26, 2022

alexanderbez May 26, 2022

alexanderbez May 26, 2022

blushi commented Jun 2, 2022

	// Build builds an *App instance.
	func (a AppBuilder) Build(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptions ...func(baseapp.BaseApp)) *App {
	for _, option := range a.app.baseAppOptions {
	baseAppOptions = append(baseAppOptions, option)
	}
	// TODO: when the auth module is configured, fill-in txDecoder
	bApp := baseapp.NewBaseApp(a.app.config.AppName, logger, db, nil, baseAppOptions...)
	bApp.SetCommitMultiStoreTracer(traceStore)
	bApp.SetVersion(version.Version)
	bApp.SetInterfaceRegistry(a.app.interfaceRegistry)
	bApp.MountStores(a.app.storeKeys...)

	a.app.BaseApp = bApp
	return a.app
	}

	func provideMemoryStoreKey(key container.ModuleKey, app appWrapper) *storetypes.MemoryStoreKey {
	storeKey := storetypes.NewMemoryStoreKey(fmt.Sprintf("memory:%s", key.Name()))
	registerStoreKey(app, storeKey)
	return storeKey
	}

feat: migrate x/capability to app wiring #12058

feat: migrate x/capability to app wiring #12058

Conversation

facundomedica commented May 26, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

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

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

facundomedica May 27, 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

facundomedica commented May 26, 2022

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

blushi commented Jun 2, 2022

facundomedica commented May 26, 2022 •

edited

Loading

facundomedica May 27, 2022 •

edited

Loading