Skip to content

Commit

Permalink
[CCIP-2574] More Secure Wallet Approach (#1048)
Browse files Browse the repository at this point in the history
## Motivation

Using `NewWallet` led to some unknown and risky behaviors.

## Solution

We demand that a second wallet be already created and assume that the
test-runner will handle all funding needs for that wallet.

See related PRs
[1](smartcontractkit/chainlink-testing-framework#997)
and [2](smartcontractkit/chainlink#13614)
  • Loading branch information
kalverra authored Jun 18, 2024
1 parent e92a005 commit 9b669da
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 46 deletions.
26 changes: 0 additions & 26 deletions integration-tests/actions/actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ package actions
import (
"context"
"crypto/ecdsa"
"encoding/hex"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -379,31 +378,6 @@ func ReturnFunds(chainlinkNodes []*client.ChainlinkK8sClient, blockchainClient b
// If we fail to return funds from some addresses, we still want to try to return funds from the rest
encounteredErrors := []error{}

if len(blockchainClient.GetWallets()) > 1 {
if err := blockchainClient.SetDefaultWallet(0); err != nil {
encounteredErrors = append(encounteredErrors, err)
} else {
for walletIndex := 1; walletIndex < len(blockchainClient.GetWallets()); walletIndex++ {
decodedKey, err := hex.DecodeString(blockchainClient.GetWallets()[walletIndex].PrivateKey())
if err != nil {
encounteredErrors = append(encounteredErrors, err)
continue
}
privKey, err := crypto.ToECDSA(decodedKey)
if err != nil {
encounteredErrors = append(encounteredErrors, err)
continue
}

err = blockchainClient.ReturnFunds(privKey)
if err != nil {
encounteredErrors = append(encounteredErrors, err)
continue
}
}
}
}

for _, chainlinkNode := range chainlinkNodes {
fundedKeys, err := chainlinkNode.ExportEVMKeysForChain(blockchainClient.GetChainID().String())
if err != nil {
Expand Down
2 changes: 2 additions & 0 deletions integration-tests/ccip-tests/smoke/ccip_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,7 @@ func TestSmokeCCIPSelfServeRateLimitOnRamp(t *testing.T) {
require.GreaterOrEqual(t, len(src.Common.BridgeTokenPools), 2, "At least two bridge token pools needed for test")
require.GreaterOrEqual(t, len(dest.Common.BridgeTokens), 2, "At least two bridge tokens needed for test")
require.GreaterOrEqual(t, len(dest.Common.BridgeTokenPools), 2, "At least two bridge token pools needed for test")
require.NotEqualValues(t, src.Common.ChainClient.GetDefaultWallet().Address(), src.Common.BridgeTokens[0].OwnerAddress.Hex(), "Token owner and CCIP wallet should be different")
// add liquidity to pools on both networks
if !pointer.GetBool(TestCfg.TestGroupInput.ExistingDeployment) {
addFund := func(ccipCommon *actions.CCIPCommon) {
Expand Down Expand Up @@ -589,6 +590,7 @@ func TestSmokeCCIPSelfServeRateLimitOffRamp(t *testing.T) {
require.GreaterOrEqual(t, len(src.Common.BridgeTokenPools), 2, "At least two bridge token pools needed for test")
require.GreaterOrEqual(t, len(dest.Common.BridgeTokens), 2, "At least two bridge tokens needed for test")
require.GreaterOrEqual(t, len(dest.Common.BridgeTokenPools), 2, "At least two bridge token pools needed for test")
require.NotEqualValues(t, src.Common.ChainClient.GetDefaultWallet().Address(), src.Common.BridgeTokens[0].OwnerAddress.Hex(), "Token owner and CCIP wallet should be different")
// add liquidity to pools on both networks
if !pointer.GetBool(TestCfg.TestGroupInput.ExistingDeployment) {
addFund := func(ccipCommon *actions.CCIPCommon) {
Expand Down
44 changes: 30 additions & 14 deletions integration-tests/ccip-tests/testsetups/ccip.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ func (c *CCIPTestConfig) useExistingDeployment() bool {
return pointer.GetBool(c.TestGroupInput.ExistingDeployment)
}

func (c *CCIPTestConfig) useSeparateTokenDeployer() bool {
return contracts.NeedTokenAdminRegistry() &&
!pointer.GetBool(c.TestGroupInput.TokenConfig.CCIPOwnerTokens) &&
!c.useExistingDeployment()
}

func (c *CCIPTestConfig) MultiCallEnabled() bool {
return pointer.GetBool(c.TestGroupInput.MulticallInOneTx)
}
Expand Down Expand Up @@ -231,10 +237,13 @@ func (c *CCIPTestConfig) SetNetworkPairs(lggr zerolog.Logger) error {
}
name := fmt.Sprintf("private-chain-%d", len(c.SelectedNetworks)+1)
c.SelectedNetworks = append(c.SelectedNetworks, blockchain.EVMNetwork{
Name: name,
ChainID: chainID,
Simulated: true,
PrivateKeys: []string{networks.AdditionalSimulatedPvtKeys[i]},
Name: name,
ChainID: chainID,
Simulated: true,
PrivateKeys: []string{
networks.AdditionalSimulatedPvtKeys[i],
"ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80", // second key for token deployments
},
ChainlinkTransactionLimit: n.ChainlinkTransactionLimit,
Timeout: n.Timeout,
MinimumConfirmations: n.MinimumConfirmations,
Expand Down Expand Up @@ -351,7 +360,8 @@ func (c *CCIPTestConfig) SetOCRParams() error {
// This is useful for setting up test specific configurations.
type TestConfigOverrideOption func(*CCIPTestConfig) string

// WithCCIPOwnerTokens sets the number of tokens to be deployed and owned by the same account that owns all CCIP contracts
// WithCCIPOwnerTokens dictates that tokens be deployed and owned by the same account that owns all CCIP contracts.
// With Self-Serve tokens, this is unrealistic.
func WithCCIPOwnerTokens() TestConfigOverrideOption {
return func(c *CCIPTestConfig) string {
c.TestGroupInput.TokenConfig.CCIPOwnerTokens = pointer.ToBool(true)
Expand Down Expand Up @@ -900,19 +910,25 @@ func CCIPDefaultTestSetUp(
chainAddGrp, _ := errgroup.WithContext(setUpArgs.SetUpContext)
lggr.Info().Msg("Deploying common contracts")

// If we have a token admin registry, we need to create a new wallet to deploy our test tokens from so that the tokens
// If we have a token admin registry, we need to use a separate to deploy our test tokens from so that the tokens
// are not owned by the same account that owns the other CCIP contracts. This emulates self-serve token setups where
// the token owner is different from the CCIP contract owner.
if contracts.NeedTokenAdminRegistry() && !pointer.GetBool(testConfig.TestGroupInput.TokenConfig.CCIPOwnerTokens) {
if testConfig.useSeparateTokenDeployer() {
for _, net := range testConfig.AllNetworks {
chainClient := chainClientByChainID[net.ChainID]
if !pointer.GetBool(testConfig.TestGroupInput.ExistingDeployment) {
// TODO: This is a total guess at how much funds we need to deploy the tokens. This could be way off, especially on live chains.
// There aren't a lot of good ways to estimate this though. See CCIP-2471.
_, err = chainClient.NewWallet(big.NewFloat(0.1))
require.NoError(t, err, "failed to create new wallet to deploy tokens from")
err = chainClient.WaitForEvents()
require.NoError(t, err, "failed to wait for events after creating new wallet")
require.GreaterOrEqual(t, len(chainClient.GetWallets()), 2, "The test is using a TokenAdminRegistry, and has CCIPOwnerTokens set to 'false'. The test needs a second wallet to deploy token contracts from. Please add a second wallet to the 'evm_clients' config option.")
tokenDeployerWallet := chainClient.GetWallets()[1]
// TODO: This is a total guess at how much funds we need to deploy the tokens. This could be way off, especially on live chains.
// There aren't a lot of good ways to estimate this though. See CCIP-2471.
recommendedTokenBalance := new(big.Int).Mul(big.NewInt(5e18), big.NewInt(int64(pointer.GetInt(testConfig.TestGroupInput.TokenConfig.NoOfTokensPerChain))))
currentTokenBalance, err := chainClient.BalanceAt(context.Background(), common.HexToAddress(tokenDeployerWallet.Address()))
require.NoError(t, err)
if currentTokenBalance.Cmp(recommendedTokenBalance) < 0 {
lggr.Warn().
Str("Token Deployer Address", tokenDeployerWallet.Address()).
Uint64("Current Balance", currentTokenBalance.Uint64()).
Uint64("Recommended Balance", recommendedTokenBalance.Uint64()).
Msg("Token Deployer wallet may be underfunded. Please ensure it has enough funds to deploy the tokens.")
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ require (
github.com/smartcontractkit/chain-selectors v1.0.17
github.com/smartcontractkit/chainlink-automation v1.0.3
github.com/smartcontractkit/chainlink-common v0.1.7-0.20240607135320-c9bc0a2ac0ce
github.com/smartcontractkit/chainlink-testing-framework v1.30.8
github.com/smartcontractkit/chainlink-testing-framework v1.31.0
github.com/smartcontractkit/chainlink-vrf v0.0.0-20231120191722-fef03814f868
github.com/smartcontractkit/chainlink/integration-tests v0.0.0-00010101000000-000000000000
github.com/smartcontractkit/chainlink/v2 v2.0.0-00010101000000-000000000000
Expand Down
4 changes: 2 additions & 2 deletions integration-tests/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1520,8 +1520,8 @@ github.com/smartcontractkit/chainlink-solana v1.0.3-0.20240605170242-555ff582f36
github.com/smartcontractkit/chainlink-solana v1.0.3-0.20240605170242-555ff582f36a/go.mod h1:QqcZSwLgEIn7YraAIRmomnBMAuVFephiHrIWVlkWbFI=
github.com/smartcontractkit/chainlink-starknet/relayer v0.0.1-beta-test.0.20240531021326-99118e47f696 h1:h1E87+z+JcUEfvbJVF56SnZA/YUFE5ewUE61MaR/Ewg=
github.com/smartcontractkit/chainlink-starknet/relayer v0.0.1-beta-test.0.20240531021326-99118e47f696/go.mod h1:OiWUTrrpSLLTMh7FINWjEh6mmDJCVPaC4yEsDCVaWdU=
github.com/smartcontractkit/chainlink-testing-framework v1.30.8 h1:RcsNZBCJ7q7dI7tyttkSV7QPW2+fQMmFneFDlvc7ymE=
github.com/smartcontractkit/chainlink-testing-framework v1.30.8/go.mod h1:E6uNEZhZZid9PHv6/Kq5Vn63GlO61ZcKB+/f0DKo3Q4=
github.com/smartcontractkit/chainlink-testing-framework v1.31.0 h1:7xge1hsbIhvqIZmvXiqiRlvLFrZ271Tzg3Scj68Ru7w=
github.com/smartcontractkit/chainlink-testing-framework v1.31.0/go.mod h1:E6uNEZhZZid9PHv6/Kq5Vn63GlO61ZcKB+/f0DKo3Q4=
github.com/smartcontractkit/chainlink-testing-framework/grafana v0.0.0-20240328204215-ac91f55f1449 h1:fX/xmGm1GBsD1ZZnooNT+eWA0hiTAqFlHzOC5CY4dy8=
github.com/smartcontractkit/chainlink-testing-framework/grafana v0.0.0-20240328204215-ac91f55f1449/go.mod h1:DC8sQMyTlI/44UCTL8QWFwb0bYNoXCfjwCv2hMivYZU=
github.com/smartcontractkit/chainlink-vrf v0.0.0-20231120191722-fef03814f868 h1:FFdvEzlYwcuVHkdZ8YnZR/XomeMGbz5E2F2HZI3I3w8=
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/load/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ require (
github.com/slack-go/slack v0.12.2
github.com/smartcontractkit/chainlink-automation v1.0.3
github.com/smartcontractkit/chainlink-common v0.1.7-0.20240607135320-c9bc0a2ac0ce
github.com/smartcontractkit/chainlink-testing-framework v1.30.8
github.com/smartcontractkit/chainlink-testing-framework v1.31.0
github.com/smartcontractkit/chainlink/integration-tests v0.0.0-20240214231432-4ad5eb95178c
github.com/smartcontractkit/chainlink/v2 v2.9.0-beta0.0.20240216210048-da02459ddad8
github.com/smartcontractkit/libocr v0.0.0-20240419185742-fd3cab206b2c
Expand Down
4 changes: 2 additions & 2 deletions integration-tests/load/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1510,8 +1510,8 @@ github.com/smartcontractkit/chainlink-solana v1.0.3-0.20240605170242-555ff582f36
github.com/smartcontractkit/chainlink-solana v1.0.3-0.20240605170242-555ff582f36a/go.mod h1:QqcZSwLgEIn7YraAIRmomnBMAuVFephiHrIWVlkWbFI=
github.com/smartcontractkit/chainlink-starknet/relayer v0.0.1-beta-test.0.20240531021326-99118e47f696 h1:h1E87+z+JcUEfvbJVF56SnZA/YUFE5ewUE61MaR/Ewg=
github.com/smartcontractkit/chainlink-starknet/relayer v0.0.1-beta-test.0.20240531021326-99118e47f696/go.mod h1:OiWUTrrpSLLTMh7FINWjEh6mmDJCVPaC4yEsDCVaWdU=
github.com/smartcontractkit/chainlink-testing-framework v1.30.8 h1:RcsNZBCJ7q7dI7tyttkSV7QPW2+fQMmFneFDlvc7ymE=
github.com/smartcontractkit/chainlink-testing-framework v1.30.8/go.mod h1:E6uNEZhZZid9PHv6/Kq5Vn63GlO61ZcKB+/f0DKo3Q4=
github.com/smartcontractkit/chainlink-testing-framework v1.31.0 h1:7xge1hsbIhvqIZmvXiqiRlvLFrZ271Tzg3Scj68Ru7w=
github.com/smartcontractkit/chainlink-testing-framework v1.31.0/go.mod h1:E6uNEZhZZid9PHv6/Kq5Vn63GlO61ZcKB+/f0DKo3Q4=
github.com/smartcontractkit/chainlink-testing-framework/grafana v0.0.0-20240328204215-ac91f55f1449 h1:fX/xmGm1GBsD1ZZnooNT+eWA0hiTAqFlHzOC5CY4dy8=
github.com/smartcontractkit/chainlink-testing-framework/grafana v0.0.0-20240328204215-ac91f55f1449/go.mod h1:DC8sQMyTlI/44UCTL8QWFwb0bYNoXCfjwCv2hMivYZU=
github.com/smartcontractkit/chainlink-vrf v0.0.0-20231120191722-fef03814f868 h1:FFdvEzlYwcuVHkdZ8YnZR/XomeMGbz5E2F2HZI3I3w8=
Expand Down

0 comments on commit 9b669da

Please sign in to comment.