Zksync testing #596
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ yarn-error.log* | |
|
|
||
| # deployments | ||
| deployments | ||
| deployments-zk | ||
| deployed_contracts.json | ||
| managingDAOTX.json | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,108 +1,96 @@ | ||
| # Deployment Checklist | ||
|
|
||
| This checklist is seen as a guide to deploy the contracts to a new chain. | ||
|
|
||
| ## Pre deployment | ||
|
|
||
| - [ ] Run `yarn` in the repository root to install the dependencies. | ||
| - [ ] Run `yarn build` in `packages/contracts` to make sure the contracts compile. | ||
| - [ ] Run `yarn test` in `packages/contracts` to make sure the contract tests succeed. | ||
|
|
||
| - To run the tests, edit `packages/contracts/.env` to contain: | ||
|
|
||
| ```env | ||
| HARDHAT_DAO_ENS_DOMAIN="testdao.eth" | ||
| HARDHAT_PLUGIN_ENS_DOMAIN="testpluginrepo.eth" | ||
|
|
||
| MANAGEMENT_DAO_SUBDOMAIN="management" | ||
| ``` | ||
|
|
||
| - [ ] Edit `packages/contracts/networks.json` and add your custom network to which you want to deploy to. | ||
|
|
||
| If contract verification is not available for your chain: | ||
|
|
||
| - Ensure that the `deploy` key for the new network looks exactly like: <br> | ||
| `"deploy": ["./deploy/new"]` | ||
|
|
||
| If contract verification is available: | ||
|
|
||
| - Define the `deploy` key like: <br> | ||
| `"deploy": ["./deploy/new", "./deploy/verification"]`. | ||
| - Define the `ETHERSCAN_KEY` variable for contract verification on the `.env` file. [Follow the Hardhat guide](https://hardhat.org/hardhat-runner/plugins/nomicfoundation-hardhat-verify) in this case. | ||
|
|
||
| - [ ] Define the settings of the ENS domain used by OSx. | ||
|
|
||
| - Define the following ENS names in the `packages/contracts/.env` file, by replacing `SEPOLIA` with the name of the network name you’re deploying to: | ||
|
|
||
| ```env | ||
| SEPOLIA_DAO_ENS_DOMAIN="my-dao.eth" | ||
| SEPOLIA_PLUGIN_ENS_DOMAIN="my-dao.eth" | ||
| ``` | ||
|
|
||
| - Ensure that domains end with a suffix like `.eth` | ||
|
|
||
| - If the target chain does not have an official ENS registry: | ||
| - A new, unofficial ENS registry will be deployed, along with a resolver | ||
| - No ownership is needed, the Managing DAO will own them | ||
| - If the target chain does have an official ENS registry: | ||
| - Ensure that the wallet under `ETH_KEY` owns the domain | ||
| - If you created the domains via the ENS app, they will be owned by an ENS wrapper which would cause the script to fail | ||
| - [Open the ENS app](https://app.ens.domains/) and click `unwrap` for each of these domains. | ||
| - [Example](https://app.ens.domains/morpheusplugin3.eth?tab=more) | ||
|
|
||
| - [ ] If desired, update `MANAGEMENT_DAO_SUBDOMAIN` on `packages/contracts/.env`. | ||
| - In case you want the Managing DAO to use a different subdomain than the default one (`management`): | ||
| ```env | ||
| MANAGEMENT_DAO_SUBDOMAIN="root" # would be root.my-dao.eth | ||
| ``` | ||
| - [ ] Define the the deployment wallet's private key on the `.env` file | ||
| ```env | ||
| ETH_KEY="your-private-key-here" # without the 0x prefix | ||
| ``` | ||
| - [ ] Verify that the deployment wallet has sufficient funds to complete a protocol deployment. | ||
|
|
||
| ## Deployment | ||
|
|
||
| When the settings above are correctly set: | ||
|
|
||
| ```sh | ||
| cd packages/contracts # if needed | ||
| yarn deploy --network <NETWORK> # Replace with mainnet, polygon, sepolia, etc | ||
| ``` | ||
|
|
||
| ## Post deployment | ||
|
|
||
| - After the script has exited, the deployment wallet will be the only one with `ROOT_PERMISSION` on your Managing DAO. | ||
| - This allows the deployent wallet to manually install plugins on the Managing DAO. | ||
| - After the required plugins are installed, `ROOT_PERMISSION` has to be revoked on the deployment wallet. | ||
| - Should the script encounter any issues, the deployment should be re-run. | ||
| - The script will detect and re-use any previously deployed contracts. | ||
| - After the process completes, check out the `packages/contracts/deployed_contracts.json` file to see the deployed contract addresses. | ||
|
|
||
| ## Other | ||
|
|
||
| ### Rerunning the deployment script | ||
|
|
||
| If you need to restart the redeployment process and want Hardhat to not reuse the existing contracts: | ||
|
|
||
| ```sh | ||
| rm -R deployments/<network-name> # replace with the actual name | ||
| ``` | ||
|
|
||
| ### Running the deployment script on a testnet | ||
|
|
||
| If you want to simulate an L3 deployment within a testnet (sepolia) which has official ENS support, you may want to force the deployment of a new ENS Registry. | ||
|
|
||
| Edit the `packages/contracts/deploy/helpers.ts` file and comment out the relevant line from `ENS_ADDRESSES` and `ENS_PUBLIC_RESOLVERS`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,3 +10,4 @@ docs/osx/03-reference-guide | |
| #Hardhat files | ||
| cache | ||
| artifacts | ||
| .upgradable | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| # ZkSync | ||
|
|
||
| ZkSync uses a zkEVM and so has different requirements. | ||
|
|
||
| 1. Ensure contracts are compiled with the zkSolc compiler | ||
|
|
||
| - In your `hardhat.config.ts`, ensure the zkSync packages are uncommented, and the non-zkSync contracts that cause conflicts are commented out. | ||
|
|
||
| ```bash | ||
| # Build your contracts with zkSolc | ||
| yarn build --network zkLocalTestnet | ||
|
|
||
| # build with solc to ensure artifacts are present | ||
| yarn build | ||
| ``` | ||
|
|
||
| 2. Start a zkNode | ||
|
|
||
| ```bash | ||
| # Start a zkSync node in a second terminal | ||
| yarn node-zkSync | ||
| ``` | ||
|
|
||
| 3. Execute tests | ||
|
|
||
| ```bash | ||
| yard test --network zkLocalTestnet | ||
| ``` | ||
|
Comment on lines
+20
to
+28
Contributor
There was a problem hiding this comment. not severe: Couldn't we have something like
Contributor
There was a problem hiding this comment. They have an in-memory node actually but I never looked into it |
||
|
|
||
| You may need to configure tests to use custom matchers and wrappers. These are handled by wrapper and matcher files | ||
|
|
||
| TODO: add more details on how to configure tests for zkSync | ||
novaknole marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 1. Check that I correctly did skip tests as your suggestion/help. | ||
|
|
||
| - I've reviewed and made a few small changes but otherwise they look good. | ||
| - The tests log immediately, so have changed the language a bit | ||
|
|
||
| 2. In tokenVotingSetupZkSync contract, you will notice what I have done and why I got setUpgradePermissions and so on. If we can find a way to write this contract simpler, would be good. | ||
|
|
||
| - It looks sensible. I think we should do a thorough review in the morning when we both have fresh eyes. | ||
|
|
||
| - I think we need to find a way to revoke the UPGRADE permission inside the uninstallation script | ||
|
|
||
| - I removed the permissionIndex from the test. This should always resolve to the final permission in the array so we can just use the permissions array length: | ||
| - If we have a governance ERC20, then we lengthen the array to 4 | ||
| - We always add +1 to the permissions array if the upgrade permission is needed | ||
|
|
||
| 3. There are two tests that fail in dao.ts due to insufficient gas on zksync. Either skip it if you feel confident or on the weekends, I will also learn what the fuck they are. | ||
|
|
||
| - Yep we need to look at these gas tests. On Monday I wasted ages on these and I'm honestly no closer to understanding wtf is going on. That being said I have said several times I think the gas logic of ZkSync doesn't make this a vulnerability. | ||
|
|
||
| 4. Take a look at GovernanceERC20Upgradeable and GovernanceWrappedERC20Upgradeable. What struck me is that the contracts they inherit from(i.e GovernanceERC20 and GovernanceWrappedERC20), they don’t have gaps which means that if we in the future decide to add new variable in GovernanceERC20Upgradeable and then in GovernanceERC20, we will mess up the storage. But let’s go with how it is. I really don’t care since this case won’t happen, but I need you to exactly understand what I mean so you’re aware of that too. | ||
|
|
||
| - Good spot. So just reiterating: | ||
| - GovE20Up <- GovE20 (wrapped has similar chain) | ||
| - We can add new storage vars to the upgradeable version but not to the GovE20, because we have no gap, so it will start writing to storage slots inside GovERC20Up | ||
| - Would a workaround not be to first declare a `__gap` of reserved storage slots on GovERC20upgradeable THEN we add new storage variables (if needed) | ||
| - We would need to be careful this doesn't linearize unexpectedly and write to UUPSUpgradeable Storage. | ||
| - Why do you say "this case won't happen"? | ||
|
|
||
| 5. In hardhat-deploy script, we shouldn’t deploy Admin plugin if network is zksync. | ||
|
|
||
| - I added a skip function to the admin deploy steps, you can take a look. | ||
|
|
||
| 6. Play with running tests on zksync and hardhat to see how it looks and if I missed anything or not. We can deal with deployment in the next days. Maybe you come across that skipped message sometimes prints ‘f’. You can change it as you see fit. | ||
|
|
||
| - I removed the `f` tests | ||
| - I ran into some issues with matchers not firing, I brought these across into our testing lib | ||
| - Ran the whole test suite on 1.4.1, lgtm other than existing comments | ||
|
|
||
| 7. On hardhat-deploy script, we should deploy TokenVotingSetupZksync and also Governance contracts as upgradeable versions. | ||
|
|
||
| 8. Make psp upgradeable(create PSPZKSYNC), and deploy it on zksync network in hardhat-deploy script. Add some tests to test whether it can be upgraded. | ||
| 9. Maybe we should write tests to check whether my GovernanceERC20Upgradeable and GovernanceWrappedERC20Upgradeable can be upgraded. | ||
| 10. I skipped TokenFactory tests completely on all networks as we’re not using it at all(do we use it ?) | ||
|
|
||
| 11. To run tests on zksync, in hardhat.config.ts, you need to comment some stuff and to run tests on hardhat, you need to uncomment and comment other stuff. I asked about this and hardhat team said to use multiple configuration files. Just know that this is required to comment/uncomment. | ||
|
|
||
| - I made a quick attempt to add `hardhat.config-zk.ts` | ||
| - I then used this in `yarn hardhat --config hardhat.config-zk.ts test --network zkLocalTestnet` but I got some weird errors related to the wrapper not being instantiated | ||
| - We can come back to this, but I don't think it's essential just yet | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.