-
Notifications
You must be signed in to change notification settings - Fork 13
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
[RelayRequest] Don't Unmarshal Request Payloads #186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking a pretty big structural question before moving on.
pkg/appgateserver/jsonrpc.go
Outdated
@@ -13,6 +14,15 @@ import ( | |||
sharedtypes "github.com/pokt-network/poktroll/x/shared/types" | |||
) | |||
|
|||
// InterimJSONRPCRequestPayload is a partial JSON RPC request payload that | |||
// excludes the params field, which is unmarshaled sperately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// excludes the params field, which is unmarshaled sperately. | |
// excludes the params field, which is unmarshaled separately. | |
// This is used because the proto object cannot be unmarshalled directly, so this is a | |
// temporary object to be used interim. |
pkg/appgateserver/jsonrpc.go
Outdated
@@ -13,6 +14,15 @@ import ( | |||
sharedtypes "github.com/pokt-network/poktroll/x/shared/types" | |||
) | |||
|
|||
// InterimJSONRPCRequestPayload is a partial JSON RPC request payload that | |||
// excludes the params field, which is unmarshaled sperately. | |||
type InterimJSONRPCRequestPayload struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the struct and its types need to be exposed publically?
proto/pocket/service/relay.proto
Outdated
} | ||
|
||
// JSONRPCRequestPayloadParamsList contains the list of parameters for a JSON-RPC request. | ||
// TODO_RESEARCH: Will this always be a string list? Or should we use a more generic type? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it and realized this could be a string of objects: [obj1, obj2, etc..]
.
Wdyt of just making prams
an Any
like you suggested in discord?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Olshansk I think an approach could be storing the payload as its bytes. And just passing these directly. The downside of this is what if we wanted to do compute units?
Answer: we interim unmarshal without the params as compute units should probably only depend on the method being called (params shouldn't affect the method as the method's "weighting" would factor in the types of params it receives) and assign the weighting that way.
The reason I am against using Any
in the protobuf is that the protobuf Any
isn't 1-1 correspondance with the go any
ie we cannot unmarhsal any type into an Any
an any needs to be an Any
type in go which the unmarhsaling step would again require some interim step of unmarshaling without the params and then a longer type checking.
Overall I think us trying to proto-ify the payload is probably the wrong path. What do we need it to be a protobuf for? Why cant be just directly pass the payload from the request to the service? Personally I think making the payload a protobuf is not for us to deal with - as we dont do anything with it, unlike the other field of the request structure.
WDYT?
8c5ae31
to
e58c848
Compare
JSONRPCRequestPayload
Summary
Human Summary
[]byte
and dont unmarshal themAI Summary
Summary generated by Reviewpad on 14 Nov 23 22:53 UTC
This pull request introduces a feature that generalizes the parameters in the JSON RPC request to be either maps or lists. It modifies multiple files to implement this feature.
Issue
Type of change
Select one or more:
Testing
make go_develop_and_test
Sanity Checklist