[WIP][Shannon][Refactor] Create GatewayClient and move relevant code from PATH to Shannon SDK #39
Conversation
commoddity
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| servicetypes "github.com/pokt-network/poktroll/x/service/types" | ||
| ) | ||
|
|
||
| // TODO_IN_THIS_PR(@commoddity): create a new `relay_test.go` file the tests the process of signing and building a relay request. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_PR(@commoddity): create a new relay_test.go file the tests the process of signing and building a relay request.
client/relay_signer.go
Outdated
| return relayRequest, nil | ||
| } | ||
|
|
||
| // TODO_IN_THIS_PR(@commoddity): add detailed godoc comment explaining the purpose of this struct. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_PR(@commoddity): add detailed godoc comment explaining the purpose of this struct.
client/fullnode.go
Outdated
| sdkTypes "github.com/pokt-network/shannon-sdk/types" | ||
| ) | ||
|
|
||
| // TODO_IN_THIS_PR(@commoddity): start a "micro readme" (just with bullet points) that captures more details about full node implementations? |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_PR(@commoddity): start a "micro readme" (just with bullet points) that captures more details about full node implementations?
client/gateway_client.go
Outdated
|
|
||
| // GetActiveSessions retrieves active sessions for a list of app addresses and a service ID. | ||
| // It verifies that each app delegates to the gateway and is staked for the requested service. | ||
| // This method encapsulates the shared logic between centralized and delegated gateway modes. |
There was a problem hiding this comment.
Not sure if this should be kept: IMO the first iteration of refactor should keep Gateway Mode concept entirely on PATH.
There was a problem hiding this comment.
This method does not deal with anything specific to the gateway mode. I've removed the comment as it was misguided and confusing.
Please see the files protocol/shannon/client_centralized.go and protocol/shannon/client_delegated.go for details.
|
|
||
| if err := relayResponse.ValidateBasic(); err != nil { | ||
| // Even if the relay response is invalid, return it (may contain failure reason) | ||
| return relayResponse, err |
There was a problem hiding this comment.
Please define custom errors for these: the current version of SDK already has that.
There was a problem hiding this comment.
I've updated the gateway client file to greatly improve custom error definitions and logging of said errors.
client/relay_signer.go
Outdated
| // | ||
| // - Returns a pointer instead of directly setting the signature on the input relay request to avoid implicit output. | ||
| // - Ideally, the function should accept a struct rather than a pointer, and also return an updated struct instead of a pointer. | ||
| func (s *relaySigner) SignRelayRequest( |
There was a problem hiding this comment.
This part looks incomplete: how is a gateway (e.g. PATH) going to call this method?
SDK needs to provide this method, e.g. on the GatewayClient, for use by PATH.
There was a problem hiding this comment.
I've updated this method to be on the GatewayClient struct.
See usage of the method in PATH here:
func (rc *requestContext) signRelayRequest(ctx context.Context, unsignedRelayReq *servicetypes.RelayRequest, app apptypes.Application) (*servicetypes.RelayRequest, error) {
// Verify the relay request's metadata, specifically the session header.
// Note: cannot use the RelayRequest's ValidateBasic() method here, as it looks for a signature in the struct, which has not been added yet at this point.
meta := unsignedRelayReq.GetMeta()
if meta.GetSessionHeader() == nil {
return nil, errors.New("signRelayRequest: relay request is missing session header")
}
sessionHeader := meta.GetSessionHeader()
if err := sessionHeader.ValidateBasic(); err != nil {
return nil, fmt.Errorf("signRelayRequest: relay request session header is invalid: %w", err)
}
// Sign the relay request using the selected app's private key
return rc.gatewayClient.SignRelayRequest(ctx, unsignedRelayReq, app)
}
client/fullnode_cache.go
Outdated
| // newFullNodeWithCache wraps a fullNode with: | ||
| // - Session cache: refreshes early to avoid thundering herd/latency spikes | ||
| // - Account public key cache: indefinite cache for account data | ||
| func newFullNodeWithCache( |
There was a problem hiding this comment.
I don't think fullnode* structs should be private: making them public and allowing the user (e.g. PATH) to customize any required aspect e.g. GRPC settings, makes more sense.
Also, please consider the approach of making fullnode* structs useful on their own: e.g. a reliable way of interacting with the protocol for purposes other than running a gateway.
There was a problem hiding this comment.
This has been changed as part of the GatewayClientCache changes.
Everything is configurable through the config structs; please let me know if you think things should be more configurable than they are now and in which ways.
adshmh
left a comment
adshmh
left a comment
There was a problem hiding this comment.
Thank you for the PR, this is a good start for the refactoring work.
Please consider making things less monolithic and more configurable, with reasonable default values/behavior.
@adshmh I've thought a bit about your feedback and implemented some changes. The main one being the renaming of the "lazy" full node into a In addition, this introduces a new public interface, the As always I am open to improvements in the naming of these components. 😅 As a result of this change, most comments here are outdated, but I have gone through and addressed each one individually as well. |
| ) | ||
|
|
||
| // GRPCClient implements OnchainDataFetcher interface. | ||
| var _ OnchainDataFetcher = &GRPCClient{} |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
var _ OnchainDataFetcher = &GRPCClient{}
| ) | ||
|
|
||
| // GatewayClientCache implements OnchainDataFetcher interface. | ||
| var _ OnchainDataFetcher = &GatewayClientCache{} |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
var _ OnchainDataFetcher = &GatewayClientCache{}
Olshansk
changed the title
This PR must be reviewed in conjunction with PATH PR 297:
pokt-network/path#297
🚨 TODO_IN_THIS_PR
Do not merge until these TODOs are resolved:
relay_test.gofile the tests the process of signing and building a relay request. - relay.go🌿 Summary
Refactor Shannon SDK by creating
GatewayClientand moving relevant code from PATH to Shannon SDK.🌱 Primary Changes:
GatewayClientstruct to handle signing and validating relays with full node integrationFullNodeinterface with caching and non-caching implementations🍃 Secondary changes:
queryCodecand moved codec initialization to types package🛠️ Type of change
Select one or more from the following:
🤯 Sanity Checklist