Feature/kgw jsonrpc #700

Yaiba · 2024-05-03T15:24:07Z

This resolves #694

This add a new kgw jsonrpc client, which will do all interactions with GKW using json rpc.

Yaiba · 2024-05-07T03:01:03Z

After some dev on this issue, I realized that we might not want to support change the impl between jsonrpc and REST in gatewayclient, as it will require two set of configurations(kgw API can support both, but it requires different configuration of kwil-db REST/jsonrpc backend)

Yaiba · 2024-05-08T20:38:05Z

core/rpc/client/gateway/jsonrpc/client.go

+	jsonrpc "github.com/kwilteam/kwil-db/core/rpc/json"
+)
+
+const (


Maybe should move to core/rpc/json ?

maybe not, admin and function methods are in core/rpc/json because it's also used in JSON rpc service server.

Yaiba · 2024-05-08T22:47:26Z

core/rpc/client/gateway/jsonrpc/client.go

+	}
+	err := errors.Join(jsonRPCErr, rpcErr)
+
+	switch jsonRPCErr.Code {


It seems we can delete this block(in every client)? IMO the jsonRPCErr.Code is very specific, and more useful for app? @jchappelow

I think that the highest level clients want more abstraction i.e. shouldn't have to know about the jsonrpc package and codes at all. Also errors.Is(err, <some error kind in core/client package>) is better for a user of core/client.Client I think.

Codes could move up to a package that's not jsonrpc specific? Or core/client could give a error code getter to extract one from this opaque error?

Oh. this is in the jsonrpc package. Hmm. I guess core/client should be checking codes an translating to Go error types...

You mean in core/client and core/gatewayclient ? yeah maybe, I just see those errors are not used much really, most of the time we just pop it up

Haven't gotten a solid solution yet, I'll leave it for now

jchappelow · 2024-05-10T22:46:45Z

core/rpc/client/gateway/jsonrpc/client.go

+// call sends a JSON-RPC request to the gateway.
+// NOTE: this is basically a copy of core/rpc/client/user/jsonrpc/client.call
+func (cl *Client) call(ctx context.Context, method string, cmd, res any) error {


In the other client we can export and call this CallMethod, so that gateway client can embed it perhaps? We just want to add the two auth related methods right?

yeah I thought about it and totally forgot about it. I think we should do it. I copied bcz that's when we have two different branch , and my branch is based on another branch..

Alright I'll make the change

Plus I think if we go that route, we don't have to worry about id := g.reqID.Add(-1) // Decrement by 1, so it won't be duplicated with txClient

jchappelow · 2024-05-10T22:50:24Z

core/gatewayclient/client.go

+	var jsonRPCErr *jsonrpc.Error
+	if errors.As(err, &jsonRPCErr) {
+		if jsonRPCErr.Code != jsonrpc.ErrorKGWNotAuthorized {


Ah. Yup! core/gatewayclient can indeed interpret based on the jsonrpc code.

Yeah, this is why I have the question #700 (comment).

In this package, this makes sense. For the consumer of core/client or core/gatewayclient, they don't need know json rpc codes. If they want to introspect to see if it's a json-rpc error that's fine, but it's preferable for external developers to be able to do a simple errors.Is with one of our predefined types. At least that's how I feel for now.

Yaiba · 2024-05-10T22:14:41Z

core/gatewayclient/client.go

-	if !errors.Is(err, rpcClient.ErrUnauthorized) {
-		return nil, err
+
+	var jsonRPCErr *jsonrpc.Error


should prefer this way of handling error or below?

ah just saw you commit

Yaiba · 2024-05-13T22:12:43Z

core/rpc/client/jsonrpc.go

+// JSONRPCClient is a JSON-RPC client that handles sending and receiving
+// JSON-RPC requests and responses. It is a low-level client that does not
+// care about the specifics of the JSON-RPC methods or responses.
+type JSONRPCClient struct {


Well the naming is kind awkward, I tempted to name it like BaseRPCClient. The reason is in the future say we support WS, we might have another base client

Meh. But if we support websocket, that's still json-rpc, just using websocket. Anyway, I think JSONRPCClient is appropriate, at least while it is defined in core/rpc/client.

Yaiba · 2024-05-13T22:19:00Z

core/gatewayclient/client.go

 )

 // GatewayClient is a client that is made to interact with a kwil gateway.
 // It inherits the functionality of the main Kwil client, but also provides
 // authentication cookies to the gateway.
 // It automatically handles the authentication process with the gateway.
 type GatewayClient struct {
-	client.Client
+	client.Client // core client


GatwayClient still needs this type embeded, I cannot move it to core/rpc/gateway/, as core/rpc are just client for handling exact input/output of APIs, but core/client deals with user input/outputs.

Yaiba · 2024-05-13T22:27:49Z

core/types/client/opts.go

@@ -45,13 +49,18 @@ func (c *Options) Apply(opts *Options) {
 		c.ChainID = opts.ChainID
 	}

+	if opts.Conn != nil {


Just a thought on the style.

TBH, I don't like this way of passing options, i.e. Client(xx, optionObj), I prefer Client(xx, WithOptA, WithOptB...), and I kind of want to be able to apply those options to all our clients.
Currently we can reuse options though embed(like core/gatewayclient/opts.GatewayOptions) , I'm not sure how to do it in Client(xx, WithOptA...) style, maybe using interface.

I don't have strong feelings about this, but the functional options pattern is generally more cumbersome and time consuming to use and modify compared to a config struct. Yet most of our code does functional options, so might as well here.

In general, I think the main benefit of functional options is on the highest level exported APIs because a config struct changing is technically a breaking change to the constructor while new option functions are not. With how unstable our Go APIs are all around, this benefit is almost non-existent.

Ironically, the most benefit for functional options is in core/client.NewClient, but the transport-aware clients deeper in core/rpc/client/... benefit less from doing it that way.

Anyway, I don't mind any options refactoring you'd like to do, but the more internal things get, the less important it is to use functional options.

Yeah, just have this thought so it's consistent everywhere.
I can work with current approach, at least all exposed clients are using config struct, it's consistent.

Yaiba · 2024-05-13T22:30:46Z

core/rpc/http/function/client.go

@@ -52,7 +52,7 @@ type service struct {
 }

 // NewAPIClient creates a new API client. Requires a userAgent string describing your application.
-// optionally a custom http.Client to allow for advanced features such as caching.
+// optionally a custom http.JSONRPCClient to allow for advanced features such as caching.


ehh sometime my IDE suggests too much refactor... Will change it back

jchappelow · 2024-05-14T17:57:42Z

core/rpc/client/user/jsonrpc/client.go

-//    message (pb) types sent in POST, or just GET with endpoint implying method.
-//  - the "method" in the outer request type is instead of the endpoint, all POST
-
+// Client is a JSON-RPC client for the Kwil user service. It use the JSONRPCClient


If we keep this file named "methods.go" (and squash all commits in this PR), git detects the rename/move of client.go and the diff for both that file and methods.go is minimized.

i.e.

-------------------------- core/rpc/client/jsonrpc.go -------------------------- similarity index 81% rename from core/rpc/client/user/jsonrpc/client.go rename to core/rpc/client/jsonrpc.go index cecb7df8..2059a708 100644

and core/rpc/client/user/jsonrpc/methods.go will have about a dozen lines changed.

cool, I was totally not thinking about this. will change .

That change seems to have worked out well. We can track the changes now, and the PR was more reviewable. Thanks.

Thanks for reminding me this aspect as well

This add jsonrpc gateway client and use it as default implementation for gatewayclient. It has three methods, all in `kgw.` namespace, `kgw.authn_param`, `kgw.authn` and `kgw.logout` It has some duplicate(copied) base jsonrpc handling from core/rpc/client/user/jsonrpc

This extract JSONRPCClient type to handles request/response for JSON RPC. Thus elimates duplicated code in core/rpc/client/gateway/jsonrpc

jchappelow

I think this "base client" / JSONRPCClient approach does the trick nicely.
I think you're still working on some changes though?

Yaiba · 2024-05-14T19:32:41Z

I think this "base client" / JSONRPCClient approach does the trick nicely. I think you're still working on some changes though?

I've done what I wanted. Will squash and merge.

jchappelow added this to the v0.8.0 milestone May 3, 2024

Yaiba force-pushed the feature/kgw-jsonrpc branch from 0b1a902 to fe11688 Compare May 6, 2024 22:55

Yaiba force-pushed the feature/kgw-jsonrpc branch from 336e069 to cbbd51f Compare May 7, 2024 16:35

Yaiba commented May 8, 2024

View reviewed changes

Yaiba force-pushed the feature/kgw-jsonrpc branch 2 times, most recently from 1b54713 to a009f24 Compare May 8, 2024 21:29

Yaiba marked this pull request as ready for review May 8, 2024 21:29

Yaiba commented May 8, 2024

View reviewed changes

Yaiba force-pushed the feature/kgw-jsonrpc branch from 04a05ea to 81cc307 Compare May 10, 2024 18:46

Yaiba changed the title ~~[WIP] Feature/kgw jsonrpc~~ Feature/kgw jsonrpc May 10, 2024

Yaiba force-pushed the feature/kgw-jsonrpc branch 3 times, most recently from e92db20 to 56f6519 Compare May 10, 2024 20:51

jchappelow reviewed May 10, 2024

View reviewed changes

Yaiba commented May 10, 2024

View reviewed changes

Yaiba force-pushed the feature/kgw-jsonrpc branch 3 times, most recently from dcb7514 to 29bb90b Compare May 13, 2024 20:17

Yaiba requested a review from jchappelow May 13, 2024 21:55

Yaiba commented May 13, 2024

View reviewed changes

jchappelow reviewed May 14, 2024

View reviewed changes

Yaiba added 2 commits May 14, 2024 13:30

core: extract JSONRPCClient and embed it

70ec5d2

This extract JSONRPCClient type to handles request/response for JSON RPC. Thus elimates duplicated code in core/rpc/client/gateway/jsonrpc

Yaiba force-pushed the feature/kgw-jsonrpc branch from c25fc68 to 70ec5d2 Compare May 14, 2024 18:31

jchappelow approved these changes May 14, 2024

View reviewed changes

Yaiba merged commit c61ba10 into kwilteam:main May 14, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/kgw jsonrpc #700

Feature/kgw jsonrpc #700

Yaiba commented May 3, 2024 •

edited

Loading

Yaiba commented May 7, 2024

Yaiba May 8, 2024

Yaiba May 10, 2024

Yaiba May 8, 2024

jchappelow May 10, 2024

jchappelow May 10, 2024

jchappelow May 10, 2024

Yaiba May 10, 2024

Yaiba May 13, 2024

jchappelow May 10, 2024

Yaiba May 10, 2024

Yaiba May 10, 2024

jchappelow May 13, 2024

jchappelow May 10, 2024

Yaiba May 10, 2024

jchappelow May 14, 2024

Yaiba May 10, 2024

Yaiba May 10, 2024

Yaiba May 13, 2024

jchappelow May 14, 2024

Yaiba May 13, 2024

Yaiba May 13, 2024 •

edited

Loading

jchappelow May 14, 2024 •

edited

Loading

Yaiba May 14, 2024

Yaiba May 13, 2024

jchappelow May 14, 2024 •

edited

Loading

Yaiba May 14, 2024

jchappelow May 14, 2024

Yaiba May 14, 2024

jchappelow left a comment

Yaiba commented May 14, 2024

Feature/kgw jsonrpc #700

Feature/kgw jsonrpc #700

Conversation

Yaiba commented May 3, 2024 • edited Loading

Yaiba commented May 7, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yaiba May 13, 2024 • edited Loading

Choose a reason for hiding this comment

jchappelow May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow May 14, 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

jchappelow left a comment

Choose a reason for hiding this comment

Yaiba commented May 14, 2024

Yaiba commented May 3, 2024 •

edited

Loading

Yaiba May 13, 2024 •

edited

Loading

jchappelow May 14, 2024 •

edited

Loading

jchappelow May 14, 2024 •

edited

Loading