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

Zksync testing #596

Draft
wants to merge 120 commits into
base: main
Choose a base branch
from
Draft

Zksync testing #596

wants to merge 120 commits into from

Conversation

novaknole
Copy link
Contributor

@novaknole novaknole commented May 15, 2024

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 and others added 30 commits April 3, 2024 12:41
Co-authored-by: Michael Heuer <[email protected]>
Co-authored-by: Michael Heuer <[email protected]>
Co-authored-by: Michael Heuer <[email protected]>
Co-authored-by: Michael Heuer <[email protected]>
add the .env for test description
Generating a human readable deployed contracts JSON file
@novaknole
Copy link
Contributor Author

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.

@heueristik
Copy link
Contributor

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).

@novaknole
Copy link
Contributor Author

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. -_-

Copy link
Contributor

@heueristik heueristik left a comment

Choose a reason for hiding this comment

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

I've compared the contracts and checked most of the files.
It wasn't possible for me to check

I ignored tests that are skipped on zkSync

This warning

┌──────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Warning: It looks like you are using 'ecrecover' to validate a signature of a user account.      │
│ zkSync Era comes with native account abstraction support, therefore it is highly recommended NOT │
│ to rely on the fact that the account has an ECDSA private key attached to it since accounts might│
│ implement other signature schemes.                                                               │
│ Read more about Account Abstraction at https://v2-docs.zksync.io/dev/developer-guides/aa.html    │
└──────────────────────────────────────────────────────────────────────────────────────────────────┘

seems to be irrelevant because we don't use ecrecover. Please confirm this.

Becuase the tests are not working on the runner, I ran the test suite locally following the packages/contracts/README-zkSync.md instructions.
In one terminal I ran

nvm use 18 && yarn clean && yarn build --network zkLocalTestnet && yarn build

Then I ran

yarn hardhat node-zksync   

and in another terminal ran

nvm use 18 && yarn test  --network zkLocalTestnet

this resulted in the following errors:


  DAO
    1) "before each" hook for "does not support the empty interface"

  TestParameterScopingCondition
    2) "before all" hook in "TestParameterScopingCondition"

  SharedPlugin
    3) "before each" hook for "increments IDs"

  Managing DAO

Deploying ManagingDao.
ManagingDAO will be owned by the (Deployer: 0xBC989fDe9e54cAd2aB4392Af6dF60f04873A033A) temporarily, while the entire framework is getting deployed. At the final step when Multisig is available, it will be installed on managingDAO and all roles for the Deployer will be revoked.
    4) "before all" hook for "should have deployments"

  DAOFactory: 
    5) "before each" hook for "reverts if no plugin is provided"

  DAORegistry
    6) "before each" hook for "succeeds even if the dao subdomain is empty"

  PluginRepoFactory: 
    ERC-165
      7) "before each" hook for "does not support the empty interface"

  PluginRepoRegistry
    8) "before each" hook for "successfully sets subdomainregistrar"

  PluginSetupProcessor
    10) "before all" hook in "PluginSetupProcessor"

  PluginSetupProcessorUpgradeable
    11) "before all" hook in "PluginSetupProcessorUpgradeable"

  ENSSubdomainRegistrar
    Check the initial ENS state
      12) "before each" hook for "unregistered domains are owned by the zero address on ENS"

  InterfaceBasedRegistry
    13) "before all" hook in "InterfaceBasedRegistry"

  MajorityVotingMock
    14) "before all" hook in "MajorityVotingMock"

  AddresslistVoting
    15) "before all" hook in "AddresslistVoting"

  AddresslistVotingSetup
    16) "before all" hook for "does not support the empty interface"

  TokenVoting
    17) "before all" hook in "TokenVoting"

  TokenVotingSetupZkSync
    18) "before all" hook for "does not support the empty interface"

  Multisig
    19) "before all" hook in "Multisig"

  MultisigSetup
    20) "before all" hook for "does not support the empty interface"

  MerkleDistributor
    plugin interface: 
      21) "before each" hook for "does not support the empty interface"

  MerkleMinter
    Upgrades
      22) "before each" hook for "from v1.0.0"

  GovernanceERC20
    23) "before all" hook in "GovernanceERC20"

  24) DAO Upgrade
       "before all" hook in "DAO Upgrade":

What is causing the tests to fail for me? I advise that you make these test pass on the runner.

Comment on lines +58 to +59
"@openzeppelin/contracts": "4.9.5",
"@openzeppelin/contracts-upgradeable": "4.9.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest OZ version is 4.9.6

@heueristik
Copy link
Contributor

Update on #596 (review):

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants