feat: gRPC server #8

Reecepbcups · 2024-08-16T01:49:47Z

Summary

A gRPC server implementation for Gordian. This way we can add interchaintest support. Then wrap gRPC in the HTTP server instead for Comet compatibility.

Keep scoped to gcosmos only

Future

TxHash -> tx retrieval from the store
support extending the gRPC interface as other modules are added / removed, specifically allowing rust crypto libraries (similar to the Signer interface)
Migrate http handler -> a gRPC client wrapper for request (feat: wrap gRPC client within http handler #14)

This reverts commit 07caec6.

Reecepbcups · 2024-08-19T19:59:06Z

gcosmos/scripts/protocgen.sh

+#!/bin/sh
+# sh ./scripts/protocgen.sh
+#
+# TODO: migrate to buf?


Given the simplicity atm with proto I feel protoc is fine until it is not?

I have attempted the buf work & the cosmos/gogoproto is not happy with duplicate definitions on v1.7.0. I think this should be scoped to its own PR in coordination with #15.

Ref buf attempt here: 4d4c9be

That's really unfortunate.

update: I finally got it to register properly (required cosmos proto with their buf container) but then it still breaks gRPC url despite list showing that service method. 😞

Standard Go clients work but not working with grpcurl seems really wrong. I think future scope

https://github.com/rollchains/gordian/tree/reece/grpc-buf-attempt

gcosmos/gserver/internal/ggrpc/server.go

Reecepbcups · 2024-08-19T20:53:35Z

gcosmos/gserver/internal/txmanager/txmanager.go

@@ -1,4 +1,4 @@
-package gsi
+package txmanager


This was needed for the gRPC server as well. So sharing within the txmanager package instead of gsi (http only)

cc @mark-rushakoff for rationale on moving the txmanager to its own package

my reply from slack for ctx:

GRPC server needs access to the SDKTxBuf for the server_debug.go commands (SubmitTransaction, PendingTransactions). Importing gsi from within ggrpc would result in an import cycle not allowed (since HTTPServerConfig will depend on ggrpc.GordianGRPCClient)

This was a proactive change in prep for the next iteration. Happy to undo for the context of this PR, but is required in the http gRPC wrapped PR

Due to the volume of other comments -- and the general size of the PR -- don't include this change in this PR. Removing it will reduce the number of touched files by four, I think.

reverted txmanager -> gsi here: a01a380

Reecepbcups · 2024-08-19T20:53:53Z

gcosmos/proto/server/grpc.proto

+message TxResultResponse {
+    // TODO: tx hash?
+    repeated Event events = 1 [json_name="events"];
+    // bytes resp = 2; //  []transaction.Msg


This is an interface on the SDK side

mark-rushakoff

I have a lot of comments.

The narrower we can get these PRs, the faster we can get individual pieces through.

mark-rushakoff · 2024-08-20T14:35:38Z

gcosmos/.gitignore

@@ -3,3 +3,5 @@

 # go build . will make a binary named gcosmos, which we never want to commit.
 gcosmos
+


Include a comment noting what this is and why it's ignored.

added context b369490

mark-rushakoff · 2024-08-20T14:40:07Z

gcosmos/README.md

+
+# Interact
+```bash
+# go install github.com/fullstorydev/grpcurl/cmd/grpcurl@latest


I would suggest GOBIN="$PWD" go install github.com/fullstorydev/grpcurl/cmd/grpcurl@latest so as to not pollute anything else on the user's system. Maybe @v1 would even be better so we don't get unexpected breakage upon a v2 release.

Then we can just git-ignore grpcurl and change the rest of the commands in this list to ./grpcurl instead of whatever is on $PATH.

good call:
handled and added to gitignore with comment: b369490#diff-62120d6b326ed1a40891671df851b595805cc273ecb5ba7cf3c34ccd194d0465R60

mark-rushakoff · 2024-08-20T14:44:13Z

gcosmos/README.md

+grpcurl -plaintext -emit-defaults -d '{"tx":"'$(cat example-tx-signed.json | base64 -w 0)'"}' localhost:9092 server.GordianGRPC/SimulateTransaction
+
+grpcurl -plaintext -emit-defaults -d '{"tx":"'$(cat example-tx-signed.json | base64 -w 0)'"}' localhost:9092 server.GordianGRPC/SubmitTransaction


base64 -w does not work by default on macOS. Is there another, portable way to express this?

Yes, migrated to use tr -d '\n' instead. Tested on my Mac and works as expected

22fed16

gcosmos/gserver/internal/ggrpc/server.go

mark-rushakoff · 2024-08-20T14:50:38Z

gcosmos/gserver/component.go

@@ -415,6 +464,7 @@ func (c *Component) StartCmdFlags() *pflag.FlagSet {
 	flags := pflag.NewFlagSet("gserver", pflag.ExitOnError)

 	flags.String(httpAddrFlag, "", "TCP address of Gordian's introspective HTTP server; if blank, server will not be started")
+	flags.String(grpcAddrFlag, "", "GRPC address of Gordian's introspective GRPC server; if blank, server will not be started")


This would be another TCP address (of the gRPC server), not a gRPC address.

TCP address is a little more clear that it is of form HOST:PORT, as opposed to SCHEME://HOST:PORT or something else.

resolved: b369490#diff-8faa55d6aabd35431c272a2a2a8785f992c9e3391dac40dc91b2be90cc27848dR466

mark-rushakoff · 2024-08-20T15:38:29Z

gcosmos/gserver/internal/ggrpc/server_debug.go

+	am := g.cfg.AppManager
+
+	if req.Address == "" {
+		return nil, fmt.Errorf("BUG: address field is required")


If this is a programmer bug (e.g. they didn't fully configure the gRPC server) then a panic is appropriate. If this could be blank due to user input then the BUG prefix is probably not appropriate.

User facing issue, removed BUG ce439ab

mark-rushakoff · 2024-08-20T15:39:40Z

gcosmos/gserver/internal/txmanager/txmanager.go

@@ -1,4 +1,4 @@
-package gsi
+package txmanager


Due to the volume of other comments -- and the general size of the PR -- don't include this change in this PR. Removing it will reduce the number of touched files by four, I think.

mark-rushakoff · 2024-08-20T15:40:53Z

gcosmos/scripts/protocgen.sh

+#!/bin/sh
+# sh ./scripts/protocgen.sh
+#
+# TODO: migrate to buf?


That's really unfortunate.

mark-rushakoff · 2024-08-20T15:41:43Z

gcosmos/scripts/protocgen.sh

+cp -r ./github.com/rollchains/gordian/gcosmos/* .
+rm -rf ./github.com


What is this supposed to be doing?

protoc generates the code in this relative directory format

So we copy the changed files -> their path. Then remove the relative temp generated directory.

That's far from obvious, reading this script in isolation. It should be commented.

mark-rushakoff · 2024-08-20T15:43:29Z

gcosmos/README.md

-go run . genesis add-genesis-account val 1000000stake --keyring-backend=test
-go run . genesis gentx val 1000000stake --keyring-backend=test --chain-id=gcosmos
-go run . genesis collect-gentxs
+# cosmos1r5v5srda7xfth3hn2s26txvrcrntldjumt8mhl


This comment is missing some words.

b369490#diff-62120d6b326ed1a40891671df851b595805cc273ecb5ba7cf3c34ccd194d0465R46

mark-rushakoff

I left a few more comments, but all of those we can address separately. I'm going to merge this.

mark-rushakoff · 2024-08-21T12:52:13Z

gcosmos/README.md

+./gcosmos genesis gentx val 1000000stake --keyring-backend=test --chain-id=gcosmos
+./gcosmos genesis collect-gentxs
+
+# rm -rf ~/.simappv2/data/application.db/


Help the reader out -- why is this commented code here? Would they want to run it? If so, when?

Same on the next section at L60.

mark-rushakoff · 2024-08-21T13:03:56Z

gcosmos/gserver/internal/ggrpc/server.go

+
+	grpcServer *grpc.Server
+
+	cfg GRPCServerConfig


I think there is only one other place in Gordian where we retain a reference to a config object (and that was a hack that needs to be cleaned up).

Sometimes it's because the config object contains some values that the parent only needs to pass through to sub-components, or the parent only composes some of the values into a different inner type.

It's a little more verbose to copy the config fields into the target type, but again as a reader, I don't want to have to make two jumps to figure out what fields my type has.

I think there is also some subtlety in having several plain unexported fields as opposed to a single unexported field that itself has many exported fields. Furthermore, while we use fully written names for exported fields like in the config object, we conventionally go towards standard shorthand notation for unexported fields, like ln instead of Listener, fs instead of FinalizationStore, etc.

I think we can clean that up in a separate PR anyway.

mark-rushakoff · 2024-08-21T13:06:08Z

gcosmos/gserver/internal/ggrpc/server.go

+	var opts []grpc.ServerOption
+	g.grpcServer = grpc.NewServer(opts...)


The previous // TODO comment here was fine. Otherwise, why declare the opts slice at all?

mark-rushakoff · 2024-08-21T13:18:03Z

gcosmos/gserver/internal/ggrpc/server.go

+	RegisterGordianGRPCServer(g.grpcServer, g)
+	reflection.Register(g.grpcServer)
+
+	if err := g.grpcServer.Serve(g.cfg.Listener); err != nil {


There is probably an expected error or two that we can swallow here, but we'll adjust that later.

mark-rushakoff · 2024-08-21T13:23:05Z

gcosmos/gserver/internal/ggrpc/server_debug.go

+func convertEvent(e []event.Event) []*Event {
+	events := make([]*Event, len(e))
+	for i, ev := range e {
+


Avoid leading blank lines in new indentation.

mark-rushakoff · 2024-08-21T13:27:26Z

gcosmos/scripts/protocgen.sh

+cp -r ./github.com/rollchains/gordian/gcosmos/* .
+rm -rf ./github.com


That's far from obvious, reading this script in isolation. It should be commented.

mark-rushakoff · 2024-08-21T13:33:45Z

gcosmos/proto/server/grpc.proto

+}
+
+message Validator {
+    bytes pub_key = 1;


The public key field is subtle because, it looks like it would contain raw public key bytes. But it doesn't because that would not contain the type of the key.

So, maybe renaming this to encoded_pub_key would make things a little more clear?

Or we could adjust the gcrypto public key interface to make it easier to get the Gordian name for the key type, and encode the key type field separately from the raw key bytes.

I think the single byte slice containing both the type and the key bytes is generally a good fit when staying internal to Gordian, but a separated type and raw key bytes is probably easier to consume from the outside, as there is no special parsing -- the gcrypto registry just prefixes the raw bytes with a fixed 8-byte header.

mark-rushakoff · 2024-08-21T13:35:09Z

gcosmos/proto/server/grpc.proto

+
+option go_package = "github.com/rollchains/gordian/gcosmos/gserver/internal/ggrpc";
+
+package server;


I'm not too sure about the location of the file (the bare proto directory), and I feel like there is a more specific name than server that we can use here. But we can adjust both of those later.

Reecepbcups added 2 commits August 15, 2024 20:14

go gRPC + connection

e73d7a1

gRPC: rust client example w/ tonic

07caec6

jackzampolin mentioned this pull request Aug 16, 2024

[EPIC] Gordian GRPC #13

Open

8 tasks

Reecepbcups added 7 commits August 19, 2024 11:40

Revert "gRPC: rust client example w/ tonic"

36e533e

This reverts commit 07caec6.

chore: cleanup (mark's req. changes)

7ddf4a2

feat: GRPC config & GetValidators endpoint

93d77a4

migrate: txbuf to internal/txmanager

c148faf

grpc: QueryAccountBalance

ba6fea0

feat: working gRPC Transaction endpoints

8876f05

chore: minor nits

7d4206f

Reecepbcups mentioned this pull request Aug 19, 2024

feat: wrap gRPC client within http handler #14

Draft

Reecepbcups added 2 commits August 19, 2024 14:58

nit: cleanups (comments, script, proto)

d7d3fd7

feat: TxResultResponse direct values

180ffd2

Reecepbcups force-pushed the reece/grpc branch from aa4ce9e to 180ffd2 Compare August 19, 2024 20:42

nit: last stale

39edef3

Reecepbcups changed the title ~~feat: gRPC~~ feat: gRPC server Aug 19, 2024

Reecepbcups commented Aug 19, 2024

View reviewed changes

Reecepbcups marked this pull request as ready for review August 19, 2024 20:52

Reecepbcups commented Aug 19, 2024

View reviewed changes

Reecepbcups requested a review from mark-rushakoff August 19, 2024 20:54

Reecepbcups added 2 commits August 19, 2024 17:12

fix: EOL at EOF

9b42df7

fix: add mis-deleted code snippet

939d712

mark-rushakoff reviewed Aug 20, 2024

View reviewed changes

Reecepbcups added 7 commits August 20, 2024 11:04

revert: internal/txmanager from gsi

a01a380

chore: comments & GOBIN installer for grpcurl

b369490

fix(macos); remove -w flag and pipe to tr newlines

22fed16

refactor: log location, Start -> serve(), grpc err handle

34b6f52

refactor: set validators at specific indexes

4ccc16b

refactor: user facing gRPC error handling

8953c7f

refactor: direct copy over SDK TxResult -> Gordian

b934e2b

Reecepbcups added 3 commits August 20, 2024 11:45

fix: remove BUG prefix for user facing issue

ce439ab

fix: cover rest of wrapped errors

63816ab

fix: g.grpcServer.Stop()

32a59be

mark-rushakoff approved these changes Aug 21, 2024

View reviewed changes

mark-rushakoff merged commit 53020d6 into main Aug 21, 2024
0 of 2 checks passed

mark-rushakoff deleted the reece/grpc branch August 21, 2024 13:39

Reecepbcups mentioned this pull request Aug 21, 2024

refactor: grpc base structure #16

Merged

		@@ -3,3 +3,5 @@

		# go build . will make a binary named gcosmos, which we never want to commit.
		gcosmos

		grpcurl -plaintext -emit-defaults -d '{"tx":"'$(cat example-tx-signed.json \| base64 -w 0)'"}' localhost:9092 server.GordianGRPC/SimulateTransaction

		grpcurl -plaintext -emit-defaults -d '{"tx":"'$(cat example-tx-signed.json \| base64 -w 0)'"}' localhost:9092 server.GordianGRPC/SubmitTransaction

		cp -r ./github.com/rollchains/gordian/gcosmos/* .
		rm -rf ./github.com

		var opts []grpc.ServerOption
		g.grpcServer = grpc.NewServer(opts...)


		option go_package = "github.com/rollchains/gordian/gcosmos/gserver/internal/ggrpc";

		package server;

feat: gRPC server #8

feat: gRPC server #8

Conversation

Reecepbcups commented Aug 16, 2024 • edited Loading

Summary

Future

Reecepbcups Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reecepbcups Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reecepbcups Aug 19, 2024 • 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

mark-rushakoff 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-rushakoff 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

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

Reecepbcups commented Aug 16, 2024 •

edited

Loading

Reecepbcups Aug 19, 2024 •

edited

Loading

Reecepbcups Aug 20, 2024 •

edited

Loading

Reecepbcups Aug 19, 2024 •

edited

Loading