-
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
Start removing HybridCodec (init + auth, bank, distribution) #6838
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6838 +/- ##
==========================================
+ Coverage 55.96% 61.42% +5.46%
==========================================
Files 224 508 +284
Lines 14203 31467 +17264
==========================================
+ Hits 7948 19329 +11381
- Misses 5674 10637 +4963
- Partials 581 1501 +920 |
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. Although for me it seems that it would be much easier if we deleted the Queriers completely
ret0, _ := ret[0].(types0.Querier) | ||
return ret0 | ||
} | ||
|
||
// NewQuerierHandler indicates an expected call of NewQuerierHandler | ||
// LegacyQuerierHandler indicates an expected call of LegacyQuerierHandler |
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.
doc is not consistent with the function name
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.
Looks great! Just added a few comments on parameter naming consistency. I didn't feel like commenting on all the modules, but keeping consistency by using legacyQuerierCdc
makes sense to me.
@@ -10,24 +10,24 @@ import ( | |||
) | |||
|
|||
// NewQuerier creates a querier for auth REST endpoints | |||
func NewQuerier(k AccountKeeper) sdk.Querier { | |||
func NewQuerier(k AccountKeeper, cdc codec.JSONMarshaler) sdk.Querier { |
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.
Should we be consistent with the parameter naming?
func NewQuerier(k AccountKeeper, cdc codec.JSONMarshaler) sdk.Querier { | |
func NewQuerier(k AccountKeeper, legacyQuerierCdc codec.JSONMarshaler) sdk.Querier { |
func (am AppModule) NewQuerierHandler() sdk.Querier { | ||
return keeper.NewQuerier(am.accountKeeper) | ||
// LegacyQuerierHandler returns the auth module sdk.Querier. | ||
func (am AppModule) LegacyQuerierHandler(jsonCdc codec.JSONMarshaler) sdk.Querier { |
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.
func (am AppModule) LegacyQuerierHandler(jsonCdc codec.JSONMarshaler) sdk.Querier { | |
func (am AppModule) LegacyQuerierHandler(legacyQuerierCdc codec.JSONMarshaler) sdk.Querier { |
Ohhh well automerged...would be nice to address these in a followup PR then @aaronc |
We cannot @fedekunze -- we've already proposed and agreed that we're keeping the legacy API around for 1 release to allow clients enough time to migrate to the new gRPC and/or gRPC HTTP gateway. |
…6838) * Start to remove HybridCodec * Rename * Fixes * Test fixes * Cleanup
ref: #6837
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/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes