deps: upgrade capability to sdk v0.50 and remove ibc-go dependency #4068

damiannolan · 2023-07-12T07:16:00Z

Description

This PR upgrades the capability module to sdk v0.50 eden release.
It removes the dependency on ibc-go testing/simapp and refactors the tests of interest to not depend on a simulation app.
Instead it uses a test a context and in-memory db in order to accomplish the same level of testing provided by simapp.

Note, I have tried to clean up and add inline code comments to capability_test.go to make it easier to understand.

closes: #4055

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

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.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

…f simapp

damiannolan

Added some context for review. More testcases could be cleaned up but I left as is to reduce diffs. The main one of concern was TestInitializeMemstore which I've cleaned up

damiannolan · 2023-07-12T07:35:10Z

modules/capability/genesis_test.go

+	// InitGenesis must be called in order to set the intial index to 1.
+	capability.InitGenesis(suite.ctx, *suite.keeper, *types.DefaultGenesis())


InitGenesis has to be called directly here in order to set the initial capability index to 1.

This used to be happening handled implicitly when testing with a simapp. This caught me off guard for a while as after removing simapp this test would fail below when trying to lookup the first capability after genesis is reinitialised.

The reasoning for this is that without setting the initial index to 1 - cap1 created on line 19 would be at index 0, and ExportGenesis starts the loop index here at 1. So after exporting and then reinitialising the gen state we'd lose first capabiliity.

damiannolan · 2023-07-12T07:37:36Z

modules/capability/capability_test.go

-	suite.keeper = keeper
-	suite.cdc = cdc
-	suite.module = capability.NewAppModule(cdc, *keeper, false)
+	encodingCfg := moduletestutil.MakeTestEncodingConfig(capability.AppModuleBasic{})


Pretty nice way to create a test encoding config 👍

damiannolan · 2023-07-12T07:43:34Z

modules/capability/capability_test.go

+func (suite *CapabilityTestSuite) NewTestContext() sdk.Context {
+	db := dbm.NewMemDB()
+	cms := store.NewCommitMultiStore(db, log.NewNopLogger(), metrics.NewNoOpMetrics())
+	cms.MountStoreWithDB(suite.storeKey, storetypes.StoreTypeIAVL, db)
+	cms.MountStoreWithDB(suite.memStoreKey, storetypes.StoreTypeMemory, db)
+	cms.MountStoreWithDB(suite.mockMemStoreKey, storetypes.StoreTypeMemory, db)
+
+	err := cms.LoadLatestVersion()
+	suite.Require().NoError(err)
+
+	return sdk.NewContext(cms, tmproto.Header{}, false, log.NewNopLogger())
 }


This has been adapted from here.

I'm unsure why the test helpers in the sdk do not support adding mem store keys, or even mounting multiple keys (I guess nobody has run into an issue with the current code yet).

I was going to make a PR to the sdk to add some extra support for this, but I felt it was cleaner and quicker to adapt it and maintain it here.

Opened cosmos/cosmos-sdk#16998

damiannolan · 2023-07-12T07:49:44Z

modules/capability/capability_test.go

 }

 // The following test case mocks a specific bug discovered in https://github.com/cosmos/cosmos-sdk/issues/9800
 // and ensures that the current code successfully fixes the issue.
+// This test emulates statesync by firstly populating persisted state by creating a new scoped keeper and capability.
+// In-memory storage is then discarded by creating a new capability keeper and app module using a mock memstore key.
+// BeginBlock is then called to populate the new in-memory store using the persisted state.
 func (suite *CapabilityTestSuite) TestInitializeMemStore() {


I spent a good bit of time trying to parse this testcase. So I've added a bunch of in-line comments as well as extra info to the godoc above. This was initially added by @AdityaSripal in cosmos/cosmos-sdk#9835 but has since seen a whole bunch of devs chopping into the code.

Hopefully its now clear what its doing and is easier to maintain.

Thank you for this!

damiannolan · 2023-07-12T07:50:33Z

modules/capability/capability_test.go

+	prevGas := ctx.GasMeter().GasConsumed()
 	prevBlockGas := ctx.BlockGasMeter().GasConsumed()
-	prevGas := ctx.BlockGasMeter().GasConsumed()


prevGas here was incorrect, it was being set to the same value as prevBlockGas

crodriguezvega · 2023-07-12T10:09:27Z

Thank you so much for handling the upgrade of the capability module, @damiannolan. This PR will supersede #3978, right? So that one can be closed?

damiannolan · 2023-07-12T10:10:50Z

Thank you so much for handling the upgrade of the capability module, @damiannolan. This PR will supersede #3978, right? So that one can be closed?

Yep, indeed we can close #3978

chatton

LGTM nice investigation ❤️

colin-axner

Damn, excellent work! This is a pretty epic pr if I have ever seen one 🔥

colin-axner · 2023-07-13T09:49:01Z

modules/capability/capability_test.go

 }

 // The following test case mocks a specific bug discovered in https://github.com/cosmos/cosmos-sdk/issues/9800
 // and ensures that the current code successfully fixes the issue.
+// This test emulates statesync by firstly populating persisted state by creating a new scoped keeper and capability.
+// In-memory storage is then discarded by creating a new capability keeper and app module using a mock memstore key.
+// BeginBlock is then called to populate the new in-memory store using the persisted state.
 func (suite *CapabilityTestSuite) TestInitializeMemStore() {


Thank you for this!

modules/capability/capability_test.go

…tialised

damiannolan · 2023-07-13T18:41:48Z

cc @faddat 👀

should be able to pick this up in #3883 using a replace directive with a local pin if you sync the latest from main, will also tag this soon and then we can move to that!

edit: tagged @ modules/capability/v1.0.0-rc2 https://github.com/cosmos/ibc-go/tree/modules/capability/v1.0.0-rc2

…4068) * adding testing/simapp dir for testing purposes * upgrade to sdk v0.50.0-alpha.1 and rm dep on ibc-go simapp * refactor and improve capability tests to use test helpers in favour of simapp * rm simapp and go mod tidy * update godoc * assert the transfer capability does not exist until memstore is reinitialised (cherry picked from commit 49cdfc5) # Conflicts: # modules/capability/capability_test.go # modules/capability/genesis_test.go # modules/capability/go.mod # modules/capability/go.sum # modules/capability/module.go # modules/capability/simulation/decoder_test.go

…ackport #4068) (#4083) * deps: upgrade capability to sdk v0.50 and remove ibc-go dependency (#4068) * adding testing/simapp dir for testing purposes * upgrade to sdk v0.50.0-alpha.1 and rm dep on ibc-go simapp * refactor and improve capability tests to use test helpers in favour of simapp * rm simapp and go mod tidy * update godoc * assert the transfer capability does not exist until memstore is reinitialised (cherry picked from commit 49cdfc5) # Conflicts: # modules/capability/capability_test.go # modules/capability/genesis_test.go # modules/capability/go.mod # modules/capability/go.sum # modules/capability/module.go # modules/capability/simulation/decoder_test.go * addressing merge conflicts * pin goleveldb using replace * updating to go 1.20 in capability workflows * just use the workflow from main.. please work --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com>

damiannolan added 6 commits July 10, 2023 22:47

adding testing/simapp dir for testing purposes

4173fab

upgrade to sdk v0.50.0-alpha.1 and rm dep on ibc-go simapp

7e6a558

refactor and improve capability tests to use test helpers in favour o…

0c1de75

…f simapp

rm simapp and go mod tidy

91e5df5

update godoc

df4640c

Merge branch 'main' into damian/capability-upgrade

09469f7

damiannolan mentioned this pull request Jul 12, 2023

[WIP] deps: upgrade capability to sdk v0.50 #4057

Closed

9 tasks

damiannolan marked this pull request as ready for review July 12, 2023 07:22

damiannolan requested review from colin-axner, AdityaSripal, crodriguezvega, charleenfei, chatton and DimitrisJim as code owners July 12, 2023 07:22

damiannolan mentioned this pull request Jul 12, 2023

Remove dependency on ibc-go from capability module #4055

Closed

3 tasks

damiannolan commented Jul 12, 2023

View reviewed changes

damiannolan added the capability Issues for capability modules label Jul 12, 2023

damiannolan mentioned this pull request Jul 12, 2023

imp: attempt to separate capability from 50 upgrade #3978

Closed

chatton approved these changes Jul 12, 2023

View reviewed changes

colin-axner approved these changes Jul 13, 2023

View reviewed changes

damiannolan added 2 commits July 13, 2023 17:22

assert the transfer capability does not exist until memstore is reini…

59d729d

…tialised

Merge branch 'main' into damian/capability-upgrade

850b70f

damiannolan merged commit 49cdfc5 into main Jul 13, 2023

damiannolan deleted the damian/capability-upgrade branch July 13, 2023 18:13

damiannolan added the backport-capability-to-v1.0.x label Jul 14, 2023

mergify bot mentioned this pull request Jul 14, 2023

deps: upgrade capability to sdk v0.50 and remove ibc-go dependency (backport #4068) #4083

Merged

damiannolan mentioned this pull request Jul 14, 2023

[Feature]: Add support for test sdk.Context with n store keys, plus mem store keys cosmos/cosmos-sdk#16998

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: upgrade capability to sdk v0.50 and remove ibc-go dependency #4068

deps: upgrade capability to sdk v0.50 and remove ibc-go dependency #4068

damiannolan commented Jul 12, 2023

damiannolan left a comment

damiannolan Jul 12, 2023

damiannolan Jul 12, 2023

damiannolan Jul 12, 2023

damiannolan Jul 14, 2023

damiannolan Jul 12, 2023

colin-axner Jul 13, 2023

damiannolan Jul 12, 2023

crodriguezvega commented Jul 12, 2023

damiannolan commented Jul 12, 2023

chatton left a comment

colin-axner left a comment

colin-axner Jul 13, 2023

damiannolan commented Jul 13, 2023 •

edited

Loading

		// InitGenesis must be called in order to set the intial index to 1.
		capability.InitGenesis(suite.ctx, suite.keeper, types.DefaultGenesis())

deps: upgrade capability to sdk v0.50 and remove ibc-go dependency #4068

deps: upgrade capability to sdk v0.50 and remove ibc-go dependency #4068

Conversation

damiannolan commented Jul 12, 2023

Description

damiannolan left a 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

crodriguezvega commented Jul 12, 2023

damiannolan commented Jul 12, 2023

chatton left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan commented Jul 13, 2023 • edited Loading

damiannolan commented Jul 13, 2023 •

edited

Loading