Migrate HardHat tests to DS-tests (Solidity tests)

[richardpringle]

Why this should be merged

It's the first big step in the direction of solving some of the security vulnerabilities related to using hardhat and other node-modules.

How this works

Currently, HardHat is still used to run the test, but all the logic is in Solidity

How this was tested

Example of passing test:
https://github.com/ava-labs/subnet-evm/actions/runs/4600753486/jobs/8127753059?pr=601#step:8:259

Next Steps

Once all the tests have been migrated over to solidity, we have a couple of options for removing hardhat. We can use forge to build all the solidity, then use ethers directly to deploy.

EthersJS has the following dependencies

root ➜ /com.docker.devenvironments.code/contract-examples (master ✗) $ yarn info ethers dependencies
yarn info v1.22.19
{
  '@adraffy/ens-normalize': '1.9.0',
  '@noble/hashes': '1.1.2',
  '@noble/secp256k1': '1.7.1',
  'aes-js': '4.0.0-beta.3',
  tslib: '2.4.0',
  ws: '8.5.0'
}

all of which have no other dependencies

root ➜ /com.docker.devenvironments.code/contract-examples (master ✗) $ npm view @adraffy/ens-normalize dependencies
root ➜ /com.docker.devenvironments.code/contract-examples (master ✗) $ npm view @noble/hashes dependencies
root ➜ /com.docker.devenvironments.code/contract-examples (master ✗) $ npm view aes-js dependencies
root ➜ /com.docker.devenvironments.code/contract-examples (master ✗) $ npm view tslib dependencies
root ➜ /com.docker.devenvironments.code/contract-examples (master ✗) $ npm view ws dependencies
root ➜ /com.docker.devenvironments.code/contract-examples (master ✗) $ 

We would still be dependent on NodeJS and mocha. While mocha still has quite a few dependencies, it is one of the most used libraries in the NodeJS ecosystem and should come up in an npm audit...

Speaking of npm audit, we should definitely be running that (there are a lot of vulnerabilities in the current dependency tree).

From there, I think it's worth evaluating whether or not we should remove the JS dependency entirely, and I'm not sure that it is.

closes #223

[aaronbuchwald]

Discussed offline, this looks like a good plan to me to:

• Migrate the testing logic to use DSTest
• Deploy/call test contracts from HardHat
• Migrate to deploy/call test contracts from Golang to remove npm/JS

[aaronbuchwald]

Code and plan looks good to me so far 👍

[darioush]

partial review, could we:

• remove the redundant tests in a separate PR
• fix the addDeployer issue that exists in the current tests?

this way this PR can be reviewed side by side and we can start removing the it.skip directives.

[darioush]

should we assert this in the setup instead of having a test?

[richardpringle]

I don't think adding assertions to the setUp functions would make for very clear errors. If for whatever reason the deployer was not setup as an admin properly, most of the tests would fail and it should be clear-ish that something is seriously wrong.

I don't have a super strong opinion here though, if you think I should add error-logging (for assertions) to the setUp block, I can do that.

[darioush]

we should still have e2e test for this.

[richardpringle]

Agreed. Is there no better place to test this functionality, though? Are we not testing the rest of the genesis stuff somewhere else?

[richardpringle]

@darioush ^

[darioush]

we should still have e2e test for this.

[richardpringle]

same reply
https://github.com/ava-labs/subnet-evm/pull/601/files#r1175457992

[ceyonur]

looks good 👍 .
just a file rename comment.

[ceyonur]

some similar patterns in other tests
#601 (comment)

[ceyonur]

similar pattern here with: #601 (comment)

[ceyonur]

similar pattern here with: #601 (comment)

[ceyonur]

similar pattern here with: #601 (comment)

[ceyonur]

similar pattern here with: #601 (comment)

[richardpringle]

@ceyonur

some similar patterns in other tests #601 (comment)

Applying the pattern right now, but consider this scenario:

You have a failing test, and the step_ function is at the bottom of the contract. I have to scroll up to the top to see what rewardManager is (for example). I find that in tests, it's actually preferable and more flexible not to follow DRY principles and instead of overly explicit where possible. Tests are not part of a greater system, and each test should be able to pass on its own; therefore, repetition is okay. Just wanted to share my opinion.

[ceyonur]

@ceyonur

some similar patterns in other tests #601 (comment)

Applying the pattern right now, but consider this scenario:

You have a failing test, and the step_ function is at the bottom of the contract. I have to scroll up to the top to see what rewardManager is (for example). I find that in tests, it's actually preferable and more flexible not to follow DRY principles and instead of overly explicit where possible. Tests are not part of a greater system, and each test should be able to pass on its own; therefore, repetition is okay. Just wanted to share my opinion.

I don't have a strong opinion on this one. I agree that tests should be explicit as possible. I just feel like calling the precompile should be done once in each contract/test. In both cases we should be consistent amongst test files, so just wanted to make sure we follow same approach in every test file. Thanks for working on this one.

[darioush]

Could we move the comments to the place of the new declarations?

[darioush]

should we remove these in favor of the new enum?

[richardpringle]

I think we can do that in a new PR... Otherwise, this PR will never get over the line.

Originally, I didn't want some weird intermediate state with half JS tests and half Solidity tests, but in hindsight, that was a mistake.

[darioush]

is it better for this to be beforeEach or before? not sure which one we'd prefer.
It seems some of the tests must be run in order so not sure having independent test setup is preferable in these tests.

[richardpringle]

I tried to make it so the tests do not need to be run in order. Before this PR, they all needed to be run in a specific order, and most could not be run in isolation, which is a bad testing practice. I believe I have run every single one of these tests in isolation, the only thing I did not test is if you could run them out of order, but I think that's more effort to verify than it's worth.

Also, DS-Test will set its own state to failed once it fails a single test. You should still be able to see what other tests pass/fail when a single test fails. Tests in general should be independent.

[darioush]

should we keep the consts only used in tests in the test file?

[darioush]

should this assertion be checking the Role against expected role and initial balance with 0?

[richardpringle]

wow... good catch

[richardpringle]

Actually, I meant to be checking the balance. Changing this to assertRole was the result of a search and replace whose search params were a little too wide, apparently.

[darioush]

same as above

[darioush]

same as above

[aaronbuchwald]

+1 for stripping this down to only the testing logic

[aaronbuchwald]

It looks like this PR increases the time that the e2e tests take from about 5min to 11min in CI and about 200s to 600s running locally on my computer.

This seems to come from the fact that the tests are completing more total transactions now that it's re-deploying the contract and calling setup for each test.

[aaronbuchwald]

Modified tests to confirm they fail as expected and got this great ouput:

Combined output:

Compiling 1 file with 0.8.0
Compilation finished successfully


  ExampleDeployerList
    ✓ precompile should see owner address has admin role (4056ms)

Error: a == b not satisfied [uint]
      Left
BigNumber { value: "1" }
     Right
BigNumber { value: "0" }

    1) precompile should see test address has no role
    ✓ contract should report test address has no admin role (4051ms)
    ✓ contract should report owner address has admin role (4047ms)
    ✓ should not let test address deploy (4084ms)

Error: Assertion Failed

    2) should allow admin to add contract as admin
    ✓ should allow admin to add deployer address as deployer through contract (4049ms)
    ✓ should let deployer address to deploy (4053ms)
    ✓ should let admin revoke deployer (4067ms)


  7 passing (2m)
  2 failing

  1) ExampleDeployerList
       precompile should see test address has no role:

      AssertionError [ERR_ASSERTION]: step_newAddressHasNoRole failed
      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/utils.ts:123:9)
      at step (test/utils.ts:33:23)
      at Object.next (test/utils.ts:14:53)
      at fulfilled (test/utils.ts:5:58)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

  2) ExampleDeployerList
       should allow admin to add contract as admin:

      AssertionError [ERR_ASSERTION]: step_adminAddContractAsAdmin failed
      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/utils.ts:123:9)
      at step (test/utils.ts:33:23)
      at Object.next (test/utils.ts:14:53)
      at fulfilled (test/utils.ts:5:58)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)





Err: exit status 2
  [FAILED] Expected
      <*exec.ExitError | 0xc001182000>: {
          ProcessState: {
              pid: 54074,
              status: 512,
              rusage: {
                  Utime: {
                      Sec: 7,
                      Usec: 863281,
                      Pad_cgo_0: [0, 0, 0, 0],
                  },
                  Stime: {
                      Sec: 0,
                      Usec: 806771,
                      Pad_cgo_0: [0, 0, 0, 0],
                  },
                  Maxrss: 322871296,
                  Ixrss: 0,
                  Idrss: 0,
                  Isrss: 0,
                  Minflt: 161476,
                  Majflt: 15,
                  Nswap: 0,
                  Inblock: 0,
                  Oublock: 0,
                  Msgsnd: 676,
                  Msgrcv: 675,
                  Nsignals: 2,
                  Nvcsw: 1554,
                  Nivcsw: 13148,
              },
          },
          Stderr: nil,
      }
  to be nil
  In [It] at: /Users/aaronbuchwald/go/src/github.com/ava-labs/subnet-evm/tests/utils/subnet.go:46 @ 05/23/23 16:37:04.524
  < Exit [It] contract deployer allow list - /Users/aaronbuchwald/go/src/github.com/ava-labs/subnet-evm/tests/precompile/solidity/suites.go:43 @ 05/23/23 16:37:04.524 (1m57.465s)
• [FAILED] [117.465 seconds]

[aaronbuchwald]

LGTM once @darioush approves