Add teleporter messenger to genesis #474
Conversation
…rter-messenger-to-genesis
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
michaelkaplan13
requested review from
iansuvak,
feuGeneA,
geoff-vball,
minghinmatthewlam,
bernard-avalabs and
cam-schultz
contracts/teleporter/README.md
Outdated
|
|
||
| **Do not deploy the `TeleporterMessenger` contract using `forge create`**. The `TeleporterMessenger` contract must be deployed to the same contract address on every chain. To achieve this, the contract can be deployed using a static transaction that uses Nick's method as documented in [this guide](https://github.com/ava-labs/teleporter/blob/main/utils/contract-deployment/README.md). Alternatively, if creating a new Subnet, the contract can be pre-allocated with the proper address and state in the new chain's [genesis file](https://docs.avax.network/build/subnet/upgrade/customize-a-subnet#setting-the-genesis-allocation). | ||
|
|
||
| As an example, to include `TeleporterMessenger` `v1.0.0` in the genesis file, include the following values in the `alloc` settings, as documented at the link above. |
There was a problem hiding this comment.
It would be helpful to explain each of these fields.
There was a problem hiding this comment.
Happy to add them, but found that the descriptions of balance/code/nonce/storage were all pretty trivial since they are relatively self-describing for an address in the context of the EVM. Thoughts?
There was a problem hiding this comment.
Of those, the only nontrivial one is storage, since it's not obvious that this refers to the ReentrancyGuards
There was a problem hiding this comment.
Ah...yeah good call out, thanks. Added an explanation now 👍
| @@ -40,51 +39,61 @@ func TestE2E(t *testing.T) { | |||
|
|
|||
| // Define the Teleporter before and after suite functions. | |||
| var _ = ginkgo.BeforeSuite(func() { | |||
| // Generate the Teleporter deployment values | |||
There was a problem hiding this comment.
It might be helpful in some debugging scenarios to provide an easy way to deploy Teleporter to all subnets via eth_sendRawTransaction versus genesis. Some ideas that come to mind:
- An explicit command line flag. I'm not sure if being able to make this switch at runtime is worth the added complexity.
- Add a
boolparamdeployViaGenesistoNewLocalNetwork. Then the only changes required aretrue->falseandDeployTeleporterContractToCChain-> DeployTeleporterContractToAllSubnets`- Can be further simplified if
DeployTeleporterContractToAllSubnetscan be made idempotent so that it can be called in all scenarios.
- Can be further simplified if
There was a problem hiding this comment.
What are the benefits of having those two different options for the E2E test execution that you were thinking of?
For local E2E tests, I think this covers both deployment flows sufficiently.
There was a problem hiding this comment.
I was just anticipating any issues arising from the different deployment methods. In such a scenario, being able to toggle between the two could be helpful. For instance, if a future change comes with state initialization in the same way as ReentrancyGuards, switching to Nick's method would help narrow down any differences.
There was a problem hiding this comment.
I think we can punt on this for now personally. Have the default E2E cover both possible deployment flows should hopefully ensure we keep them both up-to-date and compatible with any future changes.
There was a problem hiding this comment.
Sounds good. Any test failures along these lines would require root cause analysis anyway, something the suggested change would likely not help much with.
| ) | ||
| LocalNetworkInstance.SetTeleporterContractAddress(teleporterContractAddress) |
There was a problem hiding this comment.
Previously this was encapsulated in the call to DeployTeleporterContracts. Can we similarly do so in DeployTeleporterContractToCChain and/or NewLocalNetwork?
There was a problem hiding this comment.
I think it was a little odd previously actually. There was a boolean parameter in one exported function for whether or not another exported function should be called from it. Since they are both exported, I think it's cleaner to make them independent and left up to the caller.
There was a problem hiding this comment.
Ah, I had forgotten about that switch. Agreed, makes sense to keep them separate.
Co-authored-by: cam-schultz <[email protected]> Signed-off-by: Michael Kaplan <[email protected]>
…labs/teleporter into add-teleporter-messenger-to-genesis
Why this should be merged
Fixes #464
How this works
TeleporterMessengercontract is directly inserted into theallocsection of the two Subnet chains genesis files. The contract is still deployed to the C-Chain using the Nick's method transaction.