make gatewayConnector into a gRPC service#1233
Conversation
… in capability repo
221b69a to
59461ad
Compare
a97cdd2 to
59461ad
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the gatewayConnector into a gRPC service while extracting common gateway code for reuse in the capabilities repo. Key changes include new proto definitions and generated files for gateway messages, the addition of gRPC client/server implementations for gateway connectors, and updates to capabilities services to integrate the gatewayConnector dependency.
Reviewed Changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/loop/internal/pb/gatewayconnector/gateway_common.proto | New proto definitions for gateway message types |
| pkg/loop/internal/core/services/gateway/gateway_connector_handler.go | Implementation of the gateway connector handler client/server |
| pkg/loop/internal/core/services/gateway/gateway_connector.go | Adds gRPC client and server for gateway connector and integrates AddHandler functionality |
| pkg/loop/internal/core/services/capability/standard/* | Updates to include a new gatewayConnector dependency in capability initialization flows |
| pkg/capabilities/* | Updates across capabilities proto and generated files to add the gateway connector parameter |
| go.mod | Minor dependency updates |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| func (cs *CronServer) Initialise(ctx context.Context, config string, telemetryService core.TelemetryService, store core.KeyValueStore, capabilityRegistry core.CapabilitiesRegistry, errorLog core.ErrorLog, pipelineRunner core.PipelineRunnerService, relayerSet core.RelayerSet, oracleFactory core.OracleFactory) error { | ||
| if err := cs.CronCapability.Initialise(ctx, config, telemetryService, store, errorLog, pipelineRunner, relayerSet, oracleFactory); err != nil { | ||
| func (cs *CronServer) Initialise(ctx context.Context, config string, telemetryService core.TelemetryService, store core.KeyValueStore, capabilityRegistry core.CapabilitiesRegistry, errorLog core.ErrorLog, pipelineRunner core.PipelineRunnerService, relayerSet core.RelayerSet, oracleFactory core.OracleFactory, gatewayConnector core.GatewayConnector) error { | ||
| if err := cs.CronCapability.Initialise(ctx, config, telemetryService, store, errorLog, pipelineRunner, relayerSet, oracleFactory, gatewayConnector); err != nil { |
There was a problem hiding this comment.
This will fail with the current Cron Trigger https://github.com/smartcontractkit/capabilities/blob/main/cron/trigger/trigger.go#L100
There was a problem hiding this comment.
will update once this PR is merged
| pipelineRunner core.PipelineRunnerService, | ||
| relayerSet core.RelayerSet, | ||
| oracleFactory core.OracleFactory, | ||
| gatewayConnector core.GatewayConnector, |
There was a problem hiding this comment.
This change will need to be carried through to each capability. Is there a fast follow planned for that?
There was a problem hiding this comment.
yes can update all capabilities after this PR is merged
| ) | ||
|
|
||
| // GatewayConnector is a component run by Nodes to connect to a set of Gateways. | ||
| type GatewayConnector interface { |
There was a problem hiding this comment.
Is there a ticket already made to track consuming this code shuffle back into core?
There was a problem hiding this comment.
| // GatewayConnector is a component run by Nodes to connect to a set of Gateways. | ||
| type GatewayConnector interface { | ||
| // Start starts the GatewayConnector | ||
| Start(context.Context) error |
There was a problem hiding this comment.
I think we don't need Start and Close here. This interface is exposed to capabilities but under the hood there is a singleton object and its lifecycle is managed by core.
There was a problem hiding this comment.
I was under the impression that this interface is used in both core and capabilities. Are you suggesting we keep this interface narrow and define a broader interface in core like below? That sounds totally feasible too:
type GatewayConnector interface {
services.Service
network.ConnectionInitiator
core.GatewayConnector // narrow interface defined in chainlink-common for shared functionalities
// between core and capabilities repo
}
|
|
||
| import "google/protobuf/empty.proto"; | ||
|
|
||
| message MessageBody { |
There was a problem hiding this comment.
I don't like that the Message struct is duplicated as a proto. It was supposed to be an internal representation. Over the wire we send JSON-RPC-encoded messages. If @DeividasK adds his own version of Message, we would need to duplicate it too.
Another option to consider is to send raw bytes in the gRPC service and decode in the Handler using our own JSON-RPC codec. WDYT? cc @cedric-cordenier
Longer term, when we move to don2don framework for Gateways, we'll be passing raw bytes anyway.
There was a problem hiding this comment.
Does that imply that original JSON-RPC from the user will be relayed from the gateway to the DONs as "pass-through"? What if the gateway needs to append some data before sending the request data to the DONs? Sending raw bytes makes sense to me but the underlying message format should allow for wrapping the original request with additional data. Gateway Message format already allows for such flexibility because it has a raw bytes payload field. Thoughts?
There was a problem hiding this comment.
Another option to consider is to send raw bytes in the gRPC service and decode in the Handler using our own JSON-RPC codec. WDYT? cc @cedric-cordenier
I agree with this (and in fact that was my understanding of how Deividas was going to refactor things anyway)
|
|
||
| message SendMessageRequest { | ||
| string gateway_id = 1; | ||
| Message message = 2; |
There was a problem hiding this comment.
We could put a byte array here (see comment above).
|
Closing this PR and dividing it into 3 separate ones:
cc @bolekk |
https://smartcontract-it.atlassian.net/browse/PRODCRE-409