[CAPPL-19] Gateway Connector service wrapper #14356

DavidOrchard · 2024-09-05T23:24:40Z

Add serviceWrapper for gatewayconnector, cappl-19

cedric-cordenier · 2024-09-06T15:37:19Z

core/capabilities/gateway_connector/service_wrapper.go

+		if len(enabledKeys) == 0 {
+			return errors.New("no available keys found")
+		}
+		signerKey := enabledKeys[0].ToEcdsaPrivKey()


Open question: how do we handle multiple keys being enabled on the node? Will we always pick the same key?

bolekk · 2024-09-06T18:09:51Z

core/capabilities/gateway_connector/service_wrapper.go

+}
+
+func (e *serviceWrapper) Close() error {
+	return e.StopOnce("WorkflowConnectorHandler", func() (err error) {


should be "GatewayConnectorServiceWrapper"

bolekk · 2024-09-06T18:11:21Z

core/capabilities/gateway_connector/service_wrapper.go

+		}
+		// cannot assign to conf.NodeAddress (neither addressable nor a map index expression
+		// I think I'm still confused about mixing interfaces and structures, and how to use a setter on an interface.
+		conf.NodeAddress = enabledKeys[idx].ID()


You don't have to do this at all. You already used the configured address to find the needed key. No need to modify the config struct.

bolekk · 2024-09-06T18:16:40Z

core/capabilities/gateway_connector/service_wrapper.go

+			return err
+		}
+		translated := translateConfigs(conf)
+		e.connector, err = connector.NewGatewayConnector(&translated, handler, handler, clockwork.NewRealClock(), e.lggr)


With the proposed changes to GatewayConnector, soon you won't need to provide the handler here any more in the constructor (there will be an AddHandler() method to do it dynamically and possibly multiple times). For now, we should strip the workflowConnectorHandler into being only a Signer like I suggested above. Let's also move it into the service_wrapper.go file for simplicity.

bolekk · 2024-09-06T18:18:12Z

core/capabilities/gateway_connector/connector_handler.go

+	"github.com/smartcontractkit/chainlink/v2/core/services/gateway/connector"
+)
+
+type workflowConnectorHandler struct {


Let's rename it to connectorSigner, strip down as much as we can and put it together with the Wrapper in the same file. Soon the handler portion will go away as I explained in the other comment below.

bolekk · 2024-09-06T18:18:40Z

core/capabilities/gateway_connector/connector_handler.go

+	lggr        logger.Logger
+}
+
+type Response struct {


remove this

bolekk · 2024-09-06T18:18:44Z

core/capabilities/gateway_connector/connector_handler.go

+
+var (
+	_ connector.Signer = &workflowConnectorHandler{}
+	// _ connector.GatewayConnectorHandler = &workflowConnectorHandler{}


remove those

core/capabilities/gateway_connector/service_wrapper.go

bolekk · 2024-09-06T20:32:20Z

core/capabilities/gateway_connector/service_wrapper.go

+	lggr        logger.Logger
+}
+
+var (


nit: you can simplify to one line by removing the parentheses

bolekk · 2024-09-06T20:33:02Z

core/capabilities/gateway_connector/service_wrapper.go

+	_ connector.Signer = &workflowConnectorSigner{}
+)
+
+func NewWorkflowConnectorSigner(config *config.GatewayConnector, signerKey *ecdsa.PrivateKey, lggr logger.Logger) (*workflowConnectorSigner, error) {


Let's remove the word "workflow" from everywhere. Just ConnectorSigner.

bolekk · 2024-09-06T20:33:22Z

core/capabilities/gateway_connector/service_wrapper.go

+func (h *workflowConnectorSigner) HandleGatewayMessage(ctx context.Context, gatewayID string, msg *api.Message) {
+}
+func (h *workflowConnectorSigner) Start(ctx context.Context) error {
+	return h.StartOnce("WorkflowConnectorHandler", func() error {


also here in those strings, just "ConnectorSigner"

bolekk · 2024-09-06T20:33:38Z

core/capabilities/gateway_connector/service_wrapper.go

+		r.WsClientConfig = network.WebSocketClientConfig{HandshakeTimeoutMillis: f.WSHandshakeTimeoutMillis()}
+	}
+
+	// 0 are valid values


nit: unnecessary comment

I got the response that I was looking for, which is you confirming that 0 is a valid value :-)

bolekk · 2024-09-06T20:33:58Z

core/capabilities/gateway_connector/service_wrapper.go

+
+func translateConfigs(f config.GatewayConnector) connector.ConnectorConfig {
+	r := connector.ConnectorConfig{}
+	if f.NodeAddress() != "" {


You don't need those checks. If it's empty, it's empty.

bolekk · 2024-09-06T20:34:11Z

core/capabilities/gateway_connector/service_wrapper.go

+		}
+	}
+
+	if !reflect.ValueOf(f.WSHandshakeTimeoutMillis).IsZero() {


bolekk · 2024-09-06T20:36:46Z

core/capabilities/gateway_connector/service_wrapper.go

+	}
+}
+
+func (e *serviceWrapper) Start(ctx context.Context) error {


Can you add a unit test that creates the ServiceWrapper object and then calls Start() can Close() on it, please?
You can take inspiration from functions/plugin_test.go and functions/connector_handler_test.go on how to mock the dependencies.

Ideally you'd test valid NodeAddress and an invalid one (i.e. key doesn't exit).

bolekk · 2024-09-06T20:37:33Z

core/capabilities/gateway_connector/service_wrapper.go

@@ -0,0 +1,164 @@
+package gatewayConnector


I believe the convention is to use gateway_connector here.

lint barfed and said no underscores in package names.

Typically Go package names are not even full words, let alone multiple words.

Good package names are short and clear. They are lower case, with no under_scores or mixedCaps. They are often simple nouns...

Some resources:

https://go.dev/blog/package-names

https://go.dev/doc/effective_go#package-names

In this case, perhaps something like gatewayconn.

We should keep the directory aligned too, rather than just rename the package.

Interesting... we have a lot of package names with underscores throughout core. I'll remember for the future, thanks!

I find this guidance a bit confusing. gatewayConnector is bad but gatewayconn or gatewayconnector is good? I find the 2nd & 3rd a bit harder to read because it is 2 words and almost all the same height. Obviously I have to respect the lint standards, but I'm not bought into the standard.

I think you are also suggesting the directory should change from gateway_connector to gatewayconnector, though that's not in the lint afaik.

bolekk · 2024-09-09T22:38:38Z

core/capabilities/gateway_connector/service_wrapper_test.go

+	_, addr := testutils.NewPrivateKeyAndAddress(t)
+
+	config, err := chainlink.GeneralConfigOpts{}.New()
+	// I don't think this is the right way to get the GatewayConnector in.


It's not. You should do it in the line above. Instead of creating an empty GeneralConfigOpts{} object, you create one with desired config already in it. Something like:

config, err := GeneralConfigOpts{ Config: chainlink.Config{ Core: toml.Core{ Capabilities: toml.Capabilities{ GatewayConnector: toml.GatewayConnector{ // all the fields here }, }, }, }, }.New()

bolekk · 2024-09-09T22:40:12Z

core/capabilities/gateway_connector/service_wrapper_test.go

+
+	"github.com/smartcontractkit/chainlink/v2/core/internal/testutils"
+	"github.com/smartcontractkit/chainlink/v2/core/logger"
+	chainlink "github.com/smartcontractkit/chainlink/v2/core/services/chainlink/config"


No such package, I think you just wanted "(...)/services/chainlink"

bolekk · 2024-09-09T22:41:58Z

core/capabilities/gateway_connector/service_wrapper_test.go

+
+	t.Run("Start Error", func(t *testing.T) {
+		// Question, what's the best practices when testing a different configuration, is it 2 configs and then
+		// 2 handlers, or does the test do the initialization as in copy from line 27 to 41 into here?


Initialization is executed only once and then both .Run() sections re-use it. So you need to somehow create 2 configs, one valid and one invalid.

bolekk · 2024-09-09T22:43:18Z

core/capabilities/gateway_connector/service_wrapper.go

+			return err
+		}
+		translated := translateConfigs(conf)
+		e.connector, err = connector.NewGatewayConnector(&translated, signer, signer, clockwork.NewRealClock(), e.lggr)


I changed the signature of NewGatewayConnector() in this PR: #14367

bolekk · 2024-09-09T23:51:09Z

core/capabilities/gateway_connector/service_wrapper_test.go

+}
+
+// Unit test that creates the ServiceWrapper object and then calls Start() can Close() on it.
+// Take inspiration from functions/plugin_test.go and functions/connector_handler_test.go on how to mock the dependencies.


You can remove all these comments and instead name the tests:

TestGatewayConnectorServiceWrapper_CleanStartClose

TestGatewayConnectorServiceWrapper_NonexistentKey

bolekk · 2024-09-09T23:51:18Z

core/capabilities/gateway_connector/service_wrapper_test.go

+
+	config, err := generateConfig(addr)
+	ethKeystore := ksmocks.NewEth(t)
+	ethKeystore.On("EnabledKeysForChain", mock.Anything).Return([]ethkey.KeyV2{{Address: addr}})


EnabledKeysForChain takes two arguments, not one.

bolekk · 2024-09-09T23:51:58Z

core/capabilities/gateway_connector/service_wrapper_test.go

+		assert.NoError(t, handler.Close())
+	})
+
+	t.Run("Start & Stop Success", func(t *testing.T) {


You don't really need this t.Run() section here, especially that it's only a single test case.

bolekk · 2024-09-09T23:52:33Z

core/capabilities/gateway_connector/service_wrapper_test.go

+	require.NoError(t, err)
+
+	t.Cleanup(func() {
+		assert.NoError(t, handler.Close())


I think you can't have that and also call Close() manually below (Close will error if called more than once) - but please confirm.

bolekk · 2024-09-09T23:53:37Z

core/capabilities/gateway_connector/service_wrapper_test.go

+	ethKeystore.On("EnabledKeysForChain", mock.Anything).Return([]ethkey.KeyV2{{Address: addr2}})
+
+	gc := config.Capabilities().GatewayConnector()
+	handler := NewGatewayConnectorServiceWrapper(&gc, ethKeystore, logger)


Should be named wrapper, not handler.

bolekk · 2024-09-09T23:53:55Z

core/capabilities/gateway_connector/service_wrapper_test.go

+	"github.com/test-go/testify/mock"
+)
+
+func generateConfig(addr common.Address) (chainlink.GeneralConfig, error) {


You can put more shared logic in this helper. Instead of returning config, return the whole Wrapper object with mocks already set.

Ok, I'd wanted to do that but apparently misunderstood your earlier comment.

bolekk · 2024-09-10T00:06:21Z

core/capabilities/gateway_connector/service_wrapper.go

+type serviceWrapper struct {
+	services.StateMachine
+
+	config    *config.GatewayConnector


No pointer here - GatewayConnector in an interface.

bolekk · 2024-09-10T00:06:35Z

core/capabilities/gateway_connector/service_wrapper.go

+
+var _ connector.Signer = &connectorSigner{}
+
+func NewConnectorSigner(config *config.GatewayConnector, signerKey *ecdsa.PrivateKey, lggr logger.Logger) (*connectorSigner, error) {


No pointer needed for config.

bolekk · 2024-09-10T00:06:39Z

core/capabilities/gateway_connector/service_wrapper.go

+}
+
+// NOTE: this wrapper is needed to make sure that our services are started after Keystore.
+func NewGatewayConnectorServiceWrapper(config *config.GatewayConnector, keystore keystore.Eth, lggr logger.Logger) *serviceWrapper {


GatewayConnector is an interface, so you don't need a pointer (*)

bolekk · 2024-09-10T00:11:11Z

core/capabilities/gateway_connector/service_wrapper.go

+	"github.com/smartcontractkit/chainlink/v2/core/services/keystore/keys/ethkey"
+)
+
+type serviceWrapper struct {


I realized that the Wrapper also needs to have a getter for the underlying Connector object (we need to extract it once it's created). Please add it and also make ServiceWrapper public (uppercase) so we can pass it around in application.go.

justinkaseman · 2024-09-10T21:42:38Z

core/capabilities/gateway_connector/service_wrapper.go

+			return err
+		}
+		translated := translateConfigs(conf)
+		e.connector, err = connector.NewGatewayConnector(&translated, signer, clockwork.NewRealClock(), e.lggr)


Instead of using a real clock here, we may want to use a parameterized clock, such as keeping one on ServiceWrapper. This way we can pass a fake clock for testing.

@bolekk your thoughts?

makes sense to me

justinkaseman · 2024-09-10T21:43:56Z

core/capabilities/gateway_connector/service_wrapper.go

+
+var _ connector.Signer = &connectorSigner{}
+
+func NewConnectorSigner(config config.GatewayConnector, signerKey *ecdsa.PrivateKey, lggr logger.Logger) (*connectorSigner, error) {


Does it make sense for this to live in its own file?

Bolek had asked for this func to be moved into here fwiw.

I don't care too much. It's ultra-small, can be anywhere you guys prefer.

I'm now realizing that Signer doesn't need Start()/Close() so you can remove those to further simplify.
Same for SetConnector().

Honestly maybe even go a step further and make ServiceWrapper just implement connector.Signer interface :)

I also don't have a strong opinion here, reads slightly cleaner to me being separate, but I'll leave it up to your opinion David.

Honestly maybe even go a step further and make ServiceWrapper just implement connector.Signer interface :)

+1 for this, would be a lot cleaner

justinkaseman · 2024-09-10T21:46:56Z

core/capabilities/gateway_connector/service_wrapper.go

+	"github.com/smartcontractkit/chainlink/v2/core/config"
+	"github.com/smartcontractkit/chainlink/v2/core/logger"
+	"github.com/smartcontractkit/chainlink/v2/core/services/gateway/api"
+	gwCommon "github.com/smartcontractkit/chainlink/v2/core/services/gateway/common"


nit: stick to go conventional package naming
https://go.dev/blog/package-names

justinkaseman · 2024-09-10T21:53:02Z

core/capabilities/gateway_connector/service_wrapper.go

+			return errors.New("node address mismatch")
+		}
+
+		signer, err := NewConnectorSigner(e.config, signerKey, e.lggr)


nit:

Suggested change

signer, err := NewConnectorSigner(e.config, signerKey, e.lggr)

signer, err := NewConnectorSigner(conf, signerKey, e.lggr)

justinkaseman · 2024-09-10T22:02:55Z

core/capabilities/gateway_connector/service_wrapper.go

+	})
+}
+
+func (e *ServiceWrapper) Ready() error {


This is overwriting services.StateMachine's implementation right? Probably better to leave this as the default.

Oh nice, I didn't realize it has defaults.

justinkaseman · 2024-09-10T22:05:42Z

core/capabilities/gateway_connector/service_wrapper.go

+}
+
+func (e *ServiceWrapper) HealthReport() map[string]error {
+	return nil


To be thorough, how about filling this in from services.StateMachine?

Suggested change

return nil

return map[string]error{e.Name(): e.Healthy()}

bolekk · 2024-09-11T01:22:07Z

core/capabilities/gateway_connector/service_wrapper.go

+}
+
+// NOTE: this wrapper is needed to make sure that our services are started after Keystore.
+func NewGatewayConnectorServiceWrapper(config config.GatewayConnector, signerKey *ecdsa.PrivateKey, keystore keystore.Eth, clock clockwork.Clock, lggr logger.Logger) *ServiceWrapper {


We can't have signerKey as a parameter to the constructor. We extract it from Keystore inside Start(). The caller of the constructor doesn't have it.

code changed per request.

cl-sonarqube-production · 2024-09-11T04:18:13Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
83.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

cedric-cordenier reviewed Sep 6, 2024

View reviewed changes

service wrapper

e951bdb

DavidOrchard force-pushed the feature/CAPPL-19-connector-service-3 branch from 80ba68a to e951bdb Compare September 6, 2024 18:03

bolekk reviewed Sep 6, 2024

View reviewed changes

PR updates: simplify connector into signer and consolidate files

d4ce0db

bolekk reviewed Sep 6, 2024

View reviewed changes

core/capabilities/gateway_connector/service_wrapper.go Outdated Show resolved Hide resolved

keep signer as handler with empty methods for now

7784141

DavidOrchard force-pushed the feature/CAPPL-19-connector-service-3 branch from fd6ae00 to 7784141 Compare September 6, 2024 20:02

lint

ac908dd

DavidOrchard marked this pull request as ready for review September 6, 2024 20:30

DavidOrchard requested a review from a team as a code owner September 6, 2024 20:30

DavidOrchard requested review from bolekk and removed request for a team September 6, 2024 20:30

bolekk requested changes Sep 6, 2024

View reviewed changes

DavidOrchard added 3 commits September 6, 2024 13:57

PR comments except for unit test

cb3088d

package lint

b5bafec

unit test v1

79d6e4c

bolekk requested changes Sep 9, 2024

View reviewed changes

DavidOrchard added 3 commits September 9, 2024 15:55

fix unit tests

a2d5ab6

Merge branch 'develop' into feature/CAPPL-19-connector-service-3

be130db

PR Review: fix config, separate tests

ef63be6

bolekk reviewed Sep 9, 2024

View reviewed changes

bolekk reviewed Sep 10, 2024

View reviewed changes

PR review: clean up pointers, service wrapper generation.

2b66be0

DavidOrchard force-pushed the feature/CAPPL-19-connector-service-3 branch from d787b7b to 2b66be0 Compare September 10, 2024 14:57

DavidOrchard added 2 commits September 10, 2024 10:29

fix mocks return args issue

6519107

add logs to figure out Sign issue

493f94c

DavidOrchard force-pushed the feature/CAPPL-19-connector-service-3 branch from 8c73f6b to 493f94c Compare September 10, 2024 18:34

fix keys issues

cfaf0ca

DavidOrchard dismissed bolekk’s stale review via 1cfa38f September 10, 2024 21:33

DavidOrchard requested review from bolekk and justinkaseman September 10, 2024 21:33

remove handlegatewaymessage

5ac44fa

justinkaseman reviewed Sep 10, 2024

View reviewed changes

change name to gwcommon

8f26d15

justinkaseman reviewed Sep 10, 2024

View reviewed changes

remove unused funcs and clean up var reuse

c3f5616

justinkaseman reviewed Sep 10, 2024

View reviewed changes

DavidOrchard added 3 commits September 10, 2024 15:11

add clock to servicewrapper

558f577

fix bug in configs

f2f8d9c

fix Ready and healthreport

e119169

DavidOrchard requested a review from justinkaseman September 10, 2024 22:23

DavidOrchard added 2 commits September 10, 2024 18:12

remove connectorSigner struct

d572c97

Merge branch 'develop' into feature/CAPPL-19-connector-service-3

5e0ae26

bolekk previously requested changes Sep 11, 2024

View reviewed changes

remove privateKey from constructor, keep initialization in Start

d4e0068

DavidOrchard requested a review from bolekk September 11, 2024 03:46

lint

e256b2f

bolekk approved these changes Sep 11, 2024

View reviewed changes

cedric-cordenier approved these changes Sep 11, 2024

View reviewed changes

bolekk added this pull request to the merge queue Sep 11, 2024

Merged via the queue into develop with commit 51cb378 Sep 11, 2024
136 of 137 checks passed

bolekk deleted the feature/CAPPL-19-connector-service-3 branch September 11, 2024 14:10


		var _ connector.Signer = &connectorSigner{}

		func NewConnectorSigner(config config.GatewayConnector, signerKey ecdsa.PrivateKey, lggr logger.Logger) (*connectorSigner, error) {


		var _ connector.Signer = &connectorSigner{}

		func NewConnectorSigner(config config.GatewayConnector, signerKey ecdsa.PrivateKey, lggr logger.Logger) (connectorSigner, error) {

	signer, err := NewConnectorSigner(e.config, signerKey, e.lggr)
	signer, err := NewConnectorSigner(conf, signerKey, e.lggr)

[CAPPL-19] Gateway Connector service wrapper #14356

[CAPPL-19] Gateway Connector service wrapper #14356

Conversation

DavidOrchard commented Sep 5, 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

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

DavidOrchard Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

bolekk Sep 9, 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

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

bolekk Sep 10, 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

Choose a reason for hiding this comment

justinkaseman Sep 10, 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

cl-sonarqube-production bot commented Sep 11, 2024

Quality Gate passed

DavidOrchard commented Sep 5, 2024 •

edited

Loading

DavidOrchard Sep 6, 2024 •

edited

Loading

bolekk Sep 9, 2024 •

edited

Loading

bolekk Sep 10, 2024 •

edited

Loading

justinkaseman Sep 10, 2024 •

edited

Loading