feat(depinject/appconfig): support gogo proto module configs #20540
Conversation
|
Warning Rate limit exceeded@aaronc has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 49 minutes and 40 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce support for using Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
|
||
| // dynamic resolver allows marshaling gogo proto messages from the gogoproto.HybridResolver as long as those | ||
| // files have been imported before calling LoadJSON. There is similar code in autocli, this should probably | ||
| // eventually be moved into a library. |
There was a problem hiding this comment.
Do we want to refactor this code and the almost identical implementation in autocli to cosmos-proto possibly?
There was a problem hiding this comment.
We could also move it to gogoproto because it uses the hybrid resolver and then call it here: https://github.com/cosmos/gogoproto/blob/6eec9731781bf8ec4feb0e4098ffb19ee74afb50/jsonpb/jsonpb.go#L792. So that gogoproto jsonpb.Unmarshal can be used to unmarshal v2 messages from json that contain some gogo proto messages in Any's.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| import ( | ||
| "fmt" | ||
| "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import
|
This diff got pretty big because of gogoproto getting updated to v1.5.0, but maybe dependabot will open something to take care of that? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
depinject/internal/appconfig/registry.go (1)
20-20: EnsureConfigProtoMessageis properly documented to clarify its usage.Consider adding a comment above
ConfigProtoMessageexplaining its role withinModuleInitializer.depinject/appconfig/dynamic_resolver.go (1)
79-79: Format the file according to Go standards usinggofumpt.Consider running
gofumpt -extrato format the file as per Go standards.Tools
golangci-lint
79-79: File is not
gofumpt-ed with-extra(gofumpt)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (57)
client/v2/go.modis excluded by!**/*.modclient/v2/go.sumis excluded by!**/*.sumdepinject/go.modis excluded by!**/*.moddepinject/go.sumis excluded by!**/*.sumdepinject/internal/appconfiggogo/testpb/test.pb.gois excluded by!**/*.pb.gogo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumorm/go.modis excluded by!**/*.modorm/go.sumis excluded by!**/*.sumruntime/v2/go.modis excluded by!**/*.modruntime/v2/go.sumis excluded by!**/*.sumserver/v2/cometbft/go.modis excluded by!**/*.modserver/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/go.modis excluded by!**/*.modsimapp/go.sumis excluded by!**/*.sumtests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sumx/accounts/defaults/lockup/go.modis excluded by!**/*.modx/accounts/defaults/lockup/go.sumis excluded by!**/*.sumx/accounts/go.modis excluded by!**/*.modx/accounts/go.sumis excluded by!**/*.sumx/auth/go.modis excluded by!**/*.modx/auth/go.sumis excluded by!**/*.sumx/authz/go.modis excluded by!**/*.modx/authz/go.sumis excluded by!**/*.sumx/bank/go.modis excluded by!**/*.modx/bank/go.sumis excluded by!**/*.sumx/circuit/go.modis excluded by!**/*.modx/circuit/go.sumis excluded by!**/*.sumx/consensus/go.modis excluded by!**/*.modx/consensus/go.sumis excluded by!**/*.sumx/distribution/go.modis excluded by!**/*.modx/distribution/go.sumis excluded by!**/*.sumx/epochs/go.modis excluded by!**/*.modx/epochs/go.sumis excluded by!**/*.sumx/evidence/go.modis excluded by!**/*.modx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.modis excluded by!**/*.modx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.modis excluded by!**/*.modx/gov/go.sumis excluded by!**/*.sumx/group/go.modis excluded by!**/*.modx/group/go.sumis excluded by!**/*.sumx/mint/go.modis excluded by!**/*.modx/mint/go.sumis excluded by!**/*.sumx/nft/go.modis excluded by!**/*.modx/nft/go.sumis excluded by!**/*.sumx/params/go.modis excluded by!**/*.modx/params/go.sumis excluded by!**/*.sumx/protocolpool/go.modis excluded by!**/*.modx/protocolpool/go.sumis excluded by!**/*.sumx/slashing/go.modis excluded by!**/*.modx/slashing/go.sumis excluded by!**/*.sumx/staking/go.modis excluded by!**/*.modx/staking/go.sumis excluded by!**/*.sumx/upgrade/go.modis excluded by!**/*.modx/upgrade/go.sumis excluded by!**/*.sum
Files selected for processing (9)
- depinject/appconfig/config.go (4 hunks)
- depinject/appconfig/config_test.go (4 hunks)
- depinject/appconfig/dynamic_resolver.go (1 hunks)
- depinject/appconfig/module.go (2 hunks)
- depinject/internal/appconfig/buf.gen.yaml (1 hunks)
- depinject/internal/appconfig/registry.go (2 hunks)
- depinject/internal/appconfiggogo/buf.gen.yaml (1 hunks)
- depinject/internal/appconfiggogo/buf.yaml (1 hunks)
- depinject/internal/appconfiggogo/testpb/test.proto (1 hunks)
Files skipped from review due to trivial changes (4)
- depinject/internal/appconfig/buf.gen.yaml
- depinject/internal/appconfiggogo/buf.gen.yaml
- depinject/internal/appconfiggogo/buf.yaml
- depinject/internal/appconfiggogo/testpb/test.proto
Additional context used
Path-based instructions (5)
depinject/internal/appconfig/registry.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.depinject/appconfig/dynamic_resolver.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.depinject/appconfig/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.depinject/appconfig/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.depinject/appconfig/config_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
golangci-lint
depinject/appconfig/dynamic_resolver.go
79-79: File is not
gofumpt-ed with-extra(gofumpt)depinject/appconfig/module.go
9-9: use of internal package cosmossdk.io/depinject/internal/appconfig not allowed (typecheck)
depinject/appconfig/config.go
19-19: use of internal package cosmossdk.io/depinject/internal/appconfig not allowed (typecheck)
depinject/appconfig/config_test.go
15-15: use of internal package cosmossdk.io/depinject/internal/appconfig not allowed (typecheck)
Additional comments not posted (1)
depinject/internal/appconfig/registry.go (1)
8-10: Consider consolidating imports to improve readability.- gogoproto "github.com/cosmos/gogoproto/proto" - protov2 "google.golang.org/protobuf/proto" - "google.golang.org/protobuf/reflect/protoreflect" + gogoproto "github.com/cosmos/gogoproto/proto" + protov2 "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protoreflect"Likely invalid or redundant comment.
| if _, ok := res[fullName]; ok { | ||
| return nil, fmt.Errorf("duplicate module registration for %s", fullName) |
There was a problem hiding this comment.
Handle potential duplicate module registration more gracefully.
Consider logging a warning or providing a mechanism to resolve conflicts instead of returning an error immediately.
| // ModulesByModuleTypeName should be used to retrieve modules by their module type name. | ||
| // This is done lazily after module registration to deal with non-deterministic issues | ||
| // that can occur with respect to protobuf descriptor initialization. | ||
| func ModulesByProtoMessageName() (map[protoreflect.FullName]*ModuleInitializer, error) { | ||
| res := map[protoreflect.FullName]*ModuleInitializer{} | ||
| func ModulesByModuleTypeName() (map[string]*ModuleInitializer, error) { | ||
| res := map[string]*ModuleInitializer{} | ||
|
|
||
| for _, initializer := range ModuleRegistry { | ||
| descriptor := initializer.ConfigProtoMessage.ProtoReflect().Descriptor() | ||
| fullName := descriptor.FullName() | ||
| if _, ok := res[fullName]; ok { | ||
| return nil, fmt.Errorf("duplicate module registration for %s", fullName) | ||
| // as of gogoproto v1.5.0 this should work with either gogoproto or golang v2 proto | ||
| fullName := gogoproto.MessageName(initializer.ConfigProtoMessage) | ||
|
|
||
| if desc, err := gogoproto.HybridResolver.FindDescriptorByName(protoreflect.FullName(fullName)); err == nil { | ||
| modDesc := protov2.GetExtension(desc.Options(), appv1alpha1.E_Module).(*appv1alpha1.ModuleDescriptor) | ||
| if modDesc == nil { | ||
| return nil, fmt.Errorf( | ||
| "protobuf type %s registered as a module should have the option %s", | ||
| fullName, | ||
| appv1alpha1.E_Module.TypeDescriptor().FullName()) | ||
| } | ||
|
|
||
| if modDesc.GoImport == "" { | ||
| return nil, fmt.Errorf( | ||
| "protobuf type %s registered as a module should have ModuleDescriptor.go_import specified", | ||
| fullName, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Refactor error handling to reduce duplication and improve clarity.
- if modDesc == nil {
- return nil, fmt.Errorf(
- "protobuf type %s registered as a module should have the option %s",
- fullName,
- appv1alpha1.E_Module.TypeDescriptor().FullName())
- }
- if modDesc.GoImport == "" {
- return nil, fmt.Errorf(
- "protobuf type %s registered as a module should have ModuleDescriptor.go_import specified",
- fullName,
- )
- }
+ if modDesc == nil || modDesc.GoImport == "" {
+ errMsg := "protobuf type %s registered as a module should have the option %s and ModuleDescriptor.go_import specified"
+ return nil, fmt.Errorf(errMsg, fullName, appv1alpha1.E_Module.TypeDescriptor().FullName())
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // ModulesByModuleTypeName should be used to retrieve modules by their module type name. | |
| // This is done lazily after module registration to deal with non-deterministic issues | |
| // that can occur with respect to protobuf descriptor initialization. | |
| func ModulesByProtoMessageName() (map[protoreflect.FullName]*ModuleInitializer, error) { | |
| res := map[protoreflect.FullName]*ModuleInitializer{} | |
| func ModulesByModuleTypeName() (map[string]*ModuleInitializer, error) { | |
| res := map[string]*ModuleInitializer{} | |
| for _, initializer := range ModuleRegistry { | |
| descriptor := initializer.ConfigProtoMessage.ProtoReflect().Descriptor() | |
| fullName := descriptor.FullName() | |
| if _, ok := res[fullName]; ok { | |
| return nil, fmt.Errorf("duplicate module registration for %s", fullName) | |
| // as of gogoproto v1.5.0 this should work with either gogoproto or golang v2 proto | |
| fullName := gogoproto.MessageName(initializer.ConfigProtoMessage) | |
| if desc, err := gogoproto.HybridResolver.FindDescriptorByName(protoreflect.FullName(fullName)); err == nil { | |
| modDesc := protov2.GetExtension(desc.Options(), appv1alpha1.E_Module).(*appv1alpha1.ModuleDescriptor) | |
| if modDesc == nil { | |
| return nil, fmt.Errorf( | |
| "protobuf type %s registered as a module should have the option %s", | |
| fullName, | |
| appv1alpha1.E_Module.TypeDescriptor().FullName()) | |
| } | |
| if modDesc.GoImport == "" { | |
| return nil, fmt.Errorf( | |
| "protobuf type %s registered as a module should have ModuleDescriptor.go_import specified", | |
| fullName, | |
| ) | |
| } | |
| if modDesc == nil || modDesc.GoImport == "" { | |
| errMsg := "protobuf type %s registered as a module should have the option %s and ModuleDescriptor.go_import specified" | |
| return nil, fmt.Errorf(errMsg, fullName, appv1alpha1.E_Module.TypeDescriptor().FullName()) | |
| } |
| "google.golang.org/protobuf/proto" | ||
| "github.com/cosmos/gogoproto/proto" | ||
|
|
||
| internal "cosmossdk.io/depinject/internal/appconfig" |
There was a problem hiding this comment.
Usage of internal package not allowed.
The use of cosmossdk.io/depinject/internal/appconfig may lead to dependency issues or restrict the reusability of the package.
Tools
golangci-lint
9-9: use of internal package cosmossdk.io/depinject/internal/appconfig not allowed (typecheck)
| appv1alpha1 "cosmossdk.io/api/cosmos/app/v1alpha1" | ||
|
|
||
| "cosmossdk.io/depinject" | ||
| internal "cosmossdk.io/depinject/internal/appconfig" |
There was a problem hiding this comment.
Usage of internal package not allowed.
The use of cosmossdk.io/depinject/internal/appconfig may lead to dependency issues or restrict the reusability of the package.
Tools
golangci-lint
19-19: use of internal package cosmossdk.io/depinject/internal/appconfig not allowed (typecheck)
|
|
||
| "cosmossdk.io/depinject" | ||
| "cosmossdk.io/depinject/appconfig" | ||
| internal "cosmossdk.io/depinject/internal/appconfig" |
There was a problem hiding this comment.
Usage of internal package not allowed.
The use of cosmossdk.io/depinject/internal/appconfig may lead to dependency issues or restrict the reusability of the package.
Tools
golangci-lint
15-15: use of internal package cosmossdk.io/depinject/internal/appconfig not allowed (typecheck)
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
|
This PR needs another review and will make users lives easier. |
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
utACK
Can we get a chsngelog?
| // Protobuf message types used for module configuration should define the | ||
| // cosmos.app.v1alpha.module option and must explicitly specify go_package | ||
| // to make debugging easier for users. | ||
| func RegisterModule(msg proto.Message, options ...Option) { |
There was a problem hiding this comment.
Why using any of we do a type check/cast just below. Cannot we just use the gogoproto.Message interface?
There was a problem hiding this comment.
The logic I had was that we may want to support plan old go structs (not protobuf) eventually so one breaking change is better than two. When we discussed in architecture @tac0turtle said gogo is enough, although personally if people are annoyed with too much proto we should lower the barrier
There was a problem hiding this comment.
We okay to leave as is and merge this?
|
Added a CHANGELOG entry |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
depinject/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- depinject/CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (1)
depinject/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
depinject/CHANGELOG.md
[uncategorized] ~31-~31: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE)
Context: ...## 1.0.0-alpha.x Depinject is still in alpha stage even though its API is already qu...
Additional comments not posted (1)
depinject/CHANGELOG.md (1)
27-27: The changelog entry clearly outlines the new feature. Good job maintaining clarity and linking directly to the PR for traceability.
|
|
||
| ## 1.0.0-alpha.x | ||
|
|
||
| Depinject is still in alpha stage even though its API is already quite stable. |
There was a problem hiding this comment.
Consider adding "the" before "alpha stage" to improve grammatical correctness.
- Depinject is still in alpha stage even though its API is already quite stable.
+ Depinject is still in the alpha stage even though its API is already quite stable.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Depinject is still in alpha stage even though its API is already quite stable. | |
| Depinject is still in the alpha stage even though its API is already quite stable. |
Tools
LanguageTool
[uncategorized] ~31-~31: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE)
Context: ...## 1.0.0-alpha.x Depinject is still in alpha stage even though its API is already qu...
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
depinject/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- depinject/go.mod (1 hunks)
Files skipped from review due to trivial changes (1)
- depinject/go.mod
…into aaronc/appconfig-gogo
depinject/go.mod
Outdated
| require ( | ||
| cosmossdk.io/api v0.7.5 | ||
| github.com/cosmos/cosmos-proto v1.0.0-beta.5 | ||
| github.com/cosmos/gogoproto v1.4.11 |
There was a problem hiding this comment.
This PR uses gogoproto v1.5.0 so had to bump it back up
|
I am going to try to use this in a v0.50 chain and see what pops up. |
Description
Closes: #20134
This adds support for using gogo types as module configuration objects so that modules that want to support appconfig do not need to use pulsar at all. Everything else works as it did before.
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
appconfigmodule configuration types usinggithub.zerozr99.workers.dev/cosmos/gogoproto/proto.Bug Fixes
Documentation
CHANGELOG.mdto reflect new support for gogo proto configurations.Chores
github.com/cosmos/gogoproto v1.4.11ingo.mod.