integration-tests/smoke: add batching test #15292
Conversation
|
I see you updated files related to |
makramkd
changed the title
makramkd
changed the title
makramkd
requested review from
winder,
rstout,
asoliman92,
dimkouv,
mateusz-sekara,
0xAustinWang and
0xnogo
deployment/ccip/deploy.go
Outdated
| } else { | ||
| e.Logger.Infow("router already deployed", "addr", chainState.Router.Address) | ||
| } | ||
| if mc3 == nil { |
There was a problem hiding this comment.
Do we need to deploy mc3 always? Or should it have some conditional input?
There was a problem hiding this comment.
What's the best way to condition it?
There was a problem hiding this comment.
We use DeployCCIPContractConfig to tell the deployer whether to deploy or not USDC-related contracts.
chainlink/deployment/ccip/deploy.go
Line 339 in 43f0ff1
Not sure if it fits your case. Another way would be to deploy multicall explicitly on demand (using a separate function)
ccdeploy.DeployMultiCall(chainId)bring down the numMessages to 5 again because exec is exceeding the max observation size
|
|
||
| i = 0 | ||
| var execStates []map[uint64]int | ||
| outer3: |
There was a problem hiding this comment.
for i < len(sourceChains) {
select {
case outputErr := <-execErrs:
require.NoError(t, outputErr.err)
execStates = append(execStates, outputErr.output)
i++
case <-ctx.Done():
require.FailNow(t, "didn't get all exec reports before test context was done")
}
}
There was a problem hiding this comment.
Actually that does work, just weirder :D
There was a problem hiding this comment.
I tested it before writing it :D. I know it works but why is it weirder? I think it's much clearer that way.
| wg.Wait() | ||
|
|
||
| var i int | ||
| outer: |
There was a problem hiding this comment.
This is duplicated 3 times, please add a function that takes the channel as an input
There was a problem hiding this comment.
I think having the function here is not as good, there's a ton of vars to take in. Will have to think about how to refactor this to be prettier later
There was a problem hiding this comment.
It's 3 variables
- testing.T
- errs channel
- sourceChains length
Much better IMO than current.
There was a problem hiding this comment.
The chan types are all different unfortunately, there is chan error, chan outputErr[CommitReport] and chan outputErr[map[...]]
deployment/ccip/changeset/deploy.go
Outdated
| } else { | ||
| e.Logger.Infow("router already deployed", "addr", chainState.Router.Address) | ||
| } | ||
| if mc3 == nil { |
There was a problem hiding this comment.
TODO need to update this based on deployOpts here https://github.com/smartcontractkit/chainlink/pull/15349/files
makramkd
requested review from
0xnogo,
AnieeG,
asoliman92,
0xAustinWang and
mateusz-sekara
Ticket: https://smartcontract-it.atlassian.net/browse/CCIP-4166
Context for CCIP onchain reviewers: We add the Multicall3 contract in tests/helpers so that we can use it to trigger multiple CCIP messages in a single tx in the test. Repo here: https://github.com/mds1/multicall
Requires
N/A
Supports
N/A