-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(x/auth): define simplified account query service #13210
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #13210 +/- ##
==========================================
- Coverage 55.87% 54.96% -0.91%
==========================================
Files 646 648 +2
Lines 54895 55398 +503
==========================================
- Hits 30675 30452 -223
- Misses 21762 22491 +729
+ Partials 2458 2455 -3
|
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.
I'm confused where the actual gRPC handler is?
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
It needs to be implemented in x/auth |
I know, but the PR is marked R4R. Can you either move it to draft and implement the handler? |
Do we need to implement the handlers in the same PR? My preference is to do protos in one PR and impl in a second PR. But if you prefer we can add here |
Yeah prefer to have query handlers added with their proto definitions |
@aaronc why is this under package |
See the description at the top and the linked issue. Basically I'm proposing that in v1, auth exposes a simpler data model. If that's too much of a leap right now, we can keep in v1beta1 and just add this simple query. |
But where do you define that handler? In the keeper? |
You can just create a struct that wrap the keeper and implements the QueryServer |
Pushed the handler change. |
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.
The query LGTM, though I'm wondering if this is too premature.
I would add this query to v1beta1 for now TBH, and once we do the real auth refactor, we create v1 and think of a good API. For example, the name AccountInfo
could or could not be changed to simply Account
.
Maybe v1beta2? |
Sure, but why not v1beta1 actually? |
That's fine |
There's a mistake in the |
Fixed the proto-gen issue. Thanks @kocubinski ! Also added a test. Marking R4R again. |
…into aaronc/simple-auth-query-proto
…e-auth-query-proto
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, a couple of small nits
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
…e-auth-query-proto
…into aaronc/simple-auth-query-proto
Description
Ref: #13211. This proposes a query service which exposes account details to clients in a more simpler way. It could be implemented very easily with the current x/auth module and could serve as the basis for a refactored v1 x/auth module.
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
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