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

Should we expose Keepers in app.go? #881

Closed
faddat opened this issue Jun 2, 2022 · 4 comments · Fixed by #951
Closed

Should we expose Keepers in app.go? #881

faddat opened this issue Jun 2, 2022 · 4 comments · Fixed by #951
Milestone

Comments

@faddat
Copy link
Contributor

faddat commented Jun 2, 2022

Currently, in app.go we do like:

app.bankKeeper

the most common practice is to do like:

app.BankKeeper

Should we change the way that CosmWasm presents these?

@ethanfrey
Copy link
Member

ethanfrey commented Jun 2, 2022

This is a good question.

I have seen both Juno and Osmosis (big users of CosmWasm) do this, and standardising would be nice for compatibility and porting diffs to app.go easier.

I am not sure the reason to make it private? App is only available in the cmd files and the test code. All modules cannot see it, so making these members public doesn't have a security issue wrt importing modules. Not sure if there is something else I am missing.

If there is no good reason to keep them private, I agree with making them public to be more compatible.

@alpe I would love to hear your opinion here.

@faddat
Copy link
Contributor Author

faddat commented Jun 2, 2022

I felt approximately the same way about this, and it seems that it mainly boils down to the keepers.

#882 brings wasmd much closer in line with practices on Juno and Osmosis.

It is my hope that in the next month or so, Juno becomes a reference implementation of wasmd of sorts, and these changes would make moving work between the two projects much easier.

Thanks @ethanfrey @webmaster128 and @alpe -- working on and with CosmWasm has been an unexpected joy.

@faddat faddat changed the title should we expose variables in app.go? should we expose Keepers in app.go? Jun 2, 2022
@faddat
Copy link
Contributor Author

faddat commented Jun 2, 2022

Interestingly, this is throwing a gocritic linter error:

https://app.circleci.com/pipelines/github/CosmWasm/wasmd/2161/workflows/fcbace1f-bdff-40ba-bc71-67b2fbce270d/jobs/7302

To me that complicates the situation somewhat: I often slavishly follow linters to ensure that we have convention:

cosmos/ibc-go#1418

I guess that the right way here might be to remove gocritic, or to disable that, because as I mention here:

cosmos/ibc-go#1418 (comment)

I began work on this over at Osmosis, and would like to eventually end up with a linting standard that we can use throughout the ecosystem.

@alpe
Copy link
Contributor

alpe commented Jun 3, 2022

I was commenting on the PR #882 (comment) but have not had time to look into the code. Let's continue the conversation here.

Interestingly, this is throwing a gocritic linter error:

I guess some unintended refactoring issue. This is fine in the main branch. 😅

@ethanfrey ethanfrey added this to the v0.28.0 milestone Jun 17, 2022
@alpe alpe changed the title should we expose Keepers in app.go? Should we expose Keepers in app.go? Aug 23, 2022
@alpe alpe closed this as completed in #951 Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants