ignite / cli

Ignite is a CLI tool and hub designed for constructing Proof of Stake Blockchains rooted in Cosmos-SDK
https://ignite.com
Other
1.25k stars 548 forks source link

`pkg/cosmosanalysis`: support discovery of modules defined in a variable #2818

Closed jeronimoalbi closed 2 years ago

jeronimoalbi commented 2 years ago

Description The cosmosanalysis package should be able to discover registered modules also when the list of modules is defined as a variable.

This behaviour is desired to better support code generation in different blockchain applications.

Solution

Change the module discovery to recognise when the list of modules is defined as a local variable:

module.NewBasicManager(appModules...)

Or a variable defined in a different package:

module.NewBasicManager(keepers.AppModules...)
clockworkgr commented 2 years ago

Using: https://github.com/ignite/cli/pull/2820

Building clients on latest repo: SPN -> Works Gaia -> Works Crescent -> Fails (due to unrelated issue however) Akash -> Fails (modules missing) Irishub -> Fails (probably due to unrelated issue) Juno -> Fails (modules missing) Regen -> Fails (modules missing) Osmosis -> Fails (modules missing...but better than without the PR)

jeronimoalbi commented 2 years ago

In the specific case of Akash they are using a way of declaring the modules that we currently aren't able to detect. We can add such a case but if we do I would like to have a second opinion on it because I am not sure that case would be a common one.

What do you think @ilgooz, should we also handle that way of declaring the modules?

jeronimoalbi commented 2 years ago

I checked some of the blockchains where the generation failed or didn't fully generated all of the modules.

Osmosis's twap module and the Regen modules are not being generated because each of the modules are defined in a folder that doesn't contain the required RPC service implementation to be recognised as a module, their RPC implementation is located in different directory branches and we expect to locate them at the module's definition level or within a child directory. Some example modules:

As described in the previous commend Akash also have the issue with the module being defined at a different level than the RPC service implementation.

The issue with Juno is that the Go module definition is using versioning and the proto files are not using it in the go_package options and because of that we are not able to properly identify the custom modules. The Go import paths in the go_package options don't have the version suffix.

Crescent in the other hand have an issue that makes protoc fail because one of its dependencies have a proto file that is importing the same package twice, but it also uses Go module versioning which is not being used in the proto files.

One possible solution to recognise the custom modules is to look for the RPC implementation in all of the files defined in the x/custom_module directory instead of inside the package that defines the module, but it could make the generation considerably slower, so I would like to have a second opinion before making such a change.

Regarding the solution for the versioning in the Go modules and the go_package proto option I still need to figure it out if it's actually possible but I would appreciate any opinion on that too. Cosmos's ibc-go is using the version suffix in the proto files.

jeronimoalbi commented 2 years ago

Regarding the last two solutions described in the last comment, both were implemented.

Custom modules now handle the case where the RPC service implementation is defined in a different tree branch within the module.

Versioning in the Go import paths is now handled too so discovery in versioned modules works when the Go import paths in the proto are not versioned.

The generation results:

Further explanation can be found here.

@clockworkgr it would be great if you could take another look to the TS client generation result for the ones that are working. Thank you!

queencre commented 1 year ago

Thanks for testing Crescent core and writing this up @jeronimoalbi. I know this issue is already closed and there was an issue with Crescent Core. I would like to let you know that it has been resolved in this https://github.com/crescent-network/crescent/pull/101.

tbruyelle commented 1 year ago

Thanks for testing Crescent core and writing this up @jeronimoalbi. I know this issue is already closed and there was an issue with Crescent Core. I would like to let you know that it has been resolved in this crescent-network/crescent#101.

Hi @queencre, if I run our package cosmosanalysis on your app/app.go (main branch), I get this list of modules :

                            "github.com/cosmos/cosmos-sdk/x/auth",
                "github.com/cosmos/cosmos-sdk/x/genutil",
                "github.com/cosmos/cosmos-sdk/x/bank",
                "github.com/cosmos/cosmos-sdk/x/capability",
                "github.com/cosmos/cosmos-sdk/x/staking",
                "github.com/crescent-network/crescent/v4/x/mint",
                "github.com/cosmos/cosmos-sdk/x/distribution",
                "github.com/cosmos/cosmos-sdk/x/gov",
                "github.com/cosmos/cosmos-sdk/x/params",
                "github.com/cosmos/cosmos-sdk/x/crisis",
                "github.com/cosmos/cosmos-sdk/x/slashing",
                "github.com/cosmos/cosmos-sdk/x/feegrant/module",
                "github.com/cosmos/cosmos-sdk/x/authz/module",
                "github.com/cosmos/ibc-go/v3/modules/core",
                "github.com/cosmos/ibc-go/v3/modules/apps/transfer",
                "github.com/cosmos/cosmos-sdk/x/upgrade",
                "github.com/cosmos/cosmos-sdk/x/evidence",
                "github.com/cosmos/cosmos-sdk/x/auth/vesting",
                "github.com/tendermint/budget/x/budget",
                "github.com/crescent-network/crescent/v4/x/farming",
                "github.com/crescent-network/crescent/v4/x/liquidity",
                "github.com/crescent-network/crescent/v4/x/liquidstaking",
                "github.com/crescent-network/crescent/v4/x/liquidfarming",
                "github.com/crescent-network/crescent/v4/x/claim",
                "github.com/crescent-network/crescent/v4/x/marketmaker",
                "github.com/crescent-network/crescent/v4/x/lpfarm",
                "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts",
                "github.com/cosmos/cosmos-sdk/x/auth/tx",
                "github.com/cosmos/cosmos-sdk/client/grpc/tmservice",

Can you tell me what's missing ?

queencre commented 1 year ago

@tbruyelle I don't think you are missing anything. Those are the exact custom modules that we have. FYI, Crescent Mainnet is currently running with v3.0.0 and the main branch is the latest development branch.

tbruyelle commented 1 year ago

OK I ran the same algorithm on v3.0.0, here is the diff with what I got from main. Is there anything wrong this time ?

                "github.com/cosmos/cosmos-sdk/x/bank",
                "github.com/cosmos/cosmos-sdk/x/capability",
                "github.com/cosmos/cosmos-sdk/x/staking",
-               "github.com/crescent-network/crescent/v4/x/mint",
+               "github.com/crescent-network/crescent/v3/x/mint",
                "github.com/cosmos/cosmos-sdk/x/distribution",
                "github.com/cosmos/cosmos-sdk/x/gov",
                "github.com/cosmos/cosmos-sdk/x/params",
                            "github.com/cosmos/cosmos-sdk/x/crisis",
                "github.com/cosmos/cosmos-sdk/x/slashing",
                "github.com/cosmos/cosmos-sdk/x/feegrant/module",
                "github.com/cosmos/cosmos-sdk/x/authz/module",
-               "github.com/cosmos/ibc-go/v3/modules/core",
-               "github.com/cosmos/ibc-go/v3/modules/apps/transfer",
+               "github.com/cosmos/ibc-go/v2/modules/core",
                "github.com/cosmos/cosmos-sdk/x/upgrade",
                "github.com/cosmos/cosmos-sdk/x/evidence",
+               "github.com/cosmos/ibc-go/v2/modules/apps/transfer",
                "github.com/cosmos/cosmos-sdk/x/auth/vesting",
                "github.com/tendermint/budget/x/budget",
-               "github.com/crescent-network/crescent/v4/x/farming",
-               "github.com/crescent-network/crescent/v4/x/liquidity",
-               "github.com/crescent-network/crescent/v4/x/liquidstaking",
-               "github.com/crescent-network/crescent/v4/x/liquidfarming",
-               "github.com/crescent-network/crescent/v4/x/claim",
-               "github.com/crescent-network/crescent/v4/x/marketmaker",
-               "github.com/crescent-network/crescent/v4/x/lpfarm",
-               "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts",
+               "github.com/crescent-network/crescent/v3/x/farming",
+               "github.com/crescent-network/crescent/v3/x/liquidity",
+               "github.com/crescent-network/crescent/v3/x/liquidstaking",
+               "github.com/crescent-network/crescent/v3/x/liquidfarming",
+               "github.com/crescent-network/crescent/v3/x/claim",
+               "github.com/crescent-network/crescent/v3/x/marketmaker",
+               "github.com/crescent-network/crescent/v3/x/lpfarm",
                "github.com/cosmos/cosmos-sdk/x/auth/tx",
                "github.com/cosmos/cosmos-sdk/client/grpc/tmservice",

If needed this is the complete module list for v3.0.0 :

                            "github.com/cosmos/cosmos-sdk/x/auth",
                "github.com/cosmos/cosmos-sdk/x/genutil",
                "github.com/cosmos/cosmos-sdk/x/bank",
                "github.com/cosmos/cosmos-sdk/x/capability",
                "github.com/cosmos/cosmos-sdk/x/staking",
                "github.com/crescent-network/crescent/v3/x/mint",
                "github.com/cosmos/cosmos-sdk/x/distribution",
                "github.com/cosmos/cosmos-sdk/x/gov",
                "github.com/cosmos/cosmos-sdk/x/params",
                "github.com/cosmos/cosmos-sdk/x/crisis",
                "github.com/cosmos/cosmos-sdk/x/slashing",
                "github.com/cosmos/cosmos-sdk/x/feegrant/module",
                "github.com/cosmos/cosmos-sdk/x/authz/module",
                "github.com/cosmos/ibc-go/v2/modules/core",
                "github.com/cosmos/cosmos-sdk/x/upgrade",
                "github.com/cosmos/cosmos-sdk/x/evidence",
                "github.com/cosmos/ibc-go/v2/modules/apps/transfer",
                "github.com/cosmos/cosmos-sdk/x/auth/vesting",
                "github.com/tendermint/budget/x/budget",
                "github.com/crescent-network/crescent/v3/x/farming",
                "github.com/crescent-network/crescent/v3/x/liquidity",
                "github.com/crescent-network/crescent/v3/x/liquidstaking",
                "github.com/crescent-network/crescent/v3/x/liquidfarming",
                "github.com/crescent-network/crescent/v3/x/claim",
                "github.com/crescent-network/crescent/v3/x/marketmaker",
                "github.com/crescent-network/crescent/v3/x/lpfarm",
                "github.com/cosmos/cosmos-sdk/x/auth/tx",
                "github.com/cosmos/cosmos-sdk/client/grpc/tmservice",
jeronimoalbi commented 1 year ago

Thanks for testing Crescent core and writing this up @jeronimoalbi. I know this issue is already closed and there was an issue with Crescent Core. I would like to let you know that it has been resolved in this crescent-network/crescent#101.

Thank you very much for the feedback @queencre!

Just generated the Typescript code and I can see that it works, all the basic modules that were registered in the app are discovered 🎉 .

queencre commented 1 year ago

@tbruyelle It looks fine to me.