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

feat(codegen): fix module discovery logic #2146

Conversation

gjermundgaraba
Copy link
Contributor

Issue: #835

Background

The code generation for vuex was not able to find all modules due to the nature of the algorithm used to discover modules. Previously it checked for an implementation of the msg interface which would signal that the code was indeed a cosmos sdk-based module.

During the development I found no simple way of determining for a non-msg module (query only) such as cosmos/tx/v1beta1 if it was a proper sdk-based module or just any other proto files. So the approach has changed a little, but that also means it covers another issue which was mentioned in the code about generating code for non-registered modules (those are now skipped)

What has been done

  • Added a function in pkg/cosmosanlysis/app to find all registered modules (see comments for details on the algorithm)
  • Changed the aforementioned algorithm in pkg/cosmosanlysis/module to use the registered modules as both a filter and a way to look up the implemented code to check if the proto files are implemented there
  • Written both unit and integration tests to cover the changes + expand on the module test with more useful test data

Bounty address in commit message.

Note: I would like some extra scrutiny on my ast code (pkg/cosmosanlysis/app/app.go) since it is the first time I am doing ast parsing in go. I found it a bit more verbose and "indenty" than I would've liked. Perhaps someone with more experience can give me pointers to make it better!

Lastly, thanks for the bounty opportunity. I hope this helps and I am happy to jump on more the future :)

ilgooz and others added 30 commits December 17, 2021 16:33
chore: sync master
Co-authored-by: Danilo Pantani <danpantani@gmail.com>
Grammar fix at line 7.

Co-authored-by: Danilo Pantani <danpantani@gmail.com>
Grammar fix at line 55.

Co-authored-by: Danilo Pantani <danpantani@gmail.com>
* add no simulation flag

* add no simulation flag to scaffold message command

* move the simulation modifier function to inside the no simulation condition

Co-authored-by: Lucas Bertrand <lucas.bertrand.22@gmail.com>
* feat(docs): Remove the link to non existing prerequisite

* Update docs/guide/install.md

Co-authored-by: Barrie Byron <barrie.byron@tendermint.com>
* init

* Implement apply request

* fix prepare

* initialize apply test

* Apply remove tests

* refactor request

* command verify

* fix chain home

* Add comment

* simulate and refactor

* Lint

* refacotr

* refactor error

* changes in log

* lint

* Update starport/cmd/network_request_verify.go

Co-authored-by: Danilo Pantani <danpantani@gmail.com>

* Update starport/services/network/request.go

Co-authored-by: Danilo Pantani <danpantani@gmail.com>

* Danilo suggest

* lint

* Ilker comment

* comment

* ilker comment

Co-authored-by: Danilo Pantani <danpantani@gmail.com>
* set gas price from gentx

* merge conflicts

* use zero stake coin

* use gas price and self delegation as a flag

* use errors.wrap instead errors.new
…nite#2004)

* edits for consistency and migration doc for style

* Update docs/migration/index.md

Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>

* typo and add testutil/sample change to cosmostestutil/sample

* add import paths

Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
…e#2013)

* simple edits for heading style to use sentence case

* remove tick mark for headings and fix typo and add punctuation throughout

* update to 0.19.2 in examples and change case

* add preventing regen and change case

* minor genesis edits and change case of headings

* sentence case and minor edits for all KB articles
* Update Hello World tutorial to v0.19.2

* Update Blog to v0.19.2

* Update connect blockchain to v0.19.2

* Update IBC relayer tutorial to v0.19.2
…gnite#2019)

* Hello and index change case

* simple case change, typo fix, some minor edits
… ranges (ignite#2031)

* remove sort side effect in numbers.ParseList

* Apply suggestions from code review

Co-authored-by: Petr Ivanov <petr.ivanov@tendermint.com>
Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
@ilgooz ilgooz linked an issue Mar 10, 2022 that may be closed by this pull request
Copy link
Member

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

Hey, I made a quick review. Looks great! 🎉

@ilgooz ilgooz changed the title feat(pkg/cosmosanlysis): include all registered modules in vuex codegen feat(codegen): fix module discovery logic Mar 10, 2022
ilgooz and others added 3 commits March 10, 2022 21:14
…`publish` command (ignite#2122)

* add mainnet and total-supply flags to the pusblish command

* create campaign publish command

* update stmt

* update spn version

* Update starport/cmd/network_chain_publish.go

* Update starport/services/network/campaign.go

* update spn version

Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
@gjermundgaraba
Copy link
Contributor Author

@ilgooz Thanks for the review! I've updated the PR with the changes requested. I also added some tests for other methods in goanalysis since I was already adding some tests there. Hope that is OK :)

ilgooz and others added 4 commits March 11, 2022 08:08
…gnite#2089)

* creates `network campaign account list [campaign-id]` command

* run make format

* use goroutine to fetch the campaign accounts

* use Coin and Share type instead string

* add the validator account flag back

* add event options

* print a msg if the summary is empty

* remove unused flags

* put back the keyring flags

* add the keyring flags to the right cmd method

* update spn version

* fix events unit tests
* add `n chain reward-set` command

* fix comments for reward-set cmd

* improve the log messages

* fix block indentation

* add reward flags to the publish command

* run make format

* fix the spn upgrade version

* improve the log message

* add validator account flag

* improve the reward log with the new message response

* improve the events messages and logs

* improve the event message options adding the icon

* fix method names and text color

* fix events struct comment

* fix lint

* move chain reward to reward

* improve the error return message

* update spn version

* fix events unit tests

* fix duplicated log message

* fix reward height flag

* update spn version

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
@gjermundgaraba
Copy link
Contributor Author

@ilgooz I've updated the PR with the requested changes :)

@gjermundgaraba gjermundgaraba requested a review from ilgooz March 14, 2022 14:34
Copy link
Member

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, added one more comment.

var basicModules []string
for _, pkg := range pkgs {
for p, f := range pkg.Files {
if !strings.HasSuffix(p, appFileName) {
Copy link
Member

Choose a reason for hiding this comment

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

If there is another file with the same suffix but doesn't comply the checks this will break the build process. I think we should specifically look for the app file and only operate on this file.

Even we can implement something like cosmosanalysis.FindAppFilePath() (path string, err error), to find app path through code analysis.

In the implementation of this func, we can look for if there is file that has a type that implements following funcs:

// The assigned name of the app.
Name() string

// Application updates every begin block.
BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock

// Application updates every end block.
EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock

Normally only the app file should have a type that implements these funcs. But in case there are more files for ex. there might be a type in tests that implements these, then we can look for the file name as the last check to see if it's app.go and use that one.

This way, even if the app file's name is not app.go, we still will be able to discover that file. And in case there are more files implements the funcs above, we basically fallback and find app.go by file name but still check if implements the funcs or not. If we cannot find the implementation, FindAppFilePath can fail with an error to report to user. This flow gives us a nice flexibility and also verbosity in case app file cannot be found.

Func should return the absolute path to the app file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable! I will do that.
Quick question on the FindAppFilePath, should it take in the path to look for app.go in, or will it do a deep find or assume ./app/* when it searches?

Copy link
Member

@ilgooz ilgooz Mar 15, 2022

Choose a reason for hiding this comment

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

Probably we can even do a '**/*' from the root path of the chain, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that will work. I ended up sending in the chain root since we already have it and just going from "." doesn't work out when scaffolding the chain (and probably not when using --path either). But it seems to be working fine.

Update coming soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilgooz I updated the PR now with some changes to account for this. I also added some more tests cosmosanalysis since I was doing some refactoring to be able to reuse the logic already there. Let me know if it makes sense or you want any additional changes to it.

Copy link
Member

Choose a reason for hiding this comment

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

That's great!

Pantani and others added 6 commits March 15, 2022 13:37
…gnite#2144)

* rever launch

* rename revert launch cmd file

* create a method to reset the genesis time

* improve the cmd logic

Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
…me (ignite#2154)

Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
Copy link
Member

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

LGTM! This is a great implementation! Thank you for addressing the change requests and adding extra tests! 🎊 ❤️

@ilgooz ilgooz changed the base branch from develop to release/v0.19.5 March 16, 2022 06:37
@ilgooz ilgooz merged commit 96e8c68 into ignite:release/v0.19.5 Mar 16, 2022
ilgooz added a commit that referenced this pull request Mar 16, 2022
@ilgooz
Copy link
Member

ilgooz commented Mar 16, 2022

We were going to make a hot release and I misconfigured the merge process for this PR. To fix my mistake, I created another PR for your changes and directly merged to develop. 👍 #2159

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable code generation for query only modules too