refactor!: refactor LoadOffchainClient arguments by jkongie · Pull Request #437 · smartcontractkit/chainlink-deployments-framework

jkongie · 2025-09-16T12:28:37Z

[BREAKING CHANGE] Refactors the offchain client arguments to remove unnecessary arguments. Converts some of the arguments into functional options with defaults.

Adds tests for the offchain client.

changeset-bot · 2025-09-16T12:28:41Z

🦋 Changeset detected

Latest commit: b238a07

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
chainlink-deployments-framework	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jkongie · 2025-09-16T12:30:56Z

engine/cld/legacy/cli/commands/jd.go

 	}

-	return offchain.LoadOffchainClient(ctx, domain, envKey, cfg, lggr, true)
+	return offchain.LoadOffchainClient(ctx, domain, cfg.Offchain.JobDistributor,


We do not allow skipping initializing JD here, otherwise the JD commands wouldn't work

jkongie · 2025-09-16T12:31:04Z

engine/cld/internal/credentials/credentials.go

@@ -1,4 +1,4 @@
-package internal
+package credentials


jkongie · 2025-09-17T04:11:06Z

engine/cld/offchain/offchain.go

+	}
+
+	if endpoints.WSRPC == "" || endpoints.GRPC == "" {
+		return nil, ErrEndpointsRequired


Instead of skipping as previously implemented, we return a sentinel error to let the caller decide how they wish to proceed. There are valid cases where you may want to fail entirely instead of skipping the client load. (See the JD commands case)

jkongie · 2025-09-17T04:11:41Z

engine/cld/offchain/offchain.go

+	)

-	if domain.Key() == enginedomain.MustGetDomain("keystone").Key() && endpoints.WSRPC == "" {
+	// TODO: Remove this domain specific check


We need to check if this legacy code is still actually needed and remove it in a future PR. This would allow us to clean up the function arguments even more.

jkongie · 2025-09-17T04:12:23Z

engine/cld/offchain/offchain.go

+		if loadCfg.tokenSource != nil {
+			oauth = loadCfg.tokenSource // Used for injecting a mock token source for testing.
+		} else {


I don't love this because it modifies the code to support testing, but not sure if there is a better way to handle these in unit tests

jkongie · 2025-09-17T04:12:47Z

engine/cld/offchain/offchain.go

+}
+
+// withOffchainProvider sets the offchain provider for the offchain client. Used for testing.
+func withOffchainProvider(provider foffchain.Provider) LoadOffchainClientOpt {


Private functional option because it is only used for testing

should this be in the go doc comment itself? Else in future when someone else reads it, they will ask why is this unexported?

jkongie · 2025-09-17T04:12:51Z

engine/cld/offchain/offchain.go

+func withOffchainProvider(provider foffchain.Provider) LoadOffchainClientOpt {
+	return func(c *loadConfig) {
+		c.provider = provider
+	}
+}
+
+// withTokenSource sets the oauth2 token source for the offchain client. Used for testing.
+func withTokenSource(tokenSource oauth2.TokenSource) LoadOffchainClientOpt {


Private functional option because it is only used for testing

jkongie · 2025-09-17T04:13:54Z

engine/cld/offchain/offchain.go

-
-			oauth = source
 		}
-		creds := credentials.GetCredsForEnv(env)


This is now provided as a functional option. The caller can decide which creds they want to use.

jkongie · 2025-09-17T04:16:56Z

engine/cld/environment/fork_test.go

-			if tt.expectPanic {
-				assert.Panics(t, func() {
-					_, err := LoadFork(t.Context(), tt.domain, tt.env, tt.blockNumbers, tt.options...)
-					require.NoError(t, err)
-				})
-
-				return
-			}
-


This was panicking because JD was initialized with nil and accessing it caused a panic.

Since we return an error instead we test for the error instead

graham-chainlink · 2025-09-17T05:00:23Z

.changeset/bright-taxes-stick.md

+LoadOffchainClient(ctx, domain, "testnet", config, logger, false)
+
+// New
+LoadOffchainClient(ctx, domain, config.Offchain.JobDistributor,
+    WithLogger(logger),
+    WithDryRun(true), // Note: inverted!


.mockery.yml

graham-chainlink · 2025-09-17T05:06:31Z

engine/cld/offchain/offchain.go

+// When true, the offchain client will perform read operations but no write operations.
+func WithDryRun(dryRun bool) LoadOffchainClientOpt {
+	return func(c *loadConfig) {
+		c.dryRun = dryRun
+	}
+}


i wonder if having the option alone should set it to true instead of accepting a boolean flag? Seems cleaner?

WithDryRun() // this means we want dry run

we never will use the WithDryRun(false) anyway since it is false by default

I used the boolean flag here because it made the interface cleaner and I didn't have to collect options in it's own slice.

If I removed it the argument, the caller would have to do

opts := []Opt if x == true { opts = append(opts, WithDryRun()) }

Instead with the boolean flag they can pass it in directly

Load(WithDryRun(x))

I think this is slightly cleaner, but can see an argument against it for a cleaner option.

engine/cld/environment/environment_test.go

graham-chainlink · 2025-09-17T05:14:22Z

engine/cld/offchain/offchain.go

+}
+
+// withOffchainProvider sets the offchain provider for the offchain client. Used for testing.
+func withOffchainProvider(provider foffchain.Provider) LoadOffchainClientOpt {


should this be in the go doc comment itself? Else in future when someone else reads it, they will ask why is this unexported?

[BREAKING CHANGE] Refactors the offchain client arguments to remove unnecessary arguments. Converts some of the arguments into functional options with defaults. Adds tests for the offchain client.

cl-sonarqube-production · 2025-09-17T06:18:18Z

Quality Gate failed

Failed conditions
73.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

@jkongie

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## chainlink-deployments-framework@0.49.0 ### Minor Changes - [#437](#437) [`2224427`](2224427) Thanks [@jkongie](https://github.com/jkongie)! - **[BREAKING]** Refactored `LoadOffchainClient` to use functional options ## Function Signature Changed **Before:** ```go func LoadOffchainClient(ctx, domain, env, config, logger, useRealBackends) ``` **After:** ```go func LoadOffchainClient(ctx, domain, cfg, ...opts) ``` ## Migration Required - `logger` → `WithLogger(logger)` option (optional, has default) - `useRealBackends` → `WithDryRun(!useRealBackends)` ⚠️ **inverted logic** - `env` → `WithCredentials(creds)` option (optional, defaults to TLS) - `config` → `config.Offchain.JobDistributor` **Example:** ```go // Old LoadOffchainClient(ctx, domain, "testnet", config, logger, false) // New LoadOffchainClient(ctx, domain, config.Offchain.JobDistributor, WithLogger(logger), WithDryRun(true), // Note: inverted! ) ``` - [#428](#428) [`e172683`](e172683) Thanks [@jkongie](https://github.com/jkongie)! - Adds a test engine runtime for executing changesets in unit/integration tests - [#443](#443) [`9e6bc1d`](9e6bc1d) Thanks [@graham-chainlink](https://github.com/graham-chainlink)! - feat: introduce template-input command for generating YAML input This commit introduces a new template-input command that generates YAML input templates from Go struct types for durable pipeline changesets. The command uses reflection to analyze changeset input types and produces well-formatted YAML templates with type comments to guide users in creating valid input files. - [#440](#440) [`7f1af5d`](7f1af5d) Thanks [@RodrigoAD](https://github.com/RodrigoAD)! - add support for sui in mcms commands --------- Co-authored-by: app-token-issuer-engops[bot] <144731339+app-token-issuer-engops[bot]@users.noreply.github.com>

jkongie commented Sep 16, 2025

View reviewed changes

jkongie force-pushed the CLD-633/refactor-load-offchain-client branch 4 times, most recently from 4de36f2 to cd4b723 Compare September 17, 2025 04:04

jkongie commented Sep 17, 2025

View reviewed changes

jkongie force-pushed the CLD-633/refactor-load-offchain-client branch from cd4b723 to d0d8004 Compare September 17, 2025 04:19

graham-chainlink reviewed Sep 17, 2025

View reviewed changes

jkongie marked this pull request as ready for review September 17, 2025 06:12

jkongie requested a review from a team as a code owner September 17, 2025 06:12

refactor!: refactor LoadOffchainClient arguments

b238a07

[BREAKING CHANGE] Refactors the offchain client arguments to remove unnecessary arguments. Converts some of the arguments into functional options with defaults. Adds tests for the offchain client.

jkongie force-pushed the CLD-633/refactor-load-offchain-client branch from d0d8004 to b238a07 Compare September 17, 2025 06:12

jkongie requested a review from graham-chainlink September 17, 2025 06:25

graham-chainlink approved these changes Sep 17, 2025

View reviewed changes

ajaskolski approved these changes Sep 17, 2025

View reviewed changes

bytesizedroll approved these changes Sep 17, 2025

View reviewed changes

jkongie added this pull request to the merge queue Sep 17, 2025

Merged via the queue into main with commit 2224427 Sep 17, 2025
13 of 14 checks passed

jkongie deleted the CLD-633/refactor-load-offchain-client branch September 17, 2025 08:36

app-token-issuer-engops bot mentioned this pull request Sep 17, 2025

Version Packages #442

Merged

Conversation

jkongie commented Sep 16, 2025

Uh oh!

changeset-bot bot commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cl-sonarqube-production bot commented Sep 17, 2025

Quality Gate failed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

changeset-bot bot commented Sep 16, 2025 •

edited

Loading