-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not rely on a hardcoded NitroAdjudicator address #121
Conversation
On every testground run: 1. Testground instance with sequence number 1 checks if NitroAdjudicator has been deployed. If not, the instance deploys the adjudicator. 2. All instances block until the adjudicator is deployed. Create2 is used for deploys, so a new deploy is only triggered when the creation code for NitroAdjudicator changes. Since NitroAdjudicator has no constructor, the creation code is the same as the bytecode. Note that the following are important: - Create2Deployer must exist at the hardcoded address. - The same nonce should be used for all deploys. The benefit of the above approach is that the testsuite will automatically use a deployed NitroAdjudicator contract version from the go-nitro package. Before this work, NitroAdjudicator version deployed on the chain could differ from the version supplied by the go-nitro package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I left one suggestion for an improvement that could be done pre or post merge if you are in agreement.
// One testground instance attempts to deploy NitroAdjudicator | ||
if seq == 1 { | ||
bytecode, err := client.CodeAt(ctx, naAddress, nil) // nil is latest block | ||
if err != nil { | ||
log.Fatal(err) | ||
} | ||
|
||
// Has NitroAdjudicator been deployed? If not, deploy it. | ||
if len(bytecode) == 0 { | ||
_, err = deployer.Deploy(txSubmitter, big.NewInt(0), [32]byte{}, hexBytecode) | ||
if err != nil { | ||
log.Fatal(err) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the deployment code should be in a method (cs *ChainService) DeployAdjudicator
, and then the "coordination" code (looking at the sequence number, calling out to the syncClient
) can be put into the actual test script?
That would avoid the need to change the API and have the chain service polluted with testground types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea. I am not sure I fully understand how to implement the idea.
The sequence of events in NewChainService
is:
- Initialize ethclient.Client client.
- Initialize bind.TransactOpts txSubmitter.
- Use client to compute NitroAdjudicator address.
- Use client and txSubmitter to deploy NitroAdjudicator if needed.
- Use coordination code to block all instances.
- Use client to set up a NitroAdjudicator object bound to the chain contract.
- Use client, txSubmitter, and bound NitroAdjudicator object to create a new chain service.
If below sequence is split up, we either need:
- A stateful object that contains the client and txSubmitter.
- Pass around client and txSubmitter.
Neither of the above is obviously cleaner to me than the existing code.
log.Fatal(err) | ||
} | ||
|
||
naAddress, err := deployer.ComputeAddress(&bind.CallOpts{}, [32]byte{}, crypto.Keccak256Hash(hexBytecode)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think statechannels/hardhat-docker#5 now just applies here. Not a priority, but we should probably deploy the consensus app and virtual payment app alongside the adjudicator here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Filed #122.
Merging the PR, but open to follow up changes! |
Fixes #95.
On every testground run:
Create2 is used for deploys, so a new deploy is only triggered when the creation code for NitroAdjudicator changes. Since NitroAdjudicator has no constructor, the creation code is the same as the bytecode.
Note that the following are important:
The benefit of the above approach is that the testsuite will automatically use a deployed NitroAdjudicator contract version from the go-nitro package. Before this work, NitroAdjudicator version deployed on the chain could differ from the version supplied by the go-nitro package.