Skip to content
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

Merged
merged 1 commit into from
Oct 5, 2022
Merged

Conversation

kerzhner
Copy link
Contributor

@kerzhner kerzhner commented Oct 4, 2022

Fixes #95.

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.

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.
Copy link
Contributor

@geoknee geoknee left a 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.

Comment on lines +54 to +68
// 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)
}
}
}
Copy link
Contributor

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.

Copy link
Contributor Author

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:

  1. Initialize ethclient.Client client.
  2. Initialize bind.TransactOpts txSubmitter.
  3. Use client to compute NitroAdjudicator address.
  4. Use client and txSubmitter to deploy NitroAdjudicator if needed.
  5. Use coordination code to block all instances.
  6. Use client to set up a NitroAdjudicator object bound to the chain contract.
  7. Use client, txSubmitter, and bound NitroAdjudicator object to create a new chain service.

If below sequence is split up, we either need:

  1. A stateful object that contains the client and txSubmitter.
  2. 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))
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Filed #122.

@kerzhner
Copy link
Contributor Author

kerzhner commented Oct 5, 2022

This looks good. I left one suggestion for an improvement that could be done pre or post merge if you are in agreement.

Merging the PR, but open to follow up changes!

@kerzhner kerzhner merged commit a0a5a20 into main Oct 5, 2022
@kerzhner kerzhner deleted the create2 branch October 5, 2022 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move contract deployment from hardhat-docker to go-nitro-testground
2 participants