Skip to content
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

grpc service reflection. #13601

Closed
tac0turtle opened this issue Oct 20, 2022 · 4 comments
Closed

grpc service reflection. #13601

tac0turtle opened this issue Oct 20, 2022 · 4 comments
Assignees
Labels
C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. T: Client UX T: Dev UX UX for SDK developers (i.e. how to call our code) T: UX

Comments

@tac0turtle
Copy link
Member

Summary

Currently we have two service.reflection apis.

This is somewhat confusing and could be consolidated to one.

Proposal

merge both reflection apis into one and add file descriptors, mark it as v1.

@tac0turtle tac0turtle added T: UX T: Dev UX UX for SDK developers (i.e. how to call our code) T: Client UX C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. labels Oct 20, 2022
@tac0turtle
Copy link
Member Author

the most relevant thing I would like to keep/add in the api is:

// ConfigurationDescriptor contains metadata information on the sdk.Config
message ConfigurationDescriptor {
  // bech32_account_address_prefix is the account address prefix
  string bech32_account_address_prefix = 1;
  // bech32_validator_address_prefix is the validator address prefix
  string bech32_validator_address_prefix = 2;
  // bech32_consensus_address_prefix is the consensus key address prefix
  string bech32_consensus_address_prefix = 3;
  // minimum_gas_price is the gas set by the node operator
  string minimum_gas_price = 4;
}

not sure which other parts people may find useful so would be good to understand the addition

@tac0turtle tac0turtle self-assigned this Oct 20, 2022
@aaronc
Copy link
Member

aaronc commented Oct 20, 2022

So my gripe with putting that together in a reflection API is that in my mind those all belong to different parts of the system and that separation of concerns leads to better maintainability. We should be clear about what the scope of a reflection service is. In my mind the original intent was just describing the protobuf codec - basically the FileDescriptorSet and information in it.

All those address prefixes have been bunched together into a global, but that's one of the deepest pieces of tech debts to get rid of.

In my mind, bech32 account prefixes belong with x/auth and the validator and consensus prefixes belong with x/staking. x/auth already provides an address codec and a grpc query. I think we should add similar things to staking.

Putting them in reflection if that's mainly proto codecs seems like not the right place.

@tac0turtle
Copy link
Member Author

reflection is the name of the current api, but it doesn't have to be. Bez named it node, but it could be something else.

We can add it to auth as well since it wont be a global anymore. Im open to that, then maybe we drop the current reflection apis.

@tac0turtle
Copy link
Member Author

tac0turtle commented Oct 20, 2022

I can move this to staking and auth configuration endpoints and get rid of the current apis if that is agreed on. Filedescriptor covers most all the end points there

@tac0turtle tac0turtle removed their assignment Aug 30, 2023
@tac0turtle tac0turtle self-assigned this Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. T: Client UX T: Dev UX UX for SDK developers (i.e. how to call our code) T: UX
Projects
Status: 🥳 Done
Development

No branches or pull requests

2 participants