Skip to content

Conversation

@jeronimoalbi
Copy link
Member

Closes #2818

@jeronimoalbi jeronimoalbi added the type:new To implement new feature. label Sep 9, 2022
@jeronimoalbi jeronimoalbi self-assigned this Sep 9, 2022
@jeronimoalbi jeronimoalbi changed the base branch from develop to ts-client September 9, 2022 17:24
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

Visit the preview URL for this PR (updated for commit bb4ed5e):

https://ignite-go-docs--pr2820-feat-cosmosanalysis-mv8tql5g.web.app

(expires Tue, 20 Sep 2022 09:46:09 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Base automatically changed from ts-client to develop September 12, 2022 08:16
@clockworkgr
Copy link
Collaborator

Getting this kind of errors :

Input is shadowed in the --proto_path by "/home/clockwork/go/pkg/mod/github.com/osmosis-labs/wasmd@v0.28.0-osmo-v12.1/third_party/proto/cosmos/base/tendermint/v1beta1/query.proto". Either use the latter file as your input or reorder the --proto_path so that the former file's location comes first.

According to this: google/protobuf-gradle-plugin#405 (comment) it's because we're including the same proto files multiple times somewhere

@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Sep 19, 2022

Getting this kind of errors :

Input is shadowed in the --proto_path by "/home/clockwork/go/pkg/mod/github.com/osmosis-labs/wasmd@v0.28.0-osmo-v12.1/third_party/proto/cosmos/base/tendermint/v1beta1/query.proto". Either use the latter file as your input or reorder the --proto_path so that the former file's location comes first.

@clockworkgr how can I replicate it?

@clockworkgr
Copy link
Collaborator

Getting this kind of errors :
Input is shadowed in the --proto_path by "/home/clockwork/go/pkg/mod/github.com/osmosis-labs/wasmd@v0.28.0-osmo-v12.1/third_party/proto/cosmos/base/tendermint/v1beta1/query.proto". Either use the latter file as your input or reorder the --proto_path so that the former file's location comes first.

@clockworkgr how can I replicate it?

clone latest osmosis...run ignite g ts-client

@jeronimoalbi
Copy link
Member Author

Getting this kind of errors :
Input is shadowed in the --proto_path by "/home/clockwork/go/pkg/mod/github.com/osmosis-labs/wasmd@v0.28.0-osmo-v12.1/third_party/proto/cosmos/base/tendermint/v1beta1/query.proto". Either use the latter file as your input or reorder the --proto_path so that the former file's location comes first.

@clockworkgr how can I replicate it?

clone latest osmosis...run ignite g ts-client

It is working for me with c77822b which is the latest change in the feat/cosmosanalysis-module-discovery branch so there might be some other condition that leads to that error.

@clockworkgr
Copy link
Collaborator

Getting this kind of errors :
Input is shadowed in the --proto_path by "/home/clockwork/go/pkg/mod/github.com/osmosis-labs/wasmd@v0.28.0-osmo-v12.1/third_party/proto/cosmos/base/tendermint/v1beta1/query.proto". Either use the latter file as your input or reorder the --proto_path so that the former file's location comes first.

@clockworkgr how can I replicate it?

clone latest osmosis...run ignite g ts-client

It is working for me with c77822b which is the latest change in the feat/cosmosanalysis-module-discovery branch so there might be some other condition that leads to that error.

is it generating all used modules for you? i dont see auth/bank/staking etc in my output

@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Sep 19, 2022

is it generating all used modules for you? i dont see auth/bank/staking etc in my output

I have to take a look at it

Update: I have checked different cases and the modules that are failing are described in this comment.

jeronimoalbi added 7 commits September 21, 2022 12:10
Some apps define the Go import path in the proto files without the
version suffix while the Go module uses a version suffix. This change
allows the custom module discovery to work in such case.
Changeset modifies the module discovery to search inside custom module
folders for the RPC services implementation.
The change allows discovery for blockchain apps that are versioned. It
also handles the cases where custom app modules are imported using a Go
import that might or might not contain the version suffix.
The protp package is required to generate code for other blockchains
like Gaia, otherwise generation would fail because the proto option
`cosmos_proto.scalar` won't be available while generating the typescript
code for some cosmos SDK modules. Cosmos SDK removed the `cosmos_proto`
from its third party proto includes in changeset
cosmos/cosmos-sdk@0cb7fd0
@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Sep 23, 2022

With the latest changes I got to a point where:

  • SPN (works)
  • Osmosis (works)
  • Juno (works)
  • Gaia (works)
  • Akash (fails)
  • Regen (fails)
  • Crescent (fails)

Akash fails because the way the modules are defined which is not the usual way and it could be handled by the CLI.

Regen have an issue because of a Cosmos ORM proto file imported in the ecocredit module. The file is not defining a go_package option. I think it's not something we can handle in the CLI:

unable to determine Go import path for "cosmos/orm/v1/orm.proto"

Please specify either:
	• a "go_package" option in the .proto source file, or
	• a "M" argument on the command line.

See https://developers.google.com/protocol-buffers/docs/reference/go-generated#package for more information.

Crescent have an issue with the budget module, specifically with the budget.proto file from Tendermint budget module which has a duplicated import that breaks the generation, so we can't handle it in the CLI.

Regarding Gaia "working" before what was working was the generation of third party modules while the custom ones were not discovered, after one of the changes the custom modules were discovered and the generation was failing because the cosmos_proto dependency was not available at some point. I tried to find the best way to fix Gaias's generation given that it's an importan blockchain and the only solution I found is to include the cosmos_proto package in the CLI which I think it's not ideal. The cosmos_proto was removed from the Cosmos SDK in cosmos/cosmos-sdk@0cb7fd0. Gaia has an older third party implementation that doesn't contain the required scalar proto field option.
Gaia's TS client generation error without the cosmos_proto included in the CLI:

cannot build app:

	error while running command /var/folders/mz/2tv1_svd3yv48szrgmhqwltr0000gn/T/protoc1440136535 -I /var/folders/mz/2tv1_svd3yv48szrgmhqwltr0000gn/T/531218074 --plugin /var/folders/mz/2tv1_svd3yv48szrgmhqwltr0000gn/T/ts_proto_plugin1793840093/protoc-gen-ts_proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.46.1/proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.46.1/third_party/proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.46.1/proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.46.1/third_party/proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/gravity-devs/liquidity/v2@v2.0.0/proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/gravity-devs/liquidity/v2@v2.0.0/third_party/proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/strangelove-ventures/packet-forward-middleware/v2@v2.1.4-0.20220802012200-5a62a55a7f1d/proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/strangelove-ventures/packet-forward-middleware/v2@v2.1.4-0.20220802012200-5a62a55a7f1d/third_party/proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/ibc-go/v5@v5.0.0-rc2/proto -I /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/ibc-go/v5@v5.0.0-rc2/third_party/proto --ts_proto_out=. /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.46.1/proto/cosmos/gov/v1beta1/genesis.proto /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.46.1/proto/cosmos/gov/v1beta1/gov.proto /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.46.1/proto/cosmos/gov/v1beta1/query.proto /Users/jeronimoalbi/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.46.1/proto/cosmos/gov/v1beta1/tx.proto --ts_proto_opt=snakeToCamel=true: cosmos/base/v1beta1/coin.proto:20:8: Option "(cosmos_proto.scalar)" unknown. Ensure that your proto definition file imports the proto which defines the option.
cosmos/base/v1beta1/coin.proto:32:8: Option "(cosmos_proto.scalar)" unknown. Ensure that your proto definition file imports the proto which defines the option.
cosmos/base/v1beta1/coin.proto:37:19: Option "(cosmos_proto.scalar)" unknown. Ensure that your proto definition file imports the proto which defines the option.
cosmos/base/v1beta1/coin.proto:42:19: Option "(cosmos_proto.scalar)" unknown. Ensure that your proto definition file imports the proto which defines the option.
cosmos/gov/v1beta1/gov.proto:4:1: Import "cosmos/base/v1beta1/coin.proto" was not found or had errors.
cosmos/gov/v1beta1/gov.proto:64:12: "cosmos.base.v1beta1.Coin" is not defined.
cosmos/gov/v1beta1/gov.proto:81:12: "cosmos.base.v1beta1.Coin" is not defined.
cosmos/gov/v1beta1/gov.proto:155:12: "cosmos.base.v1beta1.Coin" is not defined.
cosmos/gov/v1beta1/genesis.proto:6:1: Import "cosmos/gov/v1beta1/gov.proto" was not found or had errors.
cosmos/gov/v1beta1/genesis.proto:15:12: "Deposit" is not defined.
cosmos/gov/v1beta1/genesis.proto:17:12: "Vote" is not defined.
cosmos/gov/v1beta1/genesis.proto:19:12: "Proposal" is not defined.
cosmos/gov/v1beta1/genesis.proto:21:3: "DepositParams" is not defined.
cosmos/gov/v1beta1/genesis.proto:23:3: "VotingParams" is not defined.
cosmos/gov/v1beta1/genesis.proto:25:3: "TallyParams" is not defined.

Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com>
tbruyelle
tbruyelle previously approved these changes Sep 27, 2022
tbruyelle
tbruyelle previously approved these changes Sep 27, 2022
tbruyelle
tbruyelle previously approved these changes Sep 28, 2022
Copy link
Contributor

@aljo242 aljo242 left a comment

Choose a reason for hiding this comment

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

Can we add some tests for this functionality in this or another PR?

@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Sep 28, 2022

Can we add some tests for this functionality in this or another PR?

Which functionality? If it's for the new module discovery cases they are tested.

@aljo242
Copy link
Contributor

aljo242 commented Sep 28, 2022

Can we add some tests for this functionality in this or another PR?

Which functionality?

We have added a lot of untested utility functions in cosmosanalysis and aren't covering this use case with a larger integration test.

@aljo242 aljo242 merged commit 03e8081 into develop Sep 30, 2022
@aljo242 aljo242 deleted the feat/cosmosanalysis-module-discovery branch September 30, 2022 12:14
Jchicode pushed a commit to Jchicode/cli that referenced this pull request Aug 9, 2023
…ariable (ignite#2820)

* feat: change module discovery to resolve modules from a variable

* feat: change module discovery to resolve modules from a pkg variable

The changeset resolves the modules defined in a variable within the same
package but in a different file.

* feat: change module discovery to resolve modules from a other pkg variable

* feat: discover app modules created using a function call

* refactor: simplify module discovery

* chore: update changelog

* chore: minor code improvements and documentation

* fix: cosmosanalysis to work when proto Go import is missing version

Some apps define the Go import path in the proto files without the
version suffix while the Go module uses a version suffix. This change
allows the custom module discovery to work in such case.

* refactor: improve discovery for different custom module layouts

Changeset modifies the module discovery to search inside custom module
folders for the RPC services implementation.

* fix: remove default from typescript client generate output flag

* refactor: improve module discovery to work with versioned Go modules

The change allows discovery for blockchain apps that are versioned. It
also handles the cases where custom app modules are imported using a Go
import that might or might not contain the version suffix.

* feat: add `cosmos_proto` to protoc includes

The protp package is required to generate code for other blockchains
like Gaia, otherwise generation would fail because the proto option
`cosmos_proto.scalar` won't be available while generating the typescript
code for some cosmos SDK modules. Cosmos SDK removed the `cosmos_proto`
from its third party proto includes in changeset
cosmos/cosmos-sdk@0cb7fd0

* feat: add error support to `NewBasicManager` call discovery

* review: add comment to basic manager discovery

Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com>

* chore: fix syntax

* fix CL

* test: registered modules for osmosis

* test: add test for standard module discovery cases

This case discover registered modules registered as NewBasicManager
function arguments.

* test: add test for module discovery with API routes

* tests: add tests for `cosmosanalysis` module helper functions

* tests: add discovery test for versioned apps

Blockchain apps are versioned when the import path in the `go.mod` file
contains a version suffix.

* test: registered modules for current spn, gaia and juno

Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com>
Co-authored-by: Thomas Bruyelle <thomas.bruyelle@tendermint.com>
Co-authored-by: Alex Johnson <alex@shmeeload.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:new To implement new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

5 participants