migrate to tmpnet & latest subnetevm #401
Conversation
feuGeneA
changed the title
feuGeneA
force-pushed
the
tmpnet
branch
3 times, most recently
from
7f92e3d to
fb516a8
Compare
feuGeneA
changed the title
feuGeneA
force-pushed
the
tmpnet
branch
5 times, most recently
from
3877d11 to
3500bcc
Compare
feuGeneA
force-pushed
the
tmpnet
branch
6 times, most recently
from
ed4fb09 to
aa040f2
Compare
feuGeneA
force-pushed
the
tmpnet
branch
2 times, most recently
from
aa040f2 to
5072743
Compare
go.mod
Outdated
| @@ -3,14 +3,13 @@ module github.com/ava-labs/teleporter | |||
| go 1.21.12 | |||
|
|
|||
| require ( | |||
| github.com/ava-labs/avalanchego v1.11.1 | |||
| github.com/ava-labs/avalanchego v1.11.9-0.20240703222923-9d71967ccbba | |||
There was a problem hiding this comment.
9d719... refers to the latest commit hash (as of this writing) in this PR. We should change this to refer to a released version, once that PR has been merged and released.
There was a problem hiding this comment.
This is probably what's blocking the e2e tests running
There was a problem hiding this comment.
nvm. I was lacking context on the issue being around conflicting dependencies on avalanchego.
feuGeneA
requested review from
iansuvak,
geoff-vball,
minghinmatthewlam,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
| @@ -22,7 +22,7 @@ | |||
| "blockGasCostStep": 500000 | |||
| }, | |||
| "warpConfig": { | |||
| "blockTimestamp": 0 | |||
| "blockTimestamp": 1719343601 | |||
There was a problem hiding this comment.
why do we need to specify a specific warp blocktimestamp, as well as the timestamp value below?
There was a problem hiding this comment.
I was experiencing a hang in the test setup, and @ceyonur said "warp cannot be activated at timestamp = 0...also set genesis timestamp to some non-zero value (a recent timestamp is fine)...it was changed after v0.6.3 when we introduced modifiable network upgrades and fixing a regression with using 0 timestamps in genesis." starting from https://platwizards.slack.com/archives/C075HLQL4BT/p1719327426806459?thread_ts=1718898895.417379&cid=C075HLQL4BT
it looks like this is the original issue as reported: ava-labs/avalanchego#2877, which was fixed by ava-labs/subnet-evm#1136 and included in https://github.com/ava-labs/subnet-evm/releases/tag/v0.6.4
| @@ -62,6 +79,12 @@ var _ = ginkgo.BeforeSuite(func() { | |||
|
|
|||
| LocalNetworkInstance.DeployTeleporterRegistryContracts(teleporterContractAddress, fundedKey) | |||
| log.Info("Set up ginkgo before suite") | |||
|
|
|||
| ginkgo.AddReportEntry( | |||
| "network directory with has node logs & configs; useful in the case of failures", | |||
There was a problem hiding this comment.
| "network directory with has node logs & configs; useful in the case of failures", | |
| "network directory with node logs & configs; useful in the case of failures", |
There was a problem hiding this comment.
addressed in ec99c4d
| n.setSubnetValues(n.tmpnet.GetSubnet("A")) | ||
| n.setSubnetValues(n.tmpnet.GetSubnet("B")) |
There was a problem hiding this comment.
Not necessarily for this PR, but parameterizing LocalNetwork with the number of subnets and their names would make this type more flexible.
There was a problem hiding this comment.
addressed in 05b7a9b
|
|
||
| var replaced string = string(templateFileBytes[:]) | ||
| for _, s := range substitutions { | ||
| replaced = strings.Replace(replaced, s.Target, s.Value, 1) |
There was a problem hiding this comment.
It seems a little odd to generalize the target text replacement in this loop, but not the number of replacements. Since we're only replacing a single value in the genesis, I think it's appropriate to just explicitly call strings.Replace(replaces, "<EVM_CHAIN_ID>", strconv.FormatUint(chainID, 10)) here. Or we can go in the other direction and also generalize the number of replacements by either calling strings.ReplaceAll or putting n in the substitutions struct.
There was a problem hiding this comment.
I wrote it this way because I wanted the substitution(s) to be visible right at the beginning of the function, so that the reader doesn't have to dig into the implementation to find out what's being substituted.
I wasn't aware of ReplaceAll, that sounds like what I want. thanks!
addressed in aceb38b
| @@ -90,7 +90,7 @@ func NewLocalNetwork( | |||
| Expect(err).Should(BeNil()) | |||
| warpChainConfigPath := f.Name() | |||
|
|
|||
| allNodes := slices.Clone(extraNodes) // to be appended w/ subnet validators | |||
| allNodes := extraNodes // to be appended w/ subnet validators | |||
There was a problem hiding this comment.
nit: why this change? I understand that we don't need to clone if we won't re-access extraNodes but the nodes themselves are pointers already and therefore light. If for some reason we want to loop through just extraNodes in the future we would get the new expanded slice instead.
There was a problem hiding this comment.
the linter made me do it! :) it gave me some seemingly bogus type checking error that it couldn't infer the output type of slices.Clone, so I just did it a different way. I'm not 100% sure why the linter didn't catch this earlier.
later on in this code, we want to iterate over allNodes, which includes not just extraNodes but also all of the subnet nodes as well. this declaration is me starting to build up the list off all nodes, starting with the extraNodes.
There was a problem hiding this comment.
Ah yeah, I understand that practically it's not necessary here just a best practice to not have two slice variables pointing to the same underlying arrays. FWIW I think it would have been solved by declaring var allNodes []*tmpnet.Node beforehand.
addresses review comment #401 (comment)
addresses review comment #401 (comment)
addresses review comment #401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
(partially?) addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comments ava-labs/icm-contracts#401 (comment) and ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
addresses review comment ava-labs/icm-contracts#401 (comment)
Why this should be merged
Fixes #310
How this works
Stops using
avalanche-network-runner, and usestmpnetinstead.How this was tested
By getting the existing e2e tests in this repo to pass.
How is this documented