feat: add autocli service proto #13597

aaronc · 2022-10-19T20:49:30Z

Description

Ref: #13282
Was part of #13281.

This adds two new gRPC query endpoints which the runtime module will implement:

/cosmos.reflection.v1.ReflectionService/FileDescriptors - for the full file descriptor set
/cosmos.autocli.v1.Query/AppOptions - autocli app options for dynamic CLI generation

autocli will use both of these to build dynamic CLIs.

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)

aaronc · 2022-10-19T21:00:30Z

Following up on #13281 (comment), the rationale for putting FileDescriptorSet into the cosmos.app query service is because the FileDescriptorSet itself along with the app config provides almost all of the reflection info needed for dynamic clients.

Some of the earlier reflection services in cosmos.base.reflection were written when less of this metadata was present in the proto files themselves. Now that we are adding more annotations, clients can get almost everything they need from the FileDescriptorSet. For autocli, the main additional pieces needed are provided by this RemoteInfoService.

Happy to consider alternate approaches.

One alternative is to just add an endpoint to list all the file descriptors and then grpc reflection can be used to query each one individually. Currently, grpc reflection doesn't have a way to list all descriptors. The downside of this approach is that it's more work on the client side to build the full FileDescriptorSet. Also, grpc reflection by default is implemented as a blackbox - we probably want to re-implement to make sure we're actually serving the right descriptors (which could be a problem with mixed gogo and proto v2 code and future semver codegen ideas).

codecov · 2022-10-19T21:07:53Z

Codecov Report

Merging #13597 (59ada67) into main (9f3575a) will increase coverage by 0.07%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13597      +/-   ##
==========================================
+ Coverage   54.69%   54.76%   +0.07%     
==========================================
  Files         645      652       +7     
  Lines       55745    56023     +278     
==========================================
+ Hits        30490    30683     +193     
- Misses      22765    22833      +68     
- Partials     2490     2507      +17

Impacted Files	Coverage Δ
x/distribution/simulation/operations.go	`80.64% <0.00%> (-9.68%)`	⬇️
tx/textual/valuerenderer/timestamp.go	`89.28% <0.00%> (ø)`
tx/textual/valuerenderer/valuerenderer.go	`79.59% <0.00%> (ø)`
tx/textual/valuerenderer/int.go	`50.00% <0.00%> (ø)`
tx/textual/valuerenderer/duration.go	`78.22% <0.00%> (ø)`
tx/textual/valuerenderer/coins.go	`73.17% <0.00%> (ø)`
tx/textual/valuerenderer/bytes.go	`62.50% <0.00%> (ø)`
tx/textual/valuerenderer/dec.go	`50.00% <0.00%> (ø)`

tac0turtle · 2022-10-20T08:36:09Z

proto/cosmos/app/v1alpha1/query.proto

+
+  // FileDescriptorSet returns the full FileDescriptorSet of the app in order
+  // to enable easier generation of dynamic clients.
+  rpc FileDescriptorSet(QueryFileDescriptorSetRequest) returns (QueryFileDescriptorSetResponse) {


why not add this to

cosmos-sdk/proto/cosmos/base/reflection/v2alpha1/reflection.proto

Line 7 in 75475a4

option go_package = "github.com/cosmos/cosmos-sdk/server/grpc/reflection/v2alpha1";

? seems like they go together

Or reflection.v1beta1 (or mark it as v1).

I would put cosmos-related stuff here (e.g. chain id), and proto-related stuff (e.g. file descriptors) in cosmos.(base).reflection, even though there's only one method there to get the FileDescriptors

agree here. I would like this approach as it cleans up some random grpc services

So how about moving the file descriptors query to cosmos.reflection.v1 (not sure we need base) as the solo query there for now? And then we move or deprecate the stuff in the other reflection packages piece by piece?

I like this approach

moved to cosmos.reflection.v1. how does that look?

aaronc · 2022-10-20T16:05:04Z

@tac0turtle @AmauryM one additional thing related to this PR in the context the of the presentation I gave on the ORM is that ideally the FileDescriptorSet and app config are committed to state (probably in the runtime module) whenver a chain starts or there is an upgrade. This way, if a light client wants to do proofs - first it does a proof on the app config + FileDescriptorSet and then it can prove anything else that uses the ORM. That maybe explains a bit why they're together here and the long term idea. But maybe it does make sense to put proto stuff in cosmos.reflection because it's more similar to grpc reflection.

amaury1093 · 2022-10-20T17:15:04Z

If we commit the FileDescriptorSet to state, then we're not allowed to modify the proto files between minor releases anymore, correct? Or in general how to deal with FileDescriptorSet mismatch between state and binary-embeded

tac0turtle · 2022-10-20T17:27:20Z

I don't think its a good idea to add it to consensus. we wouldn't be able to do non breaking things to proto files going forward

aaronc · 2022-10-20T17:33:23Z

Good point. What about adding app config to consensus?

Or should we say those things are to be proven out of band?

amaury1093 · 2022-10-20T17:51:09Z

Or should we say those things are to be proven out of band?

Then light clients would need 2 trusted sources: one for the initial header, and another for the filedescriptors.

What we can do is only commit the state.proto file descriptors to state. Then we couldn't do any changes in those files (e.g. not even proto comments updates), but maybe that's fine.

aaronc · 2022-10-20T17:54:17Z

Or should we say those things are to be proven out of band?

Then light clients would need 2 trusted sources: one for the initial header, and another for the filedescriptors.

Yeah, but maybe that's okay?

What we can do is only commit the state.proto file descriptors to state. Then we couldn't do any changes in those files (e.g. not even proto comments updates), but maybe that's fine.

I'm fine to defer this decision till a bit later. Maybe we just do this basic query and deal with proofs when we get to that.

aaronc · 2022-10-21T18:14:30Z

@tac0turtle @AmauryM does this look better? Is cosmos.reflection.v1? Okay?

tac0turtle

LGTM

amaury1093

LGTM, but I would omit module_query_safe

proto/cosmos/autocli/v1/service.proto

amaury1093 · 2022-10-24T12:15:49Z

proto/cosmos/autocli/v1/service.proto

+service RemoteInfoService {
+  // AppOptions returns the autocli options for all of the modules in an app.
+  rpc AppOptions(AppOptionsRequest) returns (AppOptionsResponse) {
+    option (cosmos.query.v1.module_query_safe) = false;


What's the benefit of adding this annotation here? I would omit if false, and add it only when true

Because I don't ever expect it to be true and want to be explicit

OK, maybe we can add a proto comment for this annotation why this is set to false explicitly. By scanning quickly, I initially thought it was true (because not omitted), and then I wondered why this particular query was set to false and not others.

Added some comments. Does this look better @AmauryM ?

Co-authored-by: Amaury <[email protected]>

aaronc · 2022-10-24T16:40:51Z

@AmauryM can you take another look?

amaury1093

LGTM

aaronc · 2022-10-25T17:15:03Z

Tagging api/v0.2.2 with this

aaronc added 3 commits October 19, 2022 13:18

feat: add autocli service proto

6249703

protogen

ffa49e3

protogen

db54c62

aaronc mentioned this pull request Oct 19, 2022

feat: add autocli app wiring + remote info support #13281

Merged

19 tasks

aaronc marked this pull request as ready for review October 19, 2022 20:57

aaronc requested a review from a team as a code owner October 19, 2022 20:57

tac0turtle reviewed Oct 20, 2022

View reviewed changes

tac0turtle assigned amaury1093 and tac0turtle Oct 20, 2022

aaronc added 4 commits October 20, 2022 14:21

move to cosmos.reflection.v1

4648c0c

docs

de513e0

proto gen

ba5365e

proto lint

1d8c533

tac0turtle added C:CLI C: Proto Proto definition and proto release labels Oct 24, 2022

tac0turtle approved these changes Oct 24, 2022

View reviewed changes

Merge branch 'main' into aaronc/autocli-service-proto

f9d9d0c

github-actions bot removed the C:CLI label Oct 24, 2022

amaury1093 reviewed Oct 24, 2022

View reviewed changes

aaronc and others added 2 commits October 24, 2022 12:37

Update proto/cosmos/autocli/v1/service.proto

e10afea

Co-authored-by: Amaury <[email protected]>

address review comments

8f5e3e6

Merge branch 'main' into aaronc/autocli-service-proto

11ba162

aaronc requested a review from amaury1093 October 24, 2022 16:40

amaury1093 approved these changes Oct 24, 2022

View reviewed changes

aaronc enabled auto-merge (squash) October 24, 2022 16:43

Merge branch 'main' into aaronc/autocli-service-proto

59ada67

aaronc merged commit a1b8ec2 into main Oct 25, 2022

aaronc deleted the aaronc/autocli-service-proto branch October 25, 2022 09:17

aaronc mentioned this pull request Oct 26, 2022

refactor!: simplify cosmos.reflection.v1 #13668

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add autocli service proto #13597

feat: add autocli service proto #13597

aaronc commented Oct 19, 2022 •

edited

Loading

aaronc commented Oct 19, 2022 •

edited

Loading

codecov bot commented Oct 19, 2022 •

edited

Loading

tac0turtle Oct 20, 2022

amaury1093 Oct 20, 2022 •

edited

Loading

tac0turtle Oct 20, 2022

aaronc Oct 20, 2022

amaury1093 Oct 20, 2022

aaronc Oct 20, 2022

aaronc commented Oct 20, 2022

amaury1093 commented Oct 20, 2022 •

edited

Loading

tac0turtle commented Oct 20, 2022

aaronc commented Oct 20, 2022

amaury1093 commented Oct 20, 2022 •

edited

Loading

aaronc commented Oct 20, 2022

aaronc commented Oct 21, 2022

tac0turtle left a comment

amaury1093 left a comment

amaury1093 Oct 24, 2022

aaronc Oct 24, 2022

amaury1093 Oct 24, 2022

aaronc Oct 24, 2022

aaronc commented Oct 24, 2022

amaury1093 left a comment

aaronc commented Oct 25, 2022

feat: add autocli service proto #13597

feat: add autocli service proto #13597

Conversation

aaronc commented Oct 19, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

aaronc commented Oct 19, 2022 • edited Loading

codecov bot commented Oct 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

amaury1093 Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Oct 20, 2022

amaury1093 commented Oct 20, 2022 • edited Loading

tac0turtle commented Oct 20, 2022

aaronc commented Oct 20, 2022

amaury1093 commented Oct 20, 2022 • edited Loading

aaronc commented Oct 20, 2022

aaronc commented Oct 21, 2022

tac0turtle left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Oct 24, 2022

amaury1093 left a comment

Choose a reason for hiding this comment

aaronc commented Oct 25, 2022

aaronc commented Oct 19, 2022 •

edited

Loading

aaronc commented Oct 19, 2022 •

edited

Loading

codecov bot commented Oct 19, 2022 •

edited

Loading

amaury1093 Oct 20, 2022 •

edited

Loading

amaury1093 commented Oct 20, 2022 •

edited

Loading

amaury1093 commented Oct 20, 2022 •

edited

Loading