⚽ Live Match: smartcontractkit/chainlink/pull/

DeividasK · 2025-07-01T15:27:46Z

This PR introduces a new VaultHandler implementation and updates to error handling.

TODOs

Propagate new handler error
Cleanup expired pending requests
Switch to using beholder over prometheus

…way-vault-flow-with-mocks

cedric-cordenier · 2025-07-09T16:46:38Z

+		cfg.RequestTimeoutSec = 30
+	}
+
+	userRateLimiter, _ := ratelimit.NewRateLimiter(cfg.UserRateLimiterConfig)


What's the second value that is being omitted here? (Not an error I hope 😬 ?)

It is error. we should handle it

cedric-cordenier · 2025-07-09T16:51:04Z

+	ErrNotAllowlisted = errors.New("sender not allowlisted")
+	ErrRateLimited    = errors.New("rate-limited")
+
+	promRequestInternalError = promauto.NewCounterVec(prometheus.CounterOpts{


We should use beholder rather than prometheus :)

cedric-cordenier · 2025-07-09T16:51:46Z

+
+	userRateLimiter   *ratelimit.RateLimiter
+	nodeRateLimiter   *ratelimit.RateLimiter
+	requestTimeoutSec int


Why not use a time.Duration here directly?

I don't see the timeout being used or enforced in our logic.
The gateway has a separate timeout for user request anyway.
If we want to use this timeout to just keep our pendingRequests queue bounded, we will need to implement a cleanup on timeout.
Currently the timeout is just used on a context when calling the don.SendToNode() method, but that doesn't affect our queue here.

Updated the timeout to affect the queue instead of just the don.SendToNode. This also effectively prunes the queue.

cedric-cordenier · 2025-07-09T16:53:29Z

+
+func NewHandler(methodConfig json.RawMessage, donConfig *config.DONConfig, don gw_handlers.DON, lggr logger.Logger) *handler {
+	var cfg Config
+	if err := json.Unmarshal(methodConfig, &cfg); err != nil {


@DeividasK A JSON unmarshal error should be a critical failure IMO; that way we catch it as soon as we deploy the job to the node.

Yes, just fail and propagate the error back.

cedric-cordenier · 2025-07-09T16:55:51Z

+		requestTimeoutSec: cfg.RequestTimeoutSec,
+		userRateLimiter:   userRateLimiter,
+		nodeRateLimiter:   nodeRateLimiter,
+		pendingRequests:   make(map[string]*pendingRequest),


Are we doing anything to ensure that this won't grow in an unbounded way? eg. expiring requests that haven't received a response in a long time

cedric-cordenier · 2025-07-09T16:57:35Z

+	ConnectorMethod = "vault"
+	HandlerName     = "VaultHandler"
+
+	promRequestInternalError = promauto.NewCounterVec(prometheus.CounterOpts{


Beholder :)

prashantkumar1982 · 2025-07-09T18:25:52Z

+
+func NewHandler(methodConfig json.RawMessage, donConfig *config.DONConfig, don gw_handlers.DON, lggr logger.Logger) *handler {
+	var cfg Config
+	if err := json.Unmarshal(methodConfig, &cfg); err != nil {


Yes, just fail and propagate the error back.

prashantkumar1982 · 2025-07-09T18:27:12Z

+		cfg.RequestTimeoutSec = 30
+	}
+
+	userRateLimiter, _ := ratelimit.NewRateLimiter(cfg.UserRateLimiterConfig)


It is error. we should handle it

prashantkumar1982 · 2025-07-09T18:30:19Z

+	return errors.New("vault service does not support legacy messages")
+}
+
+func (h *handler) HandleJSONRPCUserMessage(ctx context.Context, jsonRequest jsonrpc.Request[json.RawMessage], callbackCh chan<- gw_handlers.UserCallbackPayload) error {


nit optional:
you can define a type for this generic
type JsonRawMessageRequest = jsonrpc.Request[json.RawMessage]

And then use this type everywhere for readability.

Just helps readability in case you use it in a lot of places.

prashantkumar1982 · 2025-07-09T18:33:04Z

+
+	rawResponse, err := jsonrpc.EncodeResponse(resp)
+	if err != nil {
+		return h.sendResponse(ctx, h.errorResponse(pendingRequest.req, api.NodeReponseEncodingError, fmt.Errorf("failed to marshal response: %w", err)), pendingRequest.callbackCh)


instead of fmt.Errorf, use:
errors.New("failed to marshal response: " + err.Error()))

I think linter would already be complaining about it.

Linter seems to be fine with it 🤷‍♂️

prashantkumar1982 · 2025-07-09T18:39:25Z

+	}
+	defer delete(h.pendingRequests, resp.ID)
+
+	rawResponse, err := jsonrpc.EncodeResponse(resp)


The resp object could still contain an error inside right?
We have to propagate that error in that case, instead of sending api.NoError.

prashantkumar1982 · 2025-07-09T18:43:53Z

+		h.lggr.Infof("Processed response for request %s from node %s", resp.ID, nodeAddr)
+		return nil
+	case <-ctx.Done():
+		promRequestInternalError.WithLabelValues(h.donConfig.DonId, api.RequestTimeoutError.String()).Inc()


This failure necessarily may not be a timeout error.

prashantkumar1982 · 2025-07-09T18:57:42Z

+
+	userRateLimiter   *ratelimit.RateLimiter
+	nodeRateLimiter   *ratelimit.RateLimiter
+	requestTimeoutSec int


I don't see the timeout being used or enforced in our logic.
The gateway has a separate timeout for user request anyway.
If we want to use this timeout to just keep our pendingRequests queue bounded, we will need to implement a cleanup on timeout.
Currently the timeout is just used on a context when calling the don.SendToNode() method, but that doesn't affect our queue here.

github-actions · 2025-07-10T07:36:49Z

Flakeguard Summary

Ran new or updated tests between develop and 31cdbf2 (PRIV-96-e-2-e-gateway-vault-flow-with-mocks).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

4 Results

Name	Pass Ratio	Panicked?	Timed Out?	Race?	Runs	Failures	Package	Package Panicked?	Avg Duration	Code Owners
TestVaultHandler_HandleJSONRPCUserMessage/empty_params_error	0%	false	false	false	3	3	github.com/smartcontractkit/chainlink/v2/core/services/gateway/handlers/vault	false	0s	Unknown
TestVaultHandler_HandleJSONRPCUserMessage/happy_path	0%	false	false	false	3	3	github.com/smartcontractkit/chainlink/v2/core/services/gateway/handlers/vault	false	0s	Unknown
TestVaultHandler_HandleJSONRPCUserMessage/invalid_params_error	0%	false	false	false	3	3	github.com/smartcontractkit/chainlink/v2/core/services/gateway/handlers/vault	false	0s	Unknown
TestVaultHandler_HandleJSONRPCUserMessage/unsupported_method	0%	false	false	false	3	3	github.com/smartcontractkit/chainlink/v2/core/services/gateway/handlers/vault	false	0s	Unknown

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

github-actions · 2025-07-10T11:41:35Z

Flakeguard Summary

Ran new or updated tests between develop and 50b9b60 (PRIV-96-e-2-e-gateway-vault-flow-with-mocks).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

2 Results

Name	Pass Ratio	Panicked?	Timed Out?	Race?	Runs	Successes	Failures	Skips	Package	Package Panicked?	Avg Duration	Code Owners
TestVaultHandler_HandleJSONRPCUserMessage	0%	true	false	false	3	0	3	0	github.com/smartcontractkit/chainlink/v2/core/services/gateway/handlers/vault	true	0s	Unknown
TestVaultHandler_HandleJSONRPCUserMessage/happy_path	0%	true	false	false	3	0	3	0	github.com/smartcontractkit/chainlink/v2/core/services/gateway/handlers/vault	true	0s	Unknown

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

…way-vault-flow-with-mocks

cl-sonarqube-production · 2025-07-10T16:58:10Z

Quality Gate passed

Issues
0 New issues
1 Fixed issue
0 Accepted issues

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

See analysis details on SonarQube

cedric-cordenier · 2025-07-11T07:44:49Z

+func (h *handler) Start(ctx context.Context) error {
+	return h.StartOnce("VaultHandler", func() error {
+		h.lggr.Info("starting vault handler")
+		ctx, _ := h.stopCh.NewCtx()


You should call the cancel func by deferring it inside the go func on line 145

(And maybe calling <-ctx.Done() on line 151 for consistency

I'll address this in a follow up PR

DeividasK added 2 commits July 1, 2025 18:27

--wip-- [skip CI]

8ec4217

E2E flow with the request going to Vault and Vault sending a response

a1d6222

prashantkumar1982 reviewed Jul 4, 2025

View reviewed changes

DeividasK added 3 commits July 4, 2025 17:40

Remove comments

05cca45

Merge branch 'develop' into PRIV-96-e-2-e-gateway-vault-flow-with-mocks

741d52b

Fix running tests in CI + changeset

c80c729

DeividasK changed the title ~~Gateway Node => Vault Node implementation~~ Gateway Node proxy user requests to Vault Node Jul 8, 2025

DeividasK changed the title ~~Gateway Node proxy user requests to Vault Node~~ Gateway Node proxies user requests to Vault Node Jul 8, 2025

DeividasK added 11 commits July 8, 2025 18:32

Merge branch 'develop' into PRIV-96-e-2-e-gateway-vault-flow-with-mocks

41c120a

Fix prom counters

5a8f8dd

Fix vault_test lint errors

d7ced17

More lint fixes

e7fbd06

More cleanup

84e883c

Additional lint fixes

93fcb16

Merge branch 'develop' into PRIV-96-e-2-e-gateway-vault-flow-with-mocks

7675d17

Further refactor to HandleGatewayMessage

e45197c

Merge remote-tracking branch 'origin/develop' into PRIV-96-e-2-e-gate…

69614ac

…way-vault-flow-with-mocks

Update Vault service

0a84555

Refactoring + tests

b4437b5

cedric-cordenier reviewed Jul 9, 2025

View reviewed changes

prashantkumar1982 reviewed Jul 9, 2025

View reviewed changes

Timeout expired requests

50b9b60

Remove trailing comma

e85d634

DeividasK added 4 commits July 10, 2025 15:12

Merge remote-tracking branch 'origin/develop' into PRIV-96-e-2-e-gate…

7c7346c

…way-vault-flow-with-mocks

Fix tests

4af49b9

Fix lint issues

7a880f0

Use Beholder for metrics

61292e7

prashantkumar1982 previously approved these changes Jul 10, 2025

View reviewed changes

Comment thread core/services/gateway/handlers/vault/handler.go Resolved

Remove a leftover mutex

d027748

prashantkumar1982 approved these changes Jul 10, 2025

View reviewed changes

cedric-cordenier approved these changes Jul 11, 2025

View reviewed changes

cedric-cordenier reviewed Jul 11, 2025

View reviewed changes

cedric-cordenier approved these changes Jul 11, 2025

View reviewed changes

secure-michelemin closed this Jul 11, 2025

This was referenced Jul 11, 2025

[DO NOT MERGE] Changeset Release Preview - v2.74.0 #13148

Draft

[DO NOT MERGE] Changeset Release Preview - v2.68.0 mikeyhodl/chainlink#830

Draft

[DO NOT MERGE] Changeset Release Preview - v2.68.0 MarcelRaschke/chainlink#6

Draft

github-actions Bot mentioned this pull request Jul 25, 2025

[DO NOT MERGE] Changeset Release Preview - v2.29.0 JSTONE1111/chainlink#1

Merged

github-actions Bot mentioned this pull request Aug 21, 2025

[DO NOT MERGE] Changeset Release Preview - v2.27.0 philipjonsen/chainlink#2

Draft

Conversation

DeividasK commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODOs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cedric-cordenier Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 10, 2025

Flakeguard Summary

Found Flaky Tests ❌

Artifacts

Uh oh!

github-actions Bot commented Jul 10, 2025

Flakeguard Summary

Found Flaky Tests ❌

Artifacts

Uh oh!

Uh oh!

cl-sonarqube-production Bot commented Jul 10, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

DeividasK commented Jul 1, 2025 •

edited

Loading

cedric-cordenier Jul 9, 2025 •

edited

Loading