feat(simapp): move simapp binary into its own go.mod #6710

gjermundgaraba · 2024-06-26T12:32:01Z

Description

closes: #3968

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.

simapp/app.go

+	// SDK module keepers
+
+	// add keepers
+	app.AccountKeeper = authkeeper.NewAccountKeeper(appCodec, runtime.NewKVStoreService(keys[authtypes.StoreKey]), authtypes.ProtoBaseAccount, maccPerms, authcodec.NewBech32Codec(sdk.Bech32MainPrefix), sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String())


simapp/app.go

+	// SDK module keepers
+
+	// add keepers
+	app.AccountKeeper = authkeeper.NewAccountKeeper(appCodec, runtime.NewKVStoreService(keys[authtypes.StoreKey]), authtypes.ProtoBaseAccount, maccPerms, authcodec.NewBech32Codec(sdk.Bech32MainPrefix), sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String())


simapp/app.go

+		logger,
+	)
+	app.StakingKeeper = stakingkeeper.NewKeeper(
+		appCodec, runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), app.AccountKeeper, app.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), authcodec.NewBech32Codec(sdk.Bech32PrefixValAddr), authcodec.NewBech32Codec(sdk.Bech32PrefixConsAddr),


simapp/app.go

+		logger,
+	)
+	app.StakingKeeper = stakingkeeper.NewKeeper(
+		appCodec, runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), app.AccountKeeper, app.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), authcodec.NewBech32Codec(sdk.Bech32PrefixValAddr), authcodec.NewBech32Codec(sdk.Bech32PrefixConsAddr),


…mod-or-e2e

colin-axner · 2024-06-27T10:33:52Z

go.mod

-	cosmossdk.io/tools/confix v0.1.1
-	cosmossdk.io/x/circuit v0.1.1
-	cosmossdk.io/x/evidence v0.1.1
-	cosmossdk.io/x/feegrant v0.1.1


wahooo!!! 🎉

simapp/test_helpers.go

simapp/upgrades/upgrades.go

testing/simapp/upgrades.go

colin-axner · 2024-07-03T09:47:53Z

I will push some changes

colin-axner · 2024-07-03T14:00:16Z

simapp/app.go

+		mockModule,
+
+		// IBC light clients
+		ibctm.NewAppModule(tmLightClientModule),


we should add wasm/callbacks to this go.mod? Maybe we can do in a followup?

sounds fine to me!

colin-axner · 2024-07-03T14:30:35Z

testing/simapp/genesis_account.go

removed any files which had unused logic. We can add back as necessary

…mod-or-e2e

colin-axner · 2024-07-03T14:38:22Z

I pruned anything that stood out as unused. Now, ibc-go/simapp is where we should built our binaries, and we can import any of our go modules. The ibc-go/testing/simapp should only be used for unit tests and cannot import any go modules not in ibc-go.

In a followup, it would be great if we could add wasm/callbacks into the ibc-go/simapp and then we can remove the redundant files related to binary creation in those modules (would also drop deps in those modules)

colin-axner · 2024-07-03T14:41:43Z

simapp/upgrades.go

-	app.UpgradeKeeper.SetUpgradeHandler(
-		upgrades.V5,
-		upgrades.CreateDefaultUpgradeHandler(app.ModuleManager, app.configurator),
-	)
-
-	// NOTE: The moduleName arg of v6.CreateUpgradeHandler refers to the auth module ScopedKeeper name to which the channel capability should be migrated from.
-	// This should be the same string value provided upon instantiation of the ScopedKeeper with app.CapabilityKeeper.ScopeToModule()
-	// See: https://github.com/cosmos/ibc-go/blob/v6.1.0/testing/simapp/app.go#L310
-	app.UpgradeKeeper.SetUpgradeHandler(
-		upgrades.V6,
-		upgrades.CreateV6UpgradeHandler(
-			app.ModuleManager,
-			app.configurator,
-			app.appCodec,
-			app.keys[capabilitytypes.ModuleName],
-			app.CapabilityKeeper,
-			ibcmock.ModuleName+icacontrollertypes.SubModuleName,
-		),
-	)
-


removed as we don't run these in e2e's any longer

Oh nice! It is much neater now with all the pruning you've done 🙏

…mod-or-e2e

damiannolan

Happy to move forward with this and see how we can work things out.

My main concern is around building "release" images and dealing with pinned go modules in the new simapp mod.

damiannolan · 2024-07-08T10:56:40Z

simapp/app.go

+		mockModule,
+
+		// IBC light clients
+		ibctm.NewAppModule(tmLightClientModule),


sounds fine to me!

damiannolan · 2024-07-08T11:00:13Z

simapp/go.mod

+replace (
+	github.com/cosmos/ibc-go/modules/capability => ../modules/capability
+	github.com/cosmos/ibc-go/v8 => ../
+)


I'm not sure how this is going to work with compatibility tests. I think this is fine for PR e2e tests etc.

It could become messy if we are building images with the latest of each library etc..
Is the plan to pin these to specific releases when building an actual simapp version image?
e.g. ibc-go-simd:v9.0.0

I just assumed that was what we were already doing?

with the compatibility tests we are basically just using the regular simd image I believe. Not using the wasm/callbacks binaries.

I think this is what we are actually already doing, it's not ideal but I think until it becomes an actual problem it is okay to leave as is

So they idea will be that we tag a new release of this module with every ibc-go major (and minor?) release?

I don't think we need to tag or release the simapp go mod via github, but having a docker image built and pushed would be a good idea!

Don't we already do that (we tag the image on release, no)? Maybe I am missing something, but since we use replace in go.mod, I am not sure why this would be any different from what we have today?

we build and push an image for every tag of ibc-go

ibc-go/.github/workflows/release.yml

Lines 35 to 61 in bce3e9e

publish-docker-image:

runs-on: ubuntu-latest

steps:

- name: Checkout

uses: actions/checkout@v4

- name: Log in to the Container registry

uses: docker/login-action@e92390c5fb421da1463c202d546fed0ec5c39f20

with:

registry: ${{ env.REGISTRY }}

username: ${{ github.actor }}

password: ${{ secrets.GITHUB_TOKEN }}

- name: Extract metadata (tags, labels) for Docker

id: meta

uses: docker/metadata-action@8e5442c4ef9f78752691e2d8f8d19755c6f78e81

with:

images: ${{ env.REGISTRY }}/cosmos/${{ env.IMAGE_NAME }}

- name: Build and push Docker image

uses: docker/build-push-action@2cdde995de11925a030ce8070c3d77a52ffcf1c0

with:

context: .

push: true

tags: ${{ steps.meta.outputs.tags }}

build-args: |

IBC_GO_VERSION=${{ github.ref_name }}

It should be no different, but I think the changes have flagged some concerns about possible compatibility issues (nothing new though you're right)

chatton

LGTM! Thanks for doing this cleanup.

I also noticed that the e2e go mod changes with a go mod tidy on this PR, guess some indirect dependencies are no longer required there. I think we can run go mod tidy in the e2e dir and commit the changes.

chatton · 2024-07-08T11:22:49Z

simapp/go.mod

+replace (
+	github.com/cosmos/ibc-go/modules/capability => ../modules/capability
+	github.com/cosmos/ibc-go/v8 => ../
+)


with the compatibility tests we are basically just using the regular simd image I believe. Not using the wasm/callbacks binaries.

I think this is what we are actually already doing, it's not ideal but I think until it becomes an actual problem it is okay to leave as is

chatton · 2024-07-08T11:23:36Z

Makefile

@@ -9,7 +9,7 @@ COMMIT := $(shell git log -1 --format='%H')
 LEDGER_ENABLED ?= true
 BINDIR ?= $(GOPATH)/bin
 BUILDDIR ?= $(CURDIR)/build
-SIMAPP = ./testing/simapp
+SIMAPP = ./simapp


love how few changes are needed here!

chatton · 2024-07-08T11:24:56Z

testing/simapp/app.go

@@ -17,15 +17,6 @@ import (
 	"cosmossdk.io/core/appmodule"


can we delete this app now?

We still use it for the integration tests, I guess?

ah, I guess this means we will need to keep it around so we don't have ibc-go importing from the simapp 🤔 ?

Yeah, I think so. It has been trimmed down, but need this still.

crodriguezvega

Thank you @gjermundgaraba and @colin-axner!

modules/apps/27-interchain-accounts/host/keeper/keeper_test.go

crodriguezvega · 2024-07-08T11:43:17Z

simapp/README.md

@@ -0,0 +1,47 @@
+# simapp


I will go through this document and open a PR if it needs to be updated.

crodriguezvega · 2024-07-08T11:46:57Z

simapp/go.mod

+replace (
+	github.com/cosmos/ibc-go/modules/capability => ../modules/capability
+	github.com/cosmos/ibc-go/v8 => ../
+)


So they idea will be that we tag a new release of this module with every ibc-go major (and minor?) release?

chatton · 2024-07-08T12:22:53Z

LGTM! Thanks for doing this cleanup.

I also noticed that the e2e go mod changes with a go mod tidy on this PR, guess some indirect dependencies are no longer required there. I think we can run go mod tidy in the e2e dir and commit the changes.

might be the reason from some e2e build failures also

…mod-or-e2e

sonarqubecloud · 2024-07-09T12:34:36Z

Quality Gate passed for 'ibc-go'

Issues
15 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

gjermundgaraba added 3 commits June 25, 2024 16:34

move simapp to a separate go module

871ea13

remove unecessary modules from testing/simapp

d7589a8

Fix tests

6221eda

gjermundgaraba linked an issue Jun 26, 2024 that may be closed by this pull request

Move simapp binary into own go.mod or e2e #3968

Closed

3 tasks

github-advanced-security bot found potential problems Jun 26, 2024

View reviewed changes

gjermundgaraba marked this pull request as ready for review June 26, 2024 12:48

gjermundgaraba requested review from colin-axner, AdityaSripal, damiannolan, chatton, crodriguezvega, DimitrisJim and bznein as code owners June 26, 2024 12:48

gjermundgaraba added 2 commits June 26, 2024 14:48

Merge branch 'main' into gjermund/3968-move-simapp-binary-into-own-go…

138fff4

…mod-or-e2e

Merge branch 'main' into gjermund/3968-move-simapp-binary-into-own-go…

d308819

…mod-or-e2e

colin-axner self-assigned this Jul 1, 2024

colin-axner reviewed Jul 3, 2024

View reviewed changes

colin-axner marked this pull request as draft July 3, 2024 09:47

colin-axner reviewed Jul 3, 2024

View reviewed changes

colin-axner added 2 commits July 3, 2024 16:07

refactor: trim out unnecessary files/code of new simapp go.mod

01e341d

refactor: remove unnecessary code in testing simapp package

d6b8bff

colin-axner reviewed Jul 3, 2024

View reviewed changes

testing/simapp/genesis_account.go Outdated

Copy link

Contributor

colin-axner Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed any files which had unused logic. We can add back as necessary

colin-axner added 2 commits July 3, 2024 16:33

remove: upgrade logic and crisis module from testing simapp

d293128

Merge branch 'main' into gjermund/3968-move-simapp-binary-into-own-go…

b39e7b4

…mod-or-e2e

colin-axner marked this pull request as ready for review July 3, 2024 14:36

colin-axner added the priority PRs that need prompt reviews label Jul 3, 2024

colin-axner reviewed Jul 3, 2024

View reviewed changes

Merge branch 'main' into gjermund/3968-move-simapp-binary-into-own-go…

8e1ae9f

…mod-or-e2e

gjermundgaraba added 2 commits July 8, 2024 09:47

Update simapp go.mod

cd4359c

Fix compile issue with new simapp

7c72478

damiannolan approved these changes Jul 8, 2024

View reviewed changes

chatton approved these changes Jul 8, 2024

View reviewed changes

crodriguezvega reviewed Jul 8, 2024

View reviewed changes

gjermundgaraba and others added 6 commits July 9, 2024 10:09

Merge branch 'main' into gjermund/3968-move-simapp-binary-into-own-go…

6221687

…mod-or-e2e

lint

82bca13

go mod tidy e2e

b2df398

Merge branch 'main' into gjermund/3968-move-simapp-binary-into-own-go…

b258687

…mod-or-e2e

Merge branch 'main' into gjermund/3968-move-simapp-binary-into-own-go…

0a4d12e

…mod-or-e2e

Fix fail after merge

c3bc711

colin-axner approved these changes Jul 9, 2024

View reviewed changes

Merge branch 'main' into gjermund/3968-move-simapp-binary-into-own-go…

da7ce7c

…mod-or-e2e

gjermundgaraba added this pull request to the merge queue Jul 9, 2024

Merged via the queue into main with commit 6bc5e8f Jul 9, 2024
32 checks passed

gjermundgaraba deleted the gjermund/3968-move-simapp-binary-into-own-gomod-or-e2e branch July 9, 2024 12:55

colin-axner mentioned this pull request Aug 29, 2024

wasm and callbacks binaries should be generated from main simapp #7231

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(simapp): move simapp binary into its own go.mod #6710

feat(simapp): move simapp binary into its own go.mod #6710

gjermundgaraba commented Jun 26, 2024 •

edited

Loading

colin-axner Jun 27, 2024

colin-axner commented Jul 3, 2024

colin-axner Jul 3, 2024 •

edited

Loading

damiannolan Jul 8, 2024

colin-axner Jul 3, 2024

colin-axner commented Jul 3, 2024

colin-axner Jul 3, 2024

gjermundgaraba Jul 8, 2024 •

edited

Loading

damiannolan left a comment

damiannolan Jul 8, 2024

damiannolan Jul 8, 2024 •

edited

Loading

gjermundgaraba Jul 8, 2024

chatton Jul 8, 2024

crodriguezvega Jul 8, 2024

damiannolan Jul 8, 2024

gjermundgaraba Jul 8, 2024

chatton Jul 8, 2024

chatton left a comment

chatton Jul 8, 2024

chatton Jul 8, 2024

chatton Jul 8, 2024

gjermundgaraba Jul 8, 2024

chatton Jul 9, 2024

gjermundgaraba Jul 9, 2024

crodriguezvega left a comment

crodriguezvega Jul 8, 2024

crodriguezvega Jul 8, 2024

chatton commented Jul 8, 2024

sonarqubecloud bot commented Jul 9, 2024

	publish-docker-image:
	runs-on: ubuntu-latest
	steps:
	- name: Checkout
	uses: actions/checkout@v4

	- name: Log in to the Container registry
	uses: docker/login-action@e92390c5fb421da1463c202d546fed0ec5c39f20
	with:
	registry: ${{ env.REGISTRY }}
	username: ${{ github.actor }}
	password: ${{ secrets.GITHUB_TOKEN }}

	- name: Extract metadata (tags, labels) for Docker
	id: meta
	uses: docker/metadata-action@8e5442c4ef9f78752691e2d8f8d19755c6f78e81
	with:
	images: ${{ env.REGISTRY }}/cosmos/${{ env.IMAGE_NAME }}

	- name: Build and push Docker image
	uses: docker/build-push-action@2cdde995de11925a030ce8070c3d77a52ffcf1c0
	with:
	context: .
	push: true
	tags: ${{ steps.meta.outputs.tags }}
	build-args: \|
	IBC_GO_VERSION=${{ github.ref_name }}

feat(simapp): move simapp binary into its own go.mod #6710

feat(simapp): move simapp binary into its own go.mod #6710

Conversation

gjermundgaraba commented Jun 26, 2024 • edited Loading

Description

Choose a reason for hiding this comment

colin-axner commented Jul 3, 2024

colin-axner Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Jul 3, 2024

Choose a reason for hiding this comment

gjermundgaraba Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan Jul 8, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton 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

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Jul 8, 2024

sonarqubecloud bot commented Jul 9, 2024

Quality Gate passed for 'ibc-go'

gjermundgaraba commented Jun 26, 2024 •

edited

Loading

colin-axner Jul 3, 2024 •

edited

Loading

gjermundgaraba Jul 8, 2024 •

edited

Loading

damiannolan Jul 8, 2024 •

edited

Loading