Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Group querier service definition #63

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Mar 24, 2020

This provides the protobuf service definition for supported queries, providng the specification for #42.

The specification for how this is linked up with the querier is still TBD, but needs to be done for Cosmos SDK soon anyway so I will get to that next. Basically the starting point would be to implement the QueryServer interface either on the Keeper directly or with the Keeper as the sole member. And we should think of a relatively simple pattern for retrieving sdk.Context from context.Context (or even just a readonly store as that's all querier's should have access to ideally).

@aaronc aaronc requested a review from alpe March 24, 2020 15:23
Copy link
Collaborator

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As context: with our orm tables and Indexes we can support 3 types of queries easily:

  • By key (id]) -> returns single obj
  • By Prefix ([]byte)-> returns list
  • By Range (start, end []byte) -> returns list

Pagination ideas

that would not require to go through the whole KV store for results. Also a bit less vulnerable to modifications as the fix point for the next page is not relative.

Option A

For the list query pagination we would need some additional query payload

  • Limit uint32 - max number of entries, defaults to 20(?) when empty
  • After bytes - last element id of the previous result set; not included in the new resultset; can be nil for first page; may need to by type agnostic

Result:

  • Data repeated Model(???) - result set
  • HasMore bool
  • Proof Proof(?)

Option B

Make it easier for clients:

  • Limit uint32 - max number of entries, defaults to 20(?) when empty
  • Cursor bytes - same idea as After but not type agnostic. Clients would never build it from the result set; can be nil for first page

Result:

  • Data repeated Model(???) - result set
  • Cursor bytes - can be nil when last page is returned
  • HasMore bool - required as there may only be a first page
  • Proof Proof(?)

I like option B a bit more now as we would drop the object ID type in the requests and let the client simple return what would be the offset for the next page. All logic, encoding is server side only.


message ByAddressRequest {
bytes address = 1 [(gogoproto.casttype) = "github.com/cosmos/cosmos-sdk/types.AccAddress"];
uint64 offset = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen this in sdk code as well. General speaking it is an expensive operation to iterate through the whole KV store to count for an offset.

rpc GetGroupsByOwner(ByAddressRequest) returns (GroupsList);
rpc GetGroupAccountsByGroup(ByGroupRequest) returns (GroupAccountsList);
rpc GetGroupAccountsByOwner(ByAddressRequest) returns (GroupAccountsList);
rpc GetProposal(ProposalRequest) returns (AnyProposal);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some entities may not contain their own ID. While we could return them as they are it would be impossible for a client to do any useful operation with them. For example Vote requires a Proposal ID which is not part of ProposalBase.
In Weave we return the result as a list of Model{Key,Value} entries.

//
// Queries
//
service Query {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to be explicit with all query operations? We are not with commands (Messages). Commands are routed through handlers. We could also have query routes as in the sdk now or #50

}

message GroupsList {
repeated uint64 groups = 1;
Copy link
Collaborator

@alpe alpe Mar 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need data for proofs in the result?

@aaronc
Copy link
Member Author

aaronc commented Mar 26, 2020

So in option B you would create pagination request/response types for each model type? i.e. ProposalsRequest/Response. I would prefer that. Could you maybe just share how that would work as a protobuf code snippet?

And l don't think we need to worry about proofs right now btw

@alpe
Copy link
Collaborator

alpe commented Mar 26, 2020

I have some problems to understand the use of protobuf for queries. The ABCI defines for queries:

Data ([]byte): Raw query bytes. Can be used with or in lieu of Path.
Path (string): Path of request, like an HTTP GET path. Can be used with or in liue of Data

So the protobuf could go into the Data field of queries and Value of responses instead of json. But how does this fit with the RPC service definitions?

I have modified #42 with the cursor implementation. See https://github.com/regen-network/cosmos-modules/pull/50/files#diff-ed6ce47e449d64aeaf419a56cdaca5e5R47-R96 how this would look. Clients would not have to construct the cursors themselfs as they are returned in the response payload.

This are the data types used. It would not make sense to copy them to protobuf directly as some are read from the abci Path field

Request

  • Read from abci.Path
type queryArgs struct {
	path   string
	mod    string
	cursor Cursor
	limit  uint8
}
  • Read from abci.Data
type RangeQueryParam struct {
	start, end []byte
}
As well as binary data for RowIDs or prefixes

### Response
* Serialized into response.Value
```go
type QueryResult struct {
	Data    []Model `json:"data"`
	HasMore bool    `json:"has_more"`
	Cursor  Cursor  `json:"cursor,omitempty"`
}

type Model struct {
	Key   []byte          `json:"key", yaml:"key"`
	Value json.RawMessage `json:"value", yaml:"value"`
}

@aaronc
Copy link
Member Author

aaronc commented Mar 26, 2020

I have some problems to understand the use of protobuf for queries. The ABCI defines for queries:

Data ([]byte): Raw query bytes. Can be used with or in lieu of Path.
Path (string): Path of request, like an HTTP GET path. Can be used with or in liue of Data

So the protobuf could go into the Data field of queries and Value of responses instead of json. But how does this fit with the RPC service definitions?

For an RPC service call there are two things - method name and request payload, those would fit into ABCI Path and Data respectively. Custom queriers in Cosmos SDK (type Querier = func(ctx Context, path []string, req abci.RequestQuery) (res []byte, err Error)) just return []byte or an error. So there's a pretty straightforward way to map protobuf service definition onto this. I intend to spike out a POC of this soon.

@aaronc
Copy link
Member Author

aaronc commented Mar 26, 2020

Also I think queries directly against the table store like you are suggesting are a good idea but my thought is that they are complementary solutions. Would there be a way to generate a swagger schema just from the metadata in the orm?

For me encapsulating custom queries using the service definition to seems like a good approach because it allows existing protobuf definitions to be used. Some Cosmos SDK modules may not be using the ORM package (yet) so I would like a protobuf way for supporting queries on them.

@aaronc
Copy link
Member Author

aaronc commented Mar 26, 2020

@alpe see the changes to querier.go and the AppModule to see how I've wired up the querier to the an implementation of the QueryServer interface that was generated by the protobuf grpc plugin. Basically this uses the grpc infrastructure to route queries via path to rpc handler methods.

@aaronc
Copy link
Member Author

aaronc commented Apr 2, 2020

@alpe see cosmos/cosmos-sdk#5894 for reference on how gRPC queries will be done in Cosmos SDK.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants