refactor: use unexported types for core query servers#6794
refactor: use unexported types for core query servers#6794damiannolan merged 16 commits intomainfrom
Conversation
* chore: pull out get light client module to helper methods * another place to use getLightClientModuleRoute * use getLightClientModule in other 02-client functions * lint --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
# Conflicts: # modules/core/03-connection/keeper/verify.go # modules/light-clients/09-localhost/client_state.go
damiannolan
requested review from
AdityaSripal,
DimitrisJim,
bznein,
chatton,
crodriguezvega and
gjermundgaraba
as code owners
| ChainID: chainID, | ||
| App: app, | ||
| ProposedHeader: header, | ||
| QueryServer: app.GetIBCKeeper(), |
There was a problem hiding this comment.
Is there any reason why you didn't want to keep this with keeper.NewQueryServer(app.GetIBCKeeper().ChannelKeeper)?
There was a problem hiding this comment.
This query server is the interface that was previously defined in core/types which I removed in this PR.
It was the union of all core query servers.
type QueryServer interface {
clienttypes.QueryServer
connectiontypes.QueryServer
channeltypes.QueryServer
}I'm not sure this provides a tonne of value and each submodule query server can be created on demand since the sub keepers are public on the ibc core keeper.
If folks feel strongly about keeping this type for testing purposes then I would suggest moving this to the testing pkg, which would still require creating a struct to passthrough to each submodule query server. Similar to what was removed in core/keeper/grpc_query.go in this PR.
There was a problem hiding this comment.
Sounds good to me. I don't feel strongly about it tbh, was mostly curious about the thinking, but it makes sense! 👍
There was a problem hiding this comment.
IIRC the query server was added to the testing chain struct for testing grpcs. Given that the individual query server is created as necessary in the grpc tests, I agree it makes sense to remove. It also didn't contain all the queries available on the chain (non core ibc queries) so it might have led to confusion
gjermundgaraba
left a comment
gjermundgaraba
left a comment
There was a problem hiding this comment.
Great to get this done! LGTM! 🚀
DimitrisJim
left a comment
DimitrisJim
left a comment
There was a problem hiding this comment.
lgtm, I guess we should also add a note to changelog for this?
should we open follow up issues for apps?
| res, err := suite.chainA.QueryServer.ClientState(ctx, req) | ||
|
|
||
| queryServer := keeper.NewQueryServer(suite.chainA.GetSimApp().IBCKeeper.ClientKeeper) | ||
| res, err := queryServer.ClientState(ctx, req) |
There was a problem hiding this comment.
these could also be defined on a ClientQuerierTestSuite to further separate. Probably could also init the querier in the setup to avoid needing to call new in each test. Nice for follow up if people agree.
There was a problem hiding this comment.
Yeah, that's a nice idea 👍🏻
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
LGTM! Nice bit of cleanup
|
6 participants
Description
Refactors the query servers in core to use unexported/private
queryServerstructs.Removes the core/types
QueryServertop level interface.closes: #6775
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.