feat(orm)!: implement gRPC query support #14965

aaronc · 2023-02-09T00:43:25Z

Description

Ref #11774
This follows up on #13438 and actually implements the query servers as well as making the following changes:

queries are renamed to follow the patterns more commonly encountered in SDK queries and CLI commands, ex. Balance and Balances instead of GetBalance and ListBalance. This should play more nicely with autocli
there is a separate list method per index which matches how query servers are structured now (ex. Groups, and GroupsByMember are separate query methods)
range queries are removed (for now) - I haven't found any instances of range queries in our gRPC query servers or CLI commands, so I consider this advanced usage. We should support it but it can be dealt with later and maybe generically through the API in feat(orm): specify generic gRPC query service #11791
gRPC gateway REST annotations have been added - these follow the convention of using plurals (ex. users/{id}). Indexed are separated using a . which I'm not sure about but seems like the best generic solution so far (ex. balances.by-denom?denom=foo)

The most important files for reviewers to look at are bank_query.proto and test_schema_query.proto

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...

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...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…uery

orm/internal/codegen/codegen.go

@@ -56,7 +61,7 @@
 			continue
 		}

-		out, err := os.OpenFile(fmt.Sprintf("%s_query.proto", f.GeneratedFilenamePrefix), os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0o644)
+		out, err := os.OpenFile(queryProtoFilename(f), os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0o644)


ryanchristo

Looks like a good start. I'm wondering about the following though:

converting addresses stored in state as bytes (as well as any other values that might need to be converted)
return embedded information in response (in regen ledger we often store the primary key of another state object and need to return specific information from the other state object, e.g. a project needs to return the class id stored in a class state object, a sell order needs to return the ask price denom stored in the market state object, etc.)

ryanchristo · 2023-02-14T21:11:22Z

orm/internal/codegen/query_go.go

+	}
+	t.P(")")
+	t.P("if err != nil { return nil, err }")
+	t.P("return &", name, "Response{Value: res}, nil")


Can this be specific? e.g. Balances rather than Values

ryanchristo · 2023-02-14T21:15:32Z

orm/internal/testpb/bank_query.proto

+  // BalancesByDenom queries the Balance table using the primary key index.
+  rpc BalancesByDenom(BalancesByDenomRequest) returns (BalancesByDenomResponse) {
+    option (google.api.http) = {
+      get: "/testpb/balances.denom"


Is this conventional? Can this be more user friendly? e.g. balances-by-denom

Also worth noting the sdk uses _ but as far as I understand - is more conventional.

No it's not conventional. Initially I was thinking an HTTP matrix parameter would be nice (balances;index=denom) but that doesn't play so nicely with grpc gateway.

The thing that bothers me about balances-by-denom is that it isn't actually a domain type but rather an index, but maybe that's okay. I also considered balances/by-denom but that conflicts with the actual balances GET query sort of.

Matrix parameters look interesting but also not yet widely adopted, at least as far as I've seen.

Yea, might be nice to use the actual domain type with an index. A nice thing about balances-by-denom is that the CLI command is balance-by-denom and the name of the query is BalancesByDenom.

We had inconsistent endpoint names in Regen Ledger, which we resolved by adding alternatives, each endpoint reflects the name of the query method and then we added more conventional endpoints. We might have chosen one or the other but wanted to limit breaking changes so we chose to support both in the meantime. For example:

option (google.api.http) = { get : "/regen/ecocredit/v1/projects-by-class/{class_id}" additional_bindings : [ {get : "/regen/ecocredit/v1/projects/class/{class_id}"}, {get : "/regen/ecocredit/v1/classes/{class_id}/projects"} ] };

aaronc · 2023-02-16T15:04:19Z

Thanks for the feedback @ryanchristo

converting addresses stored in state as bytes (as well as any other values that might need to be converted)

So I think this is important even though it's kind of hard.

Would it be sufficient to just include address string parameters in the query parameters and still return the objects as they are stored in state (i.e. as bytes)? That would be much simpler. Otherwise, we need to generate a whole new set of types which include the address strings. Alternatively, we can have some sort of wrapper type (ex. BalanceResult) which includes the stored type (ex. Balance) + the address string. I'm thinking also that it's good to support both address string and bytes in queries and responses in these cases. There may be cases where people want to also query raw bytes.

return embedded information in response (in regen ledger we often store the primary key of another state object and need to return specific information from the other state object, e.g. a project needs to return the class id stored in a class state object, a sell order needs to return the ask price denom stored in the market state object, etc.)

I was originally thinking a graphql API would be best for ORM queries. That would also play more nicely with address string/bytes conversions. The reason I'm going with the grpc approach is because that's whats' supported everywhere else in the SDK and seems like it's needed at a minimum. I think it'd be hard to traverse relationships with grpc, however. So would it be fine to consider the graphql layer an addon to be developed later? Or would it maybe be preferable to skip grpc and just do graphql? Personally, I'd rather use graphql for queries from a usability perspetive. but that would be at odds with the way everything else is done currently.

ryanchristo · 2023-02-16T19:26:35Z

Would it be sufficient to just include address string parameters in the query parameters and still return the objects as they are stored in state (i.e. as bytes)? That would be much simpler. Otherwise, we need to generate a whole new set of types which include the address strings. Alternatively, we can have some sort of wrapper type (ex. BalanceResult) which includes the stored type (ex. Balance) + the address string. I'm thinking also that it's good to support both address string and bytes in queries and responses in these cases. There may be cases where people want to also query raw bytes.

I'm not in favor of returning bytes alone. As a webapp developer, I would rather not convert account bytes to an address in my web application. As a blockchain application developer, I would rather not convert when trying to debug. Maybe in some cases it would be good to support both but I haven't come across the need for bytes personally.

ryanchristo · 2023-02-16T19:34:16Z

I was originally thinking a graphql API would be best for ORM queries. That would also play more nicely with address string/bytes conversions. The reason I'm going with the grpc approach is because that's whats' supported everywhere else in the SDK and seems like it's needed at a minimum. I think it'd be hard to traverse relationships with grpc, however. So would it be fine to consider the graphql layer an addon to be developed later?

I think it would be ok to consider as an add-on to be developed later. We probably wouldn't use this feature in Regen Ledger until we have graphql support but new developers could make use of the feature.

Or would it maybe be preferable to skip grpc and just do graphql? Personally, I'd rather use graphql for queries from a usability perspetive. but that would be at odds with the way everything else is done currently.

Yea, I'm not sure about this and this might be something worth exploring and discussing with other developers. Seems like a pretty big pivot. I agree though that from a usability perspective, graphql would be preferred.

aaronc · 2023-02-23T17:46:04Z

So I'm thinking that what I will do based on the feedback is:

continue with this gRPC support even though graphql would be better because not having gRPC support doesn't seem quite right (it would give us out of the box autocli support + ADR 033 inter module queries
handle address bytes <-> string conversion on both request and response types, generating wrapper types when needed
encode secondary indexes as balances-by-denom as opposed to balances.denom

How does that sound? Any other opinions?

ryanchristo · 2023-02-23T17:49:27Z

That all sounds good to me.

…uery

github-actions · 2023-03-30T00:05:00Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2023-05-04T00:04:34Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

aaronc added 9 commits February 4, 2023 16:13

feat(orm): implement gRPC query support

3cb60d0

WIP

e3860be

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/orm-q…

cbd55c2

…uery

remove reflection stuff

601dae4

WIP

836897a

WIP

3ab936e

adding REST URLs

a9d1818

WIP on grpc gateway

aad2e79

WIP on grpc gateway

73f00ed

github-actions bot added the C:orm label Feb 9, 2023

aaronc added 2 commits February 8, 2023 19:44

cleanup

c698aad

cleanup

60ced5b

github-advanced-security bot found potential problems Feb 9, 2023

View reviewed changes

ryanchristo reviewed Feb 14, 2023

View reviewed changes

aaronc added 4 commits February 27, 2023 12:47

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/orm-q…

02dade9

…uery

update REST URL

52ca7e6

update REST URL

2898215

WIP on address bytes <-> string

42f5435

github-actions bot added the stale label Mar 30, 2023

tac0turtle removed the stale label Apr 3, 2023

aaronc mentioned this pull request Apr 13, 2023

[Feature]: Provable GraphQL Query Engine #15834

Open

github-actions bot added the Stale label May 4, 2023

aaronc closed this May 4, 2023

tac0turtle deleted the aaronc/orm-query branch February 21, 2024 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(orm)!: implement gRPC query support #14965

feat(orm)!: implement gRPC query support #14965

aaronc commented Feb 9, 2023 •

edited

Loading

ryanchristo left a comment •

edited

Loading

ryanchristo Feb 14, 2023

aaronc Feb 16, 2023

ryanchristo Feb 14, 2023

ryanchristo Feb 14, 2023

aaronc Feb 16, 2023

ryanchristo Feb 16, 2023

aaronc commented Feb 16, 2023

ryanchristo commented Feb 16, 2023

ryanchristo commented Feb 16, 2023

aaronc commented Feb 23, 2023

ryanchristo commented Feb 23, 2023

github-actions bot commented Mar 30, 2023

github-actions bot commented May 4, 2023

feat(orm)!: implement gRPC query support #14965

feat(orm)!: implement gRPC query support #14965

Conversation

aaronc commented Feb 9, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

ryanchristo left a comment • edited Loading

Choose a reason for hiding this comment

ryanchristo Feb 14, 2023

Choose a reason for hiding this comment

aaronc Feb 16, 2023

Choose a reason for hiding this comment

ryanchristo Feb 14, 2023

Choose a reason for hiding this comment

ryanchristo Feb 14, 2023

Choose a reason for hiding this comment

aaronc Feb 16, 2023

Choose a reason for hiding this comment

ryanchristo Feb 16, 2023

Choose a reason for hiding this comment

aaronc commented Feb 16, 2023

ryanchristo commented Feb 16, 2023

ryanchristo commented Feb 16, 2023

aaronc commented Feb 23, 2023

ryanchristo commented Feb 23, 2023

github-actions bot commented Mar 30, 2023

github-actions bot commented May 4, 2023

aaronc commented Feb 9, 2023 •

edited

Loading

ryanchristo left a comment •

edited

Loading