-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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(accounts): init accounts module #17443
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you follow this too:
Lines 198 to 207 in 7b7be7e
* Add a CHANGELOG.md / README.md under the new package folder | |
* Add the package in [`labeler.yml`](./.github/labeler.yml) | |
* Add the package in [`go.work.example`](./go.work.example) | |
* Add weekly dependabot checks (see [dependabot.yml](./.github/dependabot.yml)) | |
* Add tests to github workflow [test.yml](.github/workflows/test.yml) (under submodules) | |
* Configure SonarCloud | |
* Add `sonar-projects.properties` (see math [sonar-projects.properties](./math/sonar-projects.properties) for example) | |
* Add a GitHub Workflow entry for running the scans (see [test.yml](.github/workflows/test.yml)) | |
* Ask the team to add the project to SonarCloud | |
* (optional) Configure a `cosmossdk.io` vanity url by submitting a PR to [cosmos/vanity](https://github.com/cosmos/vanity). |
[Cosmos SDK - x/accounts] Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! two nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have a failing test, lets get that fixed so this can merge
it's unrelated to the PR |
Yeah it's flaky. Weirdly it's only flaky since today. I've opened an issue for it already. |
syntax="proto3"; | ||
|
||
package cosmos.accounts.v1; | ||
option go_package = "cosmossdk.io/x/accounts/v1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized, the types here should just be generated in the root of the module: https://docs.cosmos.network/v0.50/building-modules/structure#structure, without that /v1
.
Or is this on purpose for handling multiple future versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gov has the same protobuf structure: https://github.com/cosmos/cosmos-sdk/tree/main/proto/cosmos/gov/v1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the proto structure is good. My point is about the go packages.
For gov I think it is under /types and for newer modules directly in the root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah @julienrbrt, now I understood your comment better, but I think we should always prepend package versions to protobuf generated files, because if we do a bump to v2, then would end up either moving the "old" v1 files in a v1 package (breaking change), or create a v2 package with v2 files (not breaking, but inconsistent).
I do not have a strong preference, I can also amend what I did. LMK what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes sense. We should maybe have it under a /types
folder like gov for consistency with other modules.
Description
Closes: #17444
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change