Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context.Context to deployment.Environment. #15410

Merged
merged 1 commit into from
Nov 27, 2024
+22 −9
Merged

Add context.Context to deployment.Environment. #15410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion deployment/ccip/changeset/test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/ethereum/go-ethereum/core/types"
"github.com/pkg/errors"
"github.com/smartcontractkit/ccip-owner-contracts/pkg/gethwrappers"

"github.com/smartcontractkit/chainlink-ccip/pluginconfig"

commonconfig "github.com/smartcontractkit/chainlink-common/pkg/config"
Expand Down Expand Up @@ -200,7 +201,7 @@ func NewMemoryEnvironment(
require.NoError(t, node.App.Stop())
})
}
e := memory.NewMemoryEnvironmentFromChainsNodes(t, lggr, chains, nodes)
e := memory.NewMemoryEnvironmentFromChainsNodes(func() context.Context { return ctx }, lggr, chains, nodes)
envNodes, err := deployment.NodeInfo(e.NodeIDs, e.Offchain)
require.NoError(t, err)
e.ExistingAddresses = ab
Expand Down
3 changes: 3 additions & 0 deletions deployment/environment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ type Environment struct {
Chains map[uint64]Chain
NodeIDs []string
Offchain OffchainClient
GetContext func() context.Context
}

func NewEnvironment(
Expand All @@ -85,6 +86,7 @@ func NewEnvironment(
chains map[uint64]Chain,
nodeIDs []string,
offchain OffchainClient,
ctx func() context.Context,
) *Environment {
return &Environment{
Name: name,
Expand All @@ -93,6 +95,7 @@ func NewEnvironment(
Chains: chains,
NodeIDs: nodeIDs,
Offchain: offchain,
GetContext: ctx,
}
}

Expand Down
7 changes: 4 additions & 3 deletions deployment/environment/devenv/environment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ type EnvironmentConfig struct {
JDConfig JDConfig
}

func NewEnvironment(ctx context.Context, lggr logger.Logger, config EnvironmentConfig) (*deployment.Environment, *DON, error) {
func NewEnvironment(ctx func() context.Context, lggr logger.Logger, config EnvironmentConfig) (*deployment.Environment, *DON, error) {
chains, err := NewChains(lggr, config.Chains)
if err != nil {
return nil, nil, fmt.Errorf("failed to create chains: %w", err)
}
offChain, err := NewJDClient(ctx, config.JDConfig)
offChain, err := NewJDClient(ctx(), config.JDConfig)
if err != nil {
return nil, nil, fmt.Errorf("failed to create JD client: %w", err)
}
Expand All @@ -39,7 +39,7 @@ func NewEnvironment(ctx context.Context, lggr logger.Logger, config EnvironmentC
}
var nodeIDs []string
if jd.don != nil {
err = jd.don.CreateSupportedChains(ctx, config.Chains, *jd)
err = jd.don.CreateSupportedChains(ctx(), config.Chains, *jd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine as-is, but declaring one ctx := ctx() at the top of the func can be less error prone over time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but this is a more complicated case where the context is being used in the constructor rather than a method. This is often problematic because it leaves the user unable to control the two separately. I think it would be appropriate to pass a context normally to these calls that use it synchronously. But OFC that would be messy alongside the func() context.Context, so probably only along with https://github.com/smartcontractkit/chainlink/pull/15410/files#r1859485133

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in principle - my plan was to fix those constructors in a subsequent PR, where possible (or use the mentioned pattern). This was an enabling change for some other situations where contexts are being used, but naively in the middle of downstream code in this and chainlink-deployments, and I wanted the plumbing all the way through, to at least make sure we HAD cancellation, and had a way to avoid folks using weird lambdas (not like what you're suggesting, but as complete workarounds of the outer API, to plumb through things like the context, and certain secrets, etc.).

Now that I understand context better, I can do a cleanup of context more generally of code in /development and chainlink-deployments.

if err != nil {
return nil, nil, err
}
Expand All @@ -53,5 +53,6 @@ func NewEnvironment(ctx context.Context, lggr logger.Logger, config EnvironmentC
chains,
nodeIDs,
offChain,
ctx,
), jd.don, nil
}
9 changes: 7 additions & 2 deletions deployment/environment/memory/environment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

chainsel "github.com/smartcontractkit/chain-selectors"

"github.com/smartcontractkit/chainlink-common/pkg/utils/tests"
"github.com/smartcontractkit/chainlink/deployment"

"github.com/smartcontractkit/chainlink-common/pkg/logger"
Expand Down Expand Up @@ -110,10 +111,12 @@ func NewNodes(t *testing.T, logLevel zapcore.Level, chains map[uint64]deployment
return nodesByPeerID
}

func NewMemoryEnvironmentFromChainsNodes(t *testing.T,
func NewMemoryEnvironmentFromChainsNodes(
ctx func() context.Context,
lggr logger.Logger,
chains map[uint64]deployment.Chain,
nodes map[string]Node) deployment.Environment {
nodes map[string]Node,
) deployment.Environment {
var nodeIDs []string
for id := range nodes {
nodeIDs = append(nodeIDs, id)
Expand All @@ -125,6 +128,7 @@ func NewMemoryEnvironmentFromChainsNodes(t *testing.T,
chains,
nodeIDs, // Note these have the p2p_ prefix.
NewMemoryJobClient(nodes),
ctx,
)
}

Expand All @@ -143,5 +147,6 @@ func NewMemoryEnvironment(t *testing.T, lggr logger.Logger, logLevel zapcore.Lev
chains,
nodeIDs,
NewMemoryJobClient(nodes),
func() context.Context { return tests.Context(t) },
)
}
6 changes: 4 additions & 2 deletions deployment/keystone/deploy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keystone_test

import (
"context"
"encoding/json"
"fmt"
"os"
Expand All @@ -9,6 +10,7 @@ import (

"github.com/ethereum/go-ethereum/accounts/abi/bind"
chainsel "github.com/smartcontractkit/chain-selectors"

"github.com/smartcontractkit/chainlink-common/pkg/logger"
"github.com/smartcontractkit/chainlink-common/pkg/utils/tests"
"github.com/smartcontractkit/chainlink/deployment"
Expand All @@ -26,6 +28,7 @@ import (

func TestDeploy(t *testing.T) {
lggr := logger.Test(t)
ctx := tests.Context(t)

// sepolia; all nodes are on the this chain
sepoliaChainId := uint64(11155111)
Expand Down Expand Up @@ -100,7 +103,7 @@ func TestDeploy(t *testing.T) {
maps.Copy(allNodes, wfNodes)
maps.Copy(allNodes, cwNodes)
maps.Copy(allNodes, assetNodes)
env := memory.NewMemoryEnvironmentFromChainsNodes(t, lggr, allChains, allNodes)
env := memory.NewMemoryEnvironmentFromChainsNodes(func() context.Context { return ctx }, lggr, allChains, allNodes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When these constructors are called normally, is there always an appropriate context on hand to pass in already? If not (or even if so?), this signature could be simplified by making a new context internally and returning (or providing a method to invoke) cancel().

Suggested change
env := memory.NewMemoryEnvironmentFromChainsNodes(func() context.Context { return ctx }, lggr, allChains, allNodes)
env, cancel := memory.NewMemoryEnvironmentFromChainsNodes(lggr, allChains, allNodes)
t.Cleanup(cancel)

This is also compatible with cases where there is a context already:

Suggested change
env := memory.NewMemoryEnvironmentFromChainsNodes(func() context.Context { return ctx }, lggr, allChains, allNodes)
env := memory.NewMemoryEnvironmentFromChainsNodes(lggr, allChains, allNodes)
context.AfterFunc(ctx, env.Cancel)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a context in the other case - I plan to normalize them. I am trying to keep things somewhat stable. I have a lot of change to stack on this, and I'm trying to not change too much of the signatures in one PR. I want to do a pass through all of this. They were pulling a context off of testing.T internally, and this was intended to make them both work the same way, pending future fixes. I'm not trying to fix all the things in this PR.

This is a good pattern to keep in mind, however, and I'll look at it as an option for a followup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, that doesn't exactly simplify the signature, it just moves a parameter to a return type, so it's the same number of parameters (if you count return values as "out" params)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does simplify having to construct a lambda, but I have a cleanup there planned as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider the method variant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our discussion, I'll definitely look at the method variant, but for now, plumbing through the lambda is enough to unblock other work, and I'll make a point of revisiting this for a more mature signal handling once we're out of the crunch.


var ocr3Config = keystone.OracleConfigWithSecrets{
OracleConfig: keystone.OracleConfig{
Expand All @@ -109,7 +112,6 @@ func TestDeploy(t *testing.T) {
OCRSecrets: deployment.XXXGenerateTestOCRSecrets(),
}

ctx := tests.Context(t)
// explicitly deploy the contracts
cs, err := keystone.DeployContracts(lggr, &env, sepoliaChainSel)
require.NoError(t, err)
Expand Down
3 changes: 2 additions & 1 deletion integration-tests/testsetups/ccip/test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ccip

import (
"bytes"
"context"
"fmt"
"math/big"
"os"
Expand Down Expand Up @@ -123,7 +124,7 @@ func NewLocalDevEnvironment(
testEnv, cfg)
require.NoError(t, err)

e, don, err := devenv.NewEnvironment(ctx, lggr, *envConfig)
e, don, err := devenv.NewEnvironment(func() context.Context { return ctx }, lggr, *envConfig)
require.NoError(t, err)
require.NotNil(t, e)
e.ExistingAddresses = ab
Expand Down
Loading