Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: restore runtime app API registered module discovery #3785

Merged
merged 21 commits into from
Dec 4, 2023

Conversation

jeronimoalbi
Copy link
Member

Fixes #3783

@jeronimoalbi jeronimoalbi added the type:bug Something isn't working label Nov 28, 2023
@jeronimoalbi jeronimoalbi self-assigned this Nov 28, 2023
@jeronimoalbi jeronimoalbi added the skip-changelog Don't check changelog for new entries label Nov 28, 2023
@jeronimoalbi
Copy link
Member Author

@clockworkgr I think that the TS client code is properly generated but there are other issues with the TS client itself I believe. Would you be able to take a look?

I got these errors:

cosmos.base.tendermint.v1beta1/module.ts(15,10): error TS2440: Import declaration conflicts with local declaration of 'Module'.
cosmos.base.tendermint.v1beta1/module.ts(366,43): error TS2339: Property 'fromPartial' does not exist on type '(test: IgniteClient) => { module: { CosmosBaseTendermintV1Beta1: SDKModule; }; registry: [string, GeneratedType][]; }'.
cosmos.base.tendermint.v1beta1/module.ts(620,79): error TS2339: Property 'fromPartial' does not exist on type '(test: IgniteClient) => { module: { CosmosBaseTendermintV1Beta1: SDKModule; }; registry: [string, GeneratedType][]; }'.

It seems that the fix is a quick one, there is a symbol name clash.

@jeronimoalbi jeronimoalbi changed the title fix: restore runtime app API registered modules fix: restore runtime app API registered module discovery Nov 28, 2023
@clockworkgr
Copy link
Collaborator

We're still missing modules here:

nft
upgrade
feegrant
ibc core
ibc channel
ibc connection

@jeronimoalbi
Copy link
Member Author

We're still missing modules here:

nft upgrade feegrant ibc core ibc channel ibc connection

The IBC ones are not discovered because there is a different wiring for them until they are updated, I'm working on changes to discover them.

A part from that nft, feegrant and upgrade are standalone Go modules now, imported from cosmossdk.io so that might be related to the issue, but I'm not sure yet.

@clockworkgr have you noticed other missing modules?

@clockworkgr
Copy link
Collaborator

Just those 6 I think

This is a temporary feature that should be removed once IBC modules
are injected inteas of being wired using the definitions from the
scafolded app file `app/ibc.go`.
The removed snippet was not able to handle some Go import paths
which were removed, like the IBC ones, also causing that the TS
client generation was incomplete because of the missing modules.
The "cosmossdk.io/x" Go packages MUST search proto files in the Cosmos
SDK Go package because these packages don't have the proto files included
so they can't be discovered.
Discovery changed to use go mod download instead of using Go's module
path which might not be present in some context.
@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Dec 1, 2023

I think the TS client generation issues are solved already but the CI checks are taking really long to complete. I'm not sure if it's only this PR or is a general issue with the runners.

With these changes the "cosmossdk.io/x" Go packages search proto files within the Cosmos SDK Go package because these packages don't have the proto files included so they can't otherwise be discovered.

Custom wired IBC modules from the app/ibc.go file are discovered too.

TS client generated code was missing some modules which are now generated, like tx, feegrant, upgrade, the ibc.core ones and a couple others.

I will remove the Draft status and sync with main once the CI checks pass successfully.

@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Dec 1, 2023

@clockworkgr would you take a look to the TS generated code?

I see that all the modules are generated now but we are missing some TS client fixes.
Do you want to push the fixes to this branch or will you create a different PR?

@github-actions github-actions bot added component:ci CI/CD workflow and automated jobs. component:configs component:packages labels Dec 2, 2023
@clockworkgr
Copy link
Collaborator

@clockworkgr would you take a look to the TS generated code?

I see that all the modules are generated now but we are missing some TS client fixes. Do you want to push the fixes to this branch or will you create a different PR?

Ok..

There is one bug with how npm install treats the symlinked client which is part of the ignite/web templates. I'll look into it and fix it.

There are the ts template updates which I'm going to push here.

But the most important issue has to do with the module filepath for the cosmossdk.io modules.

Here:

"resolveFile": func(fullPath string) string {
rel, _ := filepath.Rel(protoPath, fullPath)
rel = strings.TrimSuffix(rel, ".proto")
return rel
},

this worked to "copy" the folder structure of the proto files into the types/ subfolder. to refer to the appropriate generated types.

For the cosmossdk.io ones, this now results in things like:
import { SoftwareUpgradeProposal } from "./types/../../../../github.com/cosmos/[email protected]/proto/cosmos/upgrade/v1beta1/upgrade";

instead of:
import { SoftwareUpgradeProposal } from "./types/cosmos/upgrade/v1beta1/upgrade";

Any ideas how to best fix this?

@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Dec 2, 2023

Any ideas how to best fix this?

Working on it 👍

Update: Fixed in b9b090a

ignite/pkg/xstrings/xstrings_test.go Show resolved Hide resolved
ignite/pkg/gomodule/gomodule.go Outdated Show resolved Hide resolved
ignite/pkg/cosmosanalysis/module/module.go Outdated Show resolved Hide resolved
@clockworkgr
Copy link
Collaborator

Any ideas how to best fix this?

Working on it 👍

Update: Fixed in b9b090a

Ok. Will check and confirm

@clockworkgr
Copy link
Collaborator

Seems to generate all modules except for NFT.. Is that expected? do we add nft to keepers by default?

I see an issue where the Paginated field of HTTPQueries is not set properly and all queries come up as non-paginated...Not sure where that error stems from though

@jeronimoalbi
Copy link
Member Author

Seems to generate all modules except for NFT.. Is that expected? do we add nft to keepers by default?

Yes, the NFT module is not scaffolded by default.

I see an issue where the Paginated field of HTTPQueries is not set properly and all queries come up as non-paginated...Not sure where that error stems from though

This one I'm not sure. I remember seeing someone mentioning an issue with the pagination but I can't find the issue. Maybe we can deal with it in a new issue?

@clockworkgr
Copy link
Collaborator

Seems to generate all modules except for NFT.. Is that expected? do we add nft to keepers by default?

Yes, the NFT module is not scaffolded by default.

ok then

I see an issue where the Paginated field of HTTPQueries is not set properly and all queries come up as non-paginated...Not sure where that error stems from though

This one I'm not sure. I remember seeing someone mentioning an issue with the pagination but I can't find the issue. Maybe we can deal with it in a new issue?

Ok, we can do that. But def needs sorting out. Along with updating the web templates once cosmjs patch is released

@ilgooz ilgooz merged commit ab3f5c1 into main Dec 4, 2023
43 checks passed
@ilgooz ilgooz deleted the fix/restore-api-module-discovery branch December 4, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:ci CI/CD workflow and automated jobs. component:configs component:packages skip-changelog Don't check changelog for new entries type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(regression): ts-client generation is incomplete
5 participants