From 5c7e945da098bdb3421640bb418c37f802d55cc3 Mon Sep 17 00:00:00 2001 From: Makram Date: Wed, 20 Nov 2024 19:41:18 +0400 Subject: [PATCH] deployment/ccip: fix buggy test assertions (#15333) Tests have been underspecifying sequence numbers; we need to specify both source and dest for a sequence number to be fully identifiable. Fixing the helpers to force this by adding a SourceDestPair type that is used as the key to all maps. --- .../ccip/changeset/active_candidate_test.go | 7 ++-- .../ccip/changeset/initial_deploy_test.go | 7 ++-- deployment/ccip/test_assertions.go | 35 ++++++++++++++----- .../smoke/ccip_messaging_test.go | 7 ++-- integration-tests/smoke/ccip_rmn_test.go | 7 ++-- integration-tests/smoke/ccip_test.go | 19 +++++++--- integration-tests/smoke/ccip_usdc_test.go | 7 ++-- integration-tests/smoke/fee_boosting_test.go | 7 ++-- 8 files changed, 70 insertions(+), 26 deletions(-) diff --git a/deployment/ccip/changeset/active_candidate_test.go b/deployment/ccip/changeset/active_candidate_test.go index 50115389a28..b6a4d331e9e 100644 --- a/deployment/ccip/changeset/active_candidate_test.go +++ b/deployment/ccip/changeset/active_candidate_test.go @@ -36,7 +36,7 @@ func TestActiveCandidate(t *testing.T) { // Need to keep track of the block number for each chain so that event subscription can be done from that block. startBlocks := make(map[uint64]*uint64) // Send a message from each chain to every other chain. - expectedSeqNum := make(map[uint64]uint64) + expectedSeqNum := make(map[ccdeploy.SourceDestPair]uint64) for src := range e.Chains { for dest, destChain := range e.Chains { if src == dest { @@ -53,7 +53,10 @@ func TestActiveCandidate(t *testing.T) { FeeToken: common.HexToAddress("0x0"), ExtraArgs: nil, }) - expectedSeqNum[dest] = msgSentEvent.SequenceNumber + expectedSeqNum[ccdeploy.SourceDestPair{ + SourceChainSelector: src, + DestChainSelector: dest, + }] = msgSentEvent.SequenceNumber } } diff --git a/deployment/ccip/changeset/initial_deploy_test.go b/deployment/ccip/changeset/initial_deploy_test.go index a299dd4971f..7c64c0c3240 100644 --- a/deployment/ccip/changeset/initial_deploy_test.go +++ b/deployment/ccip/changeset/initial_deploy_test.go @@ -84,7 +84,7 @@ func TestInitialDeploy(t *testing.T) { // Need to keep track of the block number for each chain so that event subscription can be done from that block. startBlocks := make(map[uint64]*uint64) // Send a message from each chain to every other chain. - expectedSeqNum := make(map[uint64]uint64) + expectedSeqNum := make(map[ccdeploy.SourceDestPair]uint64) for src := range e.Chains { for dest, destChain := range e.Chains { @@ -102,7 +102,10 @@ func TestInitialDeploy(t *testing.T) { FeeToken: common.HexToAddress("0x0"), ExtraArgs: nil, }) - expectedSeqNum[dest] = msgSentEvent.SequenceNumber + expectedSeqNum[ccdeploy.SourceDestPair{ + SourceChainSelector: src, + DestChainSelector: dest, + }] = msgSentEvent.SequenceNumber } } diff --git a/deployment/ccip/test_assertions.go b/deployment/ccip/test_assertions.go index 0c15c8b95ed..ac328499e96 100644 --- a/deployment/ccip/test_assertions.go +++ b/deployment/ccip/test_assertions.go @@ -147,15 +147,24 @@ func ConfirmTokenPriceUpdated( return nil } +// SourceDestPair is represents a pair of source and destination chain selectors. +// Use this as a key in maps that need to identify sequence numbers, nonces, or +// other things that require identification. +type SourceDestPair struct { + SourceChainSelector uint64 + DestChainSelector uint64 +} + // ConfirmCommitForAllWithExpectedSeqNums waits for all chains in the environment to commit the given expectedSeqNums. -// expectedSeqNums is a map of destinationchain selector to expected sequence number +// expectedSeqNums is a map that maps a (source, dest) selector pair to the expected sequence number +// to confirm the commit for. // startBlocks is a map of destination chain selector to start block number to start watching from. // If startBlocks is nil, it will start watching from the latest block. func ConfirmCommitForAllWithExpectedSeqNums( t *testing.T, e deployment.Environment, state CCIPOnChainState, - expectedSeqNums map[uint64]uint64, + expectedSeqNums map[SourceDestPair]uint64, startBlocks map[uint64]*uint64, ) { var wg errgroup.Group @@ -172,7 +181,11 @@ func ConfirmCommitForAllWithExpectedSeqNums( startBlock = startBlocks[dstChain.Selector] } - if expectedSeqNums[dstChain.Selector] == 0 { + expectedSeqNum, ok := expectedSeqNums[SourceDestPair{ + SourceChainSelector: srcChain.Selector, + DestChainSelector: dstChain.Selector, + }] + if !ok || expectedSeqNum == 0 { return nil } @@ -183,8 +196,8 @@ func ConfirmCommitForAllWithExpectedSeqNums( state.Chains[dstChain.Selector].OffRamp, startBlock, ccipocr3.SeqNumRange{ - ccipocr3.SeqNum(expectedSeqNums[dstChain.Selector]), - ccipocr3.SeqNum(expectedSeqNums[dstChain.Selector]), + ccipocr3.SeqNum(expectedSeqNum), + ccipocr3.SeqNum(expectedSeqNum), }) }) } @@ -307,7 +320,7 @@ func ConfirmExecWithSeqNrForAll( t *testing.T, e deployment.Environment, state CCIPOnChainState, - expectedSeqNums map[uint64]uint64, + expectedSeqNums map[SourceDestPair]uint64, startBlocks map[uint64]*uint64, ) (executionStates map[uint64]int) { var ( @@ -328,7 +341,11 @@ func ConfirmExecWithSeqNrForAll( startBlock = startBlocks[dstChain.Selector] } - if expectedSeqNums[dstChain.Selector] == 0 { + expectedSeqNum, ok := expectedSeqNums[SourceDestPair{ + SourceChainSelector: srcChain.Selector, + DestChainSelector: dstChain.Selector, + }] + if !ok || expectedSeqNum == 0 { return nil } @@ -338,14 +355,14 @@ func ConfirmExecWithSeqNrForAll( dstChain, state.Chains[dstChain.Selector].OffRamp, startBlock, - expectedSeqNums[dstChain.Selector], + expectedSeqNum, ) if err != nil { return err } mx.Lock() - executionStates[expectedSeqNums[dstChain.Selector]] = executionState + executionStates[expectedSeqNum] = executionState mx.Unlock() return nil diff --git a/integration-tests/smoke/ccip_messaging_test.go b/integration-tests/smoke/ccip_messaging_test.go index 6bb34658e22..4c797946f54 100644 --- a/integration-tests/smoke/ccip_messaging_test.go +++ b/integration-tests/smoke/ccip_messaging_test.go @@ -317,8 +317,11 @@ func runMessagingTestCase( FeeToken: common.HexToAddress("0x0"), ExtraArgs: extraArgs, }) - expectedSeqNum := make(map[uint64]uint64) - expectedSeqNum[tc.destChain] = msgSentEvent.SequenceNumber + expectedSeqNum := make(map[ccdeploy.SourceDestPair]uint64) + expectedSeqNum[ccdeploy.SourceDestPair{ + SourceChainSelector: tc.sourceChain, + DestChainSelector: tc.destChain, + }] = msgSentEvent.SequenceNumber out.msgSentEvent = msgSentEvent // hack diff --git a/integration-tests/smoke/ccip_rmn_test.go b/integration-tests/smoke/ccip_rmn_test.go index 4f44caccb52..4f903e84caf 100644 --- a/integration-tests/smoke/ccip_rmn_test.go +++ b/integration-tests/smoke/ccip_rmn_test.go @@ -341,7 +341,7 @@ func runRmnTestCase(t *testing.T, tc rmnTestCase) { // Need to keep track of the block number for each chain so that event subscription can be done from that block. startBlocks := make(map[uint64]*uint64) - expectedSeqNum := make(map[uint64]uint64) + expectedSeqNum := make(map[ccipdeployment.SourceDestPair]uint64) for _, msg := range tc.messagesToSend { fromChain := chainSelectors[msg.fromChainIdx] toChain := chainSelectors[msg.toChainIdx] @@ -354,7 +354,10 @@ func runRmnTestCase(t *testing.T, tc rmnTestCase) { FeeToken: common.HexToAddress("0x0"), ExtraArgs: nil, }) - expectedSeqNum[toChain] = msgSentEvent.SequenceNumber + expectedSeqNum[ccipdeployment.SourceDestPair{ + SourceChainSelector: fromChain, + DestChainSelector: toChain, + }] = msgSentEvent.SequenceNumber t.Logf("Sent message from chain %d to chain %d with seqNum %d", fromChain, toChain, msgSentEvent.SequenceNumber) } diff --git a/integration-tests/smoke/ccip_test.go b/integration-tests/smoke/ccip_test.go index 36aed7d5baa..ed0093ffbb0 100644 --- a/integration-tests/smoke/ccip_test.go +++ b/integration-tests/smoke/ccip_test.go @@ -27,7 +27,7 @@ func TestInitialDeployOnLocal(t *testing.T) { // Need to keep track of the block number for each chain so that event subscription can be done from that block. startBlocks := make(map[uint64]*uint64) // Send a message from each chain to every other chain. - expectedSeqNum := make(map[uint64]uint64) + expectedSeqNum := make(map[ccdeploy.SourceDestPair]uint64) for src := range e.Chains { for dest, destChain := range e.Chains { if src == dest { @@ -44,7 +44,10 @@ func TestInitialDeployOnLocal(t *testing.T) { FeeToken: common.HexToAddress("0x0"), ExtraArgs: nil, }) - expectedSeqNum[dest] = msgSentEvent.SequenceNumber + expectedSeqNum[ccdeploy.SourceDestPair{ + SourceChainSelector: src, + DestChainSelector: dest, + }] = msgSentEvent.SequenceNumber } } @@ -90,7 +93,7 @@ func TestTokenTransfer(t *testing.T) { // Need to keep track of the block number for each chain so that event subscription can be done from that block. startBlocks := make(map[uint64]*uint64) // Send a message from each chain to every other chain. - expectedSeqNum := make(map[uint64]uint64) + expectedSeqNum := make(map[ccdeploy.SourceDestPair]uint64) twoCoins := new(big.Int).Mul(big.NewInt(1e18), big.NewInt(2)) tx, err := srcToken.Mint( @@ -154,7 +157,10 @@ func TestTokenTransfer(t *testing.T) { FeeToken: feeToken, ExtraArgs: nil, }) - expectedSeqNum[dest] = msgSentEvent.SequenceNumber + expectedSeqNum[ccdeploy.SourceDestPair{ + SourceChainSelector: src, + DestChainSelector: dest, + }] = msgSentEvent.SequenceNumber } else { msgSentEvent := ccdeploy.TestSendRequest(t, e, state, src, dest, false, router.ClientEVM2AnyMessage{ Receiver: receiver, @@ -163,7 +169,10 @@ func TestTokenTransfer(t *testing.T) { FeeToken: feeToken, ExtraArgs: nil, }) - expectedSeqNum[dest] = msgSentEvent.SequenceNumber + expectedSeqNum[ccdeploy.SourceDestPair{ + SourceChainSelector: src, + DestChainSelector: dest, + }] = msgSentEvent.SequenceNumber } } } diff --git a/integration-tests/smoke/ccip_usdc_test.go b/integration-tests/smoke/ccip_usdc_test.go index a183808f2f8..bc527925c16 100644 --- a/integration-tests/smoke/ccip_usdc_test.go +++ b/integration-tests/smoke/ccip_usdc_test.go @@ -213,7 +213,7 @@ func transferAndWaitForSuccess( data []byte, ) { startBlocks := make(map[uint64]*uint64) - expectedSeqNum := make(map[uint64]uint64) + expectedSeqNum := make(map[ccdeploy.SourceDestPair]uint64) latesthdr, err := env.Chains[destChain].Client.HeaderByNumber(testcontext.Get(t), nil) require.NoError(t, err) @@ -227,7 +227,10 @@ func transferAndWaitForSuccess( FeeToken: common.HexToAddress("0x0"), ExtraArgs: nil, }) - expectedSeqNum[destChain] = msgSentEvent.SequenceNumber + expectedSeqNum[ccdeploy.SourceDestPair{ + SourceChainSelector: sourceChain, + DestChainSelector: destChain, + }] = msgSentEvent.SequenceNumber // Wait for all commit reports to land. ccdeploy.ConfirmCommitForAllWithExpectedSeqNums(t, env, state, expectedSeqNum, startBlocks) diff --git a/integration-tests/smoke/fee_boosting_test.go b/integration-tests/smoke/fee_boosting_test.go index 0e0fb094016..9101fe04dc8 100644 --- a/integration-tests/smoke/fee_boosting_test.go +++ b/integration-tests/smoke/fee_boosting_test.go @@ -94,7 +94,7 @@ func runFeeboostTestCase(tc feeboostTestCase) { require.NoError(tc.t, ccdeploy.AddLane(tc.deployedEnv.Env, tc.onchainState, tc.sourceChain, tc.destChain, tc.initialPrices)) startBlocks := make(map[uint64]*uint64) - expectedSeqNum := make(map[uint64]uint64) + expectedSeqNum := make(map[ccdeploy.SourceDestPair]uint64) msgSentEvent := ccdeploy.TestSendRequest(tc.t, tc.deployedEnv.Env, tc.onchainState, tc.sourceChain, tc.destChain, false, router.ClientEVM2AnyMessage{ Receiver: common.LeftPadBytes(tc.onchainState.Chains[tc.destChain].Receiver.Address().Bytes(), 32), Data: []byte("message that needs fee boosting"), @@ -102,7 +102,10 @@ func runFeeboostTestCase(tc feeboostTestCase) { FeeToken: common.HexToAddress("0x0"), ExtraArgs: nil, }) - expectedSeqNum[tc.destChain] = msgSentEvent.SequenceNumber + expectedSeqNum[ccdeploy.SourceDestPair{ + SourceChainSelector: tc.sourceChain, + DestChainSelector: tc.destChain, + }] = msgSentEvent.SequenceNumber // hack time.Sleep(30 * time.Second)