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

Client Redesign #6221

Closed
4 tasks
alexanderbez opened this issue May 14, 2020 · 9 comments
Closed
4 tasks

Client Redesign #6221

alexanderbez opened this issue May 14, 2020 · 9 comments

Comments

@alexanderbez
Copy link
Contributor

The state of the client-side functionality of the SDK is fragmented at best. We primarily expose a CLI interface for generating unsigned txs, signing txs and signing related functionality, and finally querying the state of modules and the application.

The REST service (if enabled as a separate service) performs nearly the identical aspects apart from signing related functionality (for security purposes).

A challenge that arises from this is the usage and introduction of often duplicate code and a fragmented testing suite.

I propose that the binary is started with an (optional) HTTP API (i.e. the REST service we have now). The HTTP API would be used as it is now primarily with the core distinction that now the CLI interface simply wraps the HTTP API. All CLI command handlers will essentially delegate everything to the internal HTTP API.

The logic within CLI handlers can drastically be reduced to just simple arg fetching and API delegation.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@aaronc
Copy link
Member

aaronc commented May 14, 2020

Is this still relevant with gRPC?

In ADR 021 we said we were going to have:

  • gRPC routed through the ABCI query endpoint
  • native gRPC (proxied to ABCI)
  • grpc-gateway for REST

All of these would live in the primary gaiad, etc. command on different ports and optionally enabled/disabled.

My intention in #5953 is to have the query CLI methods simply use the generated gRPC client types. We haven't gotten around to reviewing that yet, but so far I think it simplifies things quite a bit. Is this similar to what you had in mind?

@alexanderbez
Copy link
Contributor Author

alexanderbez commented May 14, 2020

Yes, this is still absolutely relevant. The gist of this proposal is to have nearly all CLI handlers (with the exception of signing) delegate to the HTTP API.

@aaronc
Copy link
Member

aaronc commented May 14, 2020

But why is that better than CLI handlers delegating to gRPC? Are you talking about tx or queries?

@alexanderbez
Copy link
Contributor Author

I never said that. The proposal is that they delegate instead of duplicating work. Be that an HTTP API or gRPC -- it's the same idea or principle.

@aaronc
Copy link
Member

aaronc commented May 14, 2020

Okay, totally makes sense then.

Does this accomplish what you have in mind: https://github.com/cosmos/cosmos-sdk/pull/5953/files#diff-17e5aa248cbcd2ce8e10c9525a1b88b4R39 ?

Basically the CLI method calls this gRPC interface rather than sprintf'ing the route and doing a bunch of marshaling:

type QueryServiceClient interface {
 	// QueryBalance queries the balance of a single coin for a single account
 	QueryBalance(ctx context.Context, in *QueryBalanceRequest, opts ...grpc.CallOption) (*QueryBalanceResponse, error)
 	// QueryBalance queries the balance of all coins for a single account
 	QueryAllBalances(ctx context.Context, in *QueryAllBalancesRequest, opts ...grpc.CallOption) (*QueryAllBalancesResponse, error)
 }

@aaronc
Copy link
Member

aaronc commented May 14, 2020

An even crazier alternative would be to actually generate CLI methods from gRPC reflection (like https://github.com/fullstorydev/grpcurl), but maybe it's better to hand tweak the args and flags for now.

@alexanderbez
Copy link
Contributor Author

Yes precisely! I'd even wager that we should even potentially remove validation and parsing -- just take args and pass them to the delegated service to further reduce boilerplate code. But yes, this is essentially the idea. In that case, we can maybe even close this issue.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2020

This issue 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 github-actions bot added the stale label Jul 4, 2020
@tac0turtle tac0turtle removed the stale label Jul 4, 2020
@tac0turtle
Copy link
Member

this is being worked on as part of lens, no need for this issue anymore.

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

No branches or pull requests

3 participants