CCIP-4160 Token Transfer Tests #15278

0xsuryansh · 2024-11-18T05:19:15Z

CCIP Integration test covering the following scenarios

Send token to EOA
Send token to contract
Send 2 tokens to receiver
Send N tokens to contract

github-actions · 2024-11-18T05:35:53Z

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

mateusz-sekara

Good job, some minor comments

integration-tests/smoke/ccip_test.go

# Conflicts: # integration-tests/smoke/ccip_test.go

# Conflicts: # integration-tests/smoke/ccip/ccip_test.go

…t_token_transfer_tests

dimkouv

Mostly minor comments, approving. Feel free to resolve and re-request review if you agree with any.

deployment/ccip/changeset/test_helpers.go

dimkouv · 2024-11-28T16:19:08Z

deployment/ccip/changeset/test_helpers.go

+	expectedSeqNum := make(map[SourceDestPair]uint64)
+	expectedSeqNumExec := make(map[SourceDestPair][]uint64)
+
+	latesthdr, err := env.Chains[destChain].Client.HeaderByNumber(testcontext.Get(t), nil)


maybe better to pass ctx to this func?

dimkouv · 2024-11-28T16:20:03Z

deployment/ccip/changeset/test_helpers.go

+	require.NoError(t, err)
+
+	require.Eventually(t, func() bool {
+		actualBalance, err := tokenContract.BalanceOf(&bind.CallOpts{Context: tests.Context(t)}, receiver)


maybe pass ctx to this func? to avoid getting a new one everyime.

dimkouv · 2024-11-28T16:20:19Z

deployment/ccip/changeset/test_helpers.go

+	tokenContract, err := burn_mint_erc677.NewBurnMintERC677(token, chain.Client)
+	require.NoError(t, err)
+
+	balance, err := tokenContract.BalanceOf(&bind.CallOpts{Context: tests.Context(t)}, receiver)


similar for ctx

dimkouv · 2024-11-28T16:21:13Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+	inMemoryEnv := false
+
+	// use this if you are testing locally in memory
+	// tenv := changeset.NewMemoryEnvironmentWithJobsAndContracts(t, lggr, 2, 4, config)


would be better if we can do this using some env var. So we can easily switch in CI or run without touching code.

Yeah, we've covered that somewhere in the convo. There is a ticket to do it with env variable. Let's address it later

dimkouv · 2024-11-28T16:22:09Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+		selfServeDestTokenPoolDeployer,
+		state,
+		e.ExistingAddresses,
+		"SELF_SERVE_TOKEN",


dimkouv · 2024-11-28T16:22:36Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+
+	tinyOneCoin := new(big.Int).SetUint64(1)
+
+	// Test scenarios are defined here


comment probably not necessary, it's obvious

dimkouv · 2024-11-28T16:24:54Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+			tokenAmounts: []router.ClientEVMTokenAmount{
+				{
+					Token:  srcToken.Address(),
+					Amount: tinyOneCoin,


Name is a bit confusing for me tinyOneCoin maybe just one and oneE18, fiveE18 etc ?

Yeah, should be just oneCoin. I think it was copypasted from USDC which required always to pass exactly 1 because this value is hardcoded in the mocked contracts

dimkouv · 2024-11-28T16:25:46Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+			},
+			receiver: state.Chains[sourceChain].Receiver.Address(),
+			expectedTokenBalances: map[common.Address]*big.Int{
+				selfServeSrcToken.Address(): new(big.Int).SetUint64(2),


can set two to a variable to follow same pattern with tinyOneCoin

dimkouv · 2024-11-28T16:26:29Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+	require.NoError(t, err)
+
+	// Simulated backend sets chainID to 1337 always
+	chainID := big.NewInt(1337)


i believe it's similar for the commented out environment setup.

I don't think we should assume this chain ID here, these tests, in theory, can run on real chains.

This should be moved to test_setups. Right now we run it only on simulated or docker. If not simulated env is used we set proper chainID. I'm gonna work as followup to extract it somewhere

b-gopalswami · 2024-11-28T16:30:05Z

integration-tests/smoke/ccip/ccip_test.go

@@ -75,134 +73,3 @@ func TestInitialDeployOnLocal(t *testing.T) {

 	// TODO: Apply the proposal.
 }
-
-func TestTokenTransfer(t *testing.T) {


Nit: As we moved out this test, the parallel count here can be reduced to one. https://github.com/smartcontractkit/chainlink/pull/15278/files#diff-c97a5a0f8e46926381c7f53223fb048419a40e9c9b8e243b3f49e15bc2e00d9eL945

b-gopalswami · 2024-11-28T16:44:55Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+	for _, scenario := range scenarios {
+		t.Run(scenario.name, func(t *testing.T) {
+			initialBalances := map[common.Address]*big.Int{}
+			for token := range scenario.expectedTokenBalances {
+				initialBalance := changeset.GetTokenBalance(t, token, scenario.receiver, e.Chains[scenario.dstChain])
+				initialBalances[token] = initialBalance
+			}
+
+			changeset.TransferAndWaitForSuccess(
+				t,
+				e,
+				state,
+				scenario.srcChain,
+				scenario.dstChain,
+				scenario.tokenAmounts,
+				scenario.receiver,
+				scenario.data,
+				scenario.expectedExecutionState,
+			)
+
+			for token, balance := range scenario.expectedTokenBalances {
+				expected := new(big.Int).Add(initialBalances[token], balance)
+				changeset.WaitForTheTokenBalance(t, token, scenario.receiver, e.Chains[scenario.dstChain], expected)
+			}
+		})
+	}


This approach is quite sequential. I would recommend splitting this test into multiple smaller tests and enabling parallel execution.

A few options we could consider include:

Using multiple wallet addresses.

Breaking the test into separate segments (though this will require repeating the setup), and assigning a dedicated runner for each segment. This would reduce the overall execution time, as the message processing wait time can be parallelized across the different tests. With this approach, we could potentially achieve at least a 3x speedup.

I think we have to figure out a way to do parallel tests that doesn't mean we repeat a ton of test setup and no flakiness

makramkd · 2024-11-29T07:13:42Z

deployment/ccip/changeset/test_helpers.go

+}
+
+// TransferAndWaitForSuccess sends a message from sourceChain to destChain and waits for it to be executed
+func TransferAndWaitForSuccess(


I think we need a ticket to move all these funcs out of changeset and into a cciptesthelpers package or something, changeset is supposed to be the public API and shouldn't have these kinda funcs.

makramkd · 2024-11-29T07:13:59Z

deployment/ccip/changeset/test_helpers.go

+	require.Equal(t, expectedStatus, states[identifier][msgSentEvent.SequenceNumber])
+}
+
+func WaitForTheTokenBalance(


nit: WaitForTokenBalance without the The

makramkd · 2024-11-29T07:15:10Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+	for _, scenario := range scenarios {
+		t.Run(scenario.name, func(t *testing.T) {
+			initialBalances := map[common.Address]*big.Int{}
+			for token := range scenario.expectedTokenBalances {
+				initialBalance := changeset.GetTokenBalance(t, token, scenario.receiver, e.Chains[scenario.dstChain])
+				initialBalances[token] = initialBalance
+			}
+
+			changeset.TransferAndWaitForSuccess(
+				t,
+				e,
+				state,
+				scenario.srcChain,
+				scenario.dstChain,
+				scenario.tokenAmounts,
+				scenario.receiver,
+				scenario.data,
+				scenario.expectedExecutionState,
+			)
+
+			for token, balance := range scenario.expectedTokenBalances {
+				expected := new(big.Int).Add(initialBalances[token], balance)
+				changeset.WaitForTheTokenBalance(t, token, scenario.receiver, e.Chains[scenario.dstChain], expected)
+			}
+		})
+	}


I think we have to figure out a way to do parallel tests that doesn't mean we repeat a ton of test setup and no flakiness

makramkd · 2024-11-29T07:16:38Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+	require.NoError(t, err)
+
+	// Simulated backend sets chainID to 1337 always
+	chainID := big.NewInt(1337)


I don't think we should assume this chain ID here, these tests, in theory, can run on real chains.

…t_token_transfer_tests

0xsuryansh changed the title ~~CCIP-4160 test refactoring~~ CCIP-4160 Token Transfer Tests Nov 18, 2024

CCIP-4160 test refactoring

3bfac63

0xsuryansh force-pushed the CCIP-4160_Implement_token_transfer_tests branch from 99c3452 to 3bfac63 Compare November 19, 2024 11:33

0xsuryansh marked this pull request as ready for review November 19, 2024 11:35

0xsuryansh requested review from a team as code owners November 19, 2024 11:35

0xsuryansh requested review from winder, rstout, asoliman92, makramkd, dimkouv, mateusz-sekara and 0xAustinWang November 19, 2024 11:35

0xsuryansh added 3 commits November 19, 2024 18:21

resolving imports and using local dev env

4617621

comments fix

24dd4f6

comments fix

8bae382

mateusz-sekara reviewed Nov 19, 2024

View reviewed changes

integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved

integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved

integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved

integration-tests/smoke/ccip_test.go Outdated Show resolved Hide resolved

makramkd reviewed Nov 20, 2024

View reviewed changes

0xsuryansh and others added 11 commits November 20, 2024 20:52

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

b4a4a98

# Conflicts: # integration-tests/smoke/ccip_test.go

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

25c1b15

# Conflicts: # integration-tests/smoke/ccip_test.go

test update with common methods

94a2527

import fix for lint

9be1966

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

18f5f2e

# Conflicts: # integration-tests/smoke/ccip/ccip_test.go

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

7f8fd25

# Conflicts: # integration-tests/smoke/ccip/ccip_test.go

refactor: updated test with new setup

3d969d6

fix: lint (imports)

e2182dd

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

8273eac

Yolo

1bc99b3

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

e1a80af

# Conflicts: # integration-tests/smoke/ccip/ccip_test.go

mateusz-sekara requested a review from a team as a code owner November 28, 2024 12:32

mateusz-sekara added 7 commits November 28, 2024 13:41

Move tt tests to a separate file

947b68d

Fix

eedb8fa

Fix

8308df9

Merge remote-tracking branch 'origin/develop' into CCIP-4160_Implemen…

6b726d7

…t_token_transfer_tests

Fix

20a017e

Fix

97c74df

Fix

7f3fb4f

mateusz-sekara previously approved these changes Nov 28, 2024

View reviewed changes

mateusz-sekara requested a review from makramkd November 28, 2024 14:26

mateusz-sekara enabled auto-merge November 28, 2024 14:52

dimkouv previously approved these changes Nov 28, 2024

View reviewed changes

b-gopalswami reviewed Nov 28, 2024

View reviewed changes

0xnogo previously approved these changes Nov 29, 2024

View reviewed changes

makramkd reviewed Nov 29, 2024

View reviewed changes

mateusz-sekara disabled auto-merge November 29, 2024 07:33

Fix

b870f35

mateusz-sekara dismissed stale reviews from 0xnogo, dimkouv, and themself via b870f35 November 29, 2024 10:08

mateusz-sekara added 3 commits November 29, 2024 11:15

Fix

be9e925

Merge remote-tracking branch 'origin/develop' into CCIP-4160_Implemen…

c7d3987

…t_token_transfer_tests

Fix

d2fa64a

mateusz-sekara enabled auto-merge November 29, 2024 10:39

lukaszcl approved these changes Nov 29, 2024

View reviewed changes

mateusz-sekara approved these changes Nov 29, 2024

View reviewed changes

mateusz-sekara added this pull request to the merge queue Nov 29, 2024

Merged via the queue into develop with commit 5633382 Nov 29, 2024
174 of 175 checks passed

mateusz-sekara deleted the CCIP-4160_Implement_token_transfer_tests branch November 29, 2024 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCIP-4160 Token Transfer Tests #15278

CCIP-4160 Token Transfer Tests #15278

0xsuryansh commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

mateusz-sekara left a comment

dimkouv left a comment

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

mateusz-sekara Nov 29, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

mateusz-sekara Nov 29, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

makramkd Nov 29, 2024

mateusz-sekara Nov 29, 2024

b-gopalswami Nov 28, 2024

b-gopalswami Nov 28, 2024

makramkd Nov 29, 2024

makramkd Nov 29, 2024

makramkd Nov 29, 2024

makramkd Nov 29, 2024

makramkd Nov 29, 2024


		tinyOneCoin := new(big.Int).SetUint64(1)

		// Test scenarios are defined here

CCIP-4160 Token Transfer Tests #15278

CCIP-4160 Token Transfer Tests #15278

Conversation

0xsuryansh commented Nov 18, 2024 • edited Loading

github-actions bot commented Nov 18, 2024 • edited Loading

AER Report: CI Core ran successfully ✅

AER Report: Operator UI CI ran successfully ✅

mateusz-sekara left a comment

Choose a reason for hiding this comment

dimkouv left a 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

Choose a reason for hiding this comment

0xsuryansh commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading