core icon indicating copy to clipboard operation
core copied to clipboard

Published 20 hours ago • verified publisher icon aragon

Zksync testing

Open novaknole opened this issue 1 year ago • 7 comments

Description

Please include a summary of the change and be sure you follow the contributions rules we do provide here

Task ID: OS-?

  • Created TokenVotingSetupZkSync.sol which will be deployed and used for TokenVoting Plugin creations. This was needed as in the TokenVotingSetup.sol, we were deploying plugins with clones which is not supported on ZkSync for not-so-important reasons for reviewers.
  • Created PluginSetupProcessorUpgradeable.sol to allow upgradeable psp. This will be deployed only on zksync to avoid compiler bugs in case it happens.
  • Created MockedHelper contract in /test to help with the tests of PSP. This was needed as psp was written with smocks. As it's not supported on zksync, MockedHelper seemed the quickest option.
  • Created ProxyFactory and ProxyLib(or more precisely, copied it from develop branch by Michael) to help ease the deployments.
  • Created Javascript classes such as wrapper, hardhat, zksync inside /test/test-utils/wrapper. This was necessary to avoid rewriting the same tests. The Wrapper class is used to instantiate the appropriate network class to distinguish how deployments must be done. There're a couple of more functions into it that are different between zksync/hardat. The wrapper class is instantiated inside hardhat.config.ts.
  • Changed hardhat.config.ts to allow compiling contracts with zksync or hardhat both.
  • tests that were using evm_setAutomine has been skipped only on zksync network as this feature is not supported on there.
  • Changed deploy scripts such as depending on network, it decides whether to deploy PluginSetupProcessor or its upgradeable version. Same for TokenVotingSetupZkSync or TokenVotingSetup.
  • Contracts are deployed with latest versions, but we're not importing artifacts to help with deployment. This is because, typechain doesn't generate the correct files for zksync(it does, but the only thing it doesn't contain is artifacts), hence deployment with typechains is not possible. I am passing the artifact's names as strings. This causes that if 2 contracts with the same name exists, it can't decide which one to deploy. So, in the Wrapper class, we got string names and importing them.

Type of change

See the framework lifecycle in packages/contracts/docs/framework-lifecycle to decide what kind of change this pull request is.

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • [ ] I have selected the correct base branch.
  • [ ] I have performed a self-review of my own code.
  • [ ] I have commented my code, particularly in hard-to-understand areas.
  • [ ] My changes generate no new warnings.
  • [ ] Any dependent changes have been merged and published in downstream modules.
  • [ ] I ran all tests with success and extended them if necessary.
  • [ ] I have updated the CHANGELOG.md file in the root folder.
  • [ ] I have updated the DEPLOYMENT_CHECKLIST file in the root folder.
  • [ ] I have updated the UPDATE_CHECKLIST file in the root folder.
  • [ ] I have updated the Subgraph and added a QA URL to the description of this PR.

novaknole avatar May 15 '24 09:05 novaknole

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

:white_check_mark: heueristik
:white_check_mark: Rekard0
:white_check_mark: novaknole
:x: Giorgi Lagidze

Giorgi Lagidze seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar May 15 '24 09:05 CLAassistant

Can we make the tests pass on the runner? I am getting 13 errors locally:

Bildschirmfoto 2024-05-15 um 15 29 10

after installing with yarn install --frozen-lockfile and after removing an .only.

heueristik avatar May 15 '24 13:05 heueristik

Is something speaking against maintaining the CHANGELOG.md?

heueristik avatar May 17 '24 09:05 heueristik

Is something speaking against maintaining the CHANGELOG.md?

@Michael-A-Heuer This PR doesn't get merged. Please focus on other stuff and resolve comments.

novaknole avatar May 17 '24 09:05 novaknole

Is something speaking against maintaining the CHANGELOG.md?

@Michael-A-Heuer This PR doesn't get merged. Please focus on other stuff and resolve comments.

IMO, keeping a changelog still makes sense even if we don't merge it (in case there is a problem and we quickly want to see the differences).

heueristik avatar May 17 '24 09:05 heueristik

Is something speaking against maintaining the CHANGELOG.md?

@Michael-A-Heuer This PR doesn't get merged. Please focus on other stuff and resolve comments.

IMO, keeping a changelog still makes sense even if we don't merge it (in case there is a problem and we quickly want to see the differences).

Again, I repeat: changes will in the end be done on the develop branch and on there, all the plugins are removed already. So you want me to write changelog and then do it again on develop branch ? Doesn't make sense. -_-

novaknole avatar May 17 '24 09:05 novaknole

Update on https://github.com/aragon/osx/pull/596#pullrequestreview-2063027335:

After explicitly setting node-zksyn back to {"latest":"0.1.0-alpha.19"}, the tests now produce different errors:

  851 passing (9m)
  53 pending
  3 failing

  1) DAO
       execute:
         reverts if failure is allowed but not enough gas is provided (many actions):
     AssertionError: Expected transaction to be reverted with custom error 'InsufficientGas', but it didn't revert
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at Context.<anonymous> (test/core/dao/dao.ts:645:7)

  2) DAO
       execute:
         reverts if failure is allowed but not enough gas is provided (one action):
     AssertionError: Expected transaction to be reverted with custom error 'InsufficientGas', but it reverted with a different custom error
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at Context.<anonymous> (test/core/dao/dao.ts:682:7)

  3) Managing DAO
       "before all" hook for "should have deployments":
     ERROR processing /Users/michaelheuer/Projects/Aragon/osx/packages/contracts/deploy/new/10_framework/00_ens_registry.ts:
Error: DAO or Plugin ENS domains have not been set in .env
    at Object.func (/Users/michaelheuer/Projects/Aragon/osx/packages/contracts/deploy/new/10_framework/00_ens_registry.ts:21:11)
    at DeploymentsManager.executeDeployScripts (/Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1212:41)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at DeploymentsManager.runDeploy (/Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1061:5)
    at Object.fixture (/Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:316:9)
    at /Users/michaelheuer/Projects/Aragon/osx/packages/contracts/test/test-utils/fixture.ts:34:5
    at /Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:341:24
    at initializeDeploymentFixture (/Users/michaelheuer/Projects/Aragon/osx/packages/contracts/test/test-utils/fixture.ts:37:3)
    at deployAll (/Users/michaelheuer/Projects/Aragon/osx/packages/contracts/test/deploy/managing-dao.ts:29:3)
    at Context.<anonymous> (/Users/michaelheuer/Projects/Aragon/osx/packages/contracts/test/deploy/managing-dao.ts:78:5)
  Error: ERROR processing /Users/michaelheuer/Projects/Aragon/osx/packages/contracts/deploy/new/10_framework/00_ens_registry.ts:
  Error: DAO or Plugin ENS domains have not been set in .env
      at Object.func (deploy/new/10_framework/00_ens_registry.ts:21:11)
      at DeploymentsManager.executeDeployScripts (/Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1212:41)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at DeploymentsManager.runDeploy (/Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1061:5)
      at Object.fixture (/Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:316:9)
      at /Users/michaelheuer/Projects/Aragon/osx/packages/contracts/test/test-utils/fixture.ts:34:5
      at /Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:341:24
      at initializeDeploymentFixture (test/test-utils/fixture.ts:37:3)
      at deployAll (test/deploy/managing-dao.ts:29:3)
      at Context.<anonymous> (test/deploy/managing-dao.ts:78:5)
      at DeploymentsManager.executeDeployScripts (/Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1215:19)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at DeploymentsManager.runDeploy (/Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1061:5)
      at Object.fixture (/Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:316:9)
      at /Users/michaelheuer/Projects/Aragon/osx/packages/contracts/test/test-utils/fixture.ts:34:5
      at /Users/michaelheuer/Projects/Aragon/osx/node_modules/hardhat-deploy/src/DeploymentsManager.ts:341:24
      at initializeDeploymentFixture (test/test-utils/fixture.ts:37:3)
      at deployAll (test/deploy/managing-dao.ts:29:3)
      at Context.<anonymous> (test/deploy/managing-dao.ts:78:5)

heueristik avatar May 17 '24 13:05 heueristik