[DPA-1367]: feat(deployment/mcms): update test helpers #16248
Conversation
graham-chainlink
requested review from
AnieeG,
kylesmartin and
a team
as code owners
graham-chainlink
requested review from
winder,
asoliman92,
makramkd,
dimkouv,
mateusz-sekara,
0xAustinWang and
0xnogo
and removed request for
a team
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
graham-chainlink
force-pushed
the
ggoh/DPA-1367/test-helpers
branch
from
9957c47 to
13d91cd
Compare
graham-chainlink
dismissed
ecPablo’s stale review
The base branch was changed.
|
|
||
| timelockAddress := mcmsState.Timelock.Address().Hex() | ||
|
|
||
| proposerAddressPerChain[uint64(chainID)] = mcmsState.ProposerMcm.Address().Hex() |
There was a problem hiding this comment.
this is confusing; why not use chainSelector instead of uint64(chainID)?
There was a problem hiding this comment.
Good catch, i was copy pasting from the original implementation. Fixed it!
|
|
||
| allBatches := []mcmstypes.BatchOperation{} | ||
| for chainSelector := range cfg.Transfers { | ||
| chainID := mcmstypes.ChainSelector(chainSelector) |
There was a problem hiding this comment.
[nit] I'm not sure we need this variable. And the name is misleading, as we typically use "chain id" to refer to EVM's chain ids...
There was a problem hiding this comment.
Thanks, i cleaned it up by inlining it since it was used in 1 place only
graham-chainlink
force-pushed
the
ggoh/DPA-1367/test-helpers
branch
from
13d91cd to
1591e75
Compare
Add equivalent test helpers which use the new MCMS library Also updated the changeset link transfer to use the new helpers as a way to test the new changes. JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1367 Co-authored-by: ajaskolski <[email protected]>
graham-chainlink
force-pushed
the
ggoh/DPA-1367/test-helpers
branch
from
1591e75 to
6b75737
Compare
| } | ||
|
|
||
| proposalutils.ExecuteMCMSProposalV2(t, e, p, sel) | ||
| proposalutils.ExecuteMCMSTimelockProposalV2(t, e, &prop, sel) |
There was a problem hiding this comment.
are you sure we should execute the timelock proposal here? I think we have to wait until the operation is ready, no? My understanding is this would be done through the CI.
There was a problem hiding this comment.
this is the test helper, so we use this helper in tests to execute the changeset immediately so we can verify the behaviour
There was a problem hiding this comment.
so the line proposalutils.ExecuteMCMSProposalV2 makes the timelock proposal ready for execution, then ExecuteMCMSTimelockProposalV2 executes it.
There was a problem hiding this comment.
I thought the line proposalutils.ExecuteMCMSProposalV2 would only schedule the batch.
Anyways, if you say that's correct I trust you.
There was a problem hiding this comment.
I think you are still correct, i had the same question before and this is what pablo said
The execute actually calls the timelock contracts scheduleBatch and we still need to call the timelocks execute function so the transfer actually goes on.
so through testing, what it shows is that proposalutils.ExecuteMCMSProposalV2 calls would schedule the batch , marking it as ready and then calling proposalutils.ExecuteMCMSProposalV2 would execute the changeset.
Hope this make sense!
|
Add equivalent test helpers which use the new MCMS library Also updated the changeset link transfer to use the new helpers as a way to test the new changes. JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1367 Co-authored-by: ajaskolski <[email protected]>
Add equivalent test helpers which use the new MCMS library
Also updated the changeset link transfer to use the new helpers as a way to test the new changes.
JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1367