Run tests against rinkeby

[sveitser]

• Fix hardhat tests against rinkeby (was missing awaits for transactions). This only (?) seems to cause a problem against slower networks because our local chains mine the transactions quickly enough.
• Run test_2user_and_submit against rinkeby

To test put RINKEBY_MNEMONIC and RINKEBY_URL in .env (see readme for instructions). Then run

hardhat --network rinkeby test
cape-test-rinkeby

The tests are quite slow.

Close #54

[alxiong]

LGTM, one minor comment.

[NimaVaziri]

Does this fix the issue described in #54? It seems there might be multiple issues in this case then, since the issue originally described there is relevant to tests failing with different outputs compared to expectations, and not having to do with async
/ mining mechanics?

[philippecamacho]

I get an error when running:

[nix-shell:~/repositories/Translucence/cape]$ hardhat test --network rinkeby
No need to generate any newer typings.


  Records Merkle Tree Benchmarks
    The Records Merkle root is updated with the frontier.
      ✓ shows how much gas is spent by updateRecordsMerkleTree (44601ms)

  Rescue benchmarks
    Gas spent for computing the Rescue function
      1) checks gas usage of TestRescue.hash
      ✓ checks gas usage of TestRescueNonOptimized.hash (60342ms)

  AccumulatingArray
    ✓ Accumulates correctly (20547ms)
    ✓ Reverts if the max length is exceeded (7472ms)

  CAPE
    Handling of nullifiers
      ✓ is possible to check for non-membership (20189ms)
      ✓ is possible to insert several elements (24653ms)

  Greeter (typescript)
    ✓ Should return the new greeting once it's changed (25891ms)

  Records Merkle Tree tests
    ✓ inserts all 27 leaves into a merkle tree of height 3 (26464ms)
    2) shows that inserting too many leaves triggers an error


  8 passing (6m)
  2 failing

  1) Rescue benchmarks
       Gas spent for computing the Rescue function
         checks gas usage of TestRescue.hash:

      AssertionError: expected 91712 to equal 90363
      + expected - actual

      -91712
      +90363
      
      at Context.<anonymous> (contracts/test/benchmarks/test-rescue.js:35:34)

  2) Records Merkle Tree tests
       shows that inserting too many leaves triggers an error:

      AssertionError: Expected transaction to be reverted
      + expected - actual

      -Transaction NOT reverted.
      +Transaction reverted.
      
` ` ` 

[sveitser]

Hmm. This worked for me yesterday but now also fails.

[sveitser]

I think it is also a race condition because the TX eventually fails in the block explorer:
https://rinkeby.etherscan.io/tx/0xc5f2bde2a8351edeeebcae445ae329441951989c53d11105483d5f0b947fef98

[sveitser]

So it seems to me that the doing an

await expect(...).to.be.rejected...

Relies on the estimate gas call to fail but we are setting gas in the rinkeby network config in hardhat.config.ts.

If we change the code to

    await expect(recordsMerkleTree.testUpdateRecordsMerkleTree(elems, { gasLimit: 10000000 })).to.be.revertedWith(
      "The tree is full."
    );

then the test also fails against geth with (not reverted).

Removing gas from the rinkeby netowrk config makes the test pass for me. Will try all the tests a few times to see if this is consistent.

Alternatively we could avoid using estimate gas and check for actual reversion of transactions. This sounds a bit more robust.

[sveitser]

Related issue

• .to.be.revertedWith only works with callStatic (Rinkeby) TrueFiEng/Waffle#579

And code in waffle: uses the error string from estimateGas if possible

https://github.com/EthWorks/Waffle/blob/3f46a6c8093cb9edb1a68c3ba15c4b4499ad595d/waffle-chai/src/matchers/revertedWith.ts#L16-L25

[sveitser]

@philippecamacho @NimaVaziri could you try again to see if this fixed the issue for you as well?

[NimaVaziri]

Retrying

[NimaVaziri]

Just retried rinkeby and the tests all passed. Retrying goerli now.

[NimaVaziri]

Retried tests against goerli too and they all passed as well. I'm still confused as to why this change fixed the original issue though but it looks like it has.

[NimaVaziri]

👍

Alternatively we could avoid using estimate gas and check for actual reversion of transactions. This sounds a bit more robust.

[NimaVaziri]

Interestingly running hardhat test --network hardhat does reproduce the original error

  1) Rescue benchmarks
       Gas spent for computing the Rescue function
         checks gas usage of TestRescue.hash:

      AssertionError: expected 91712 to equal 90363
      + expected - actual

      -91712
      +90363
      
      at Context.<anonymous> (contracts/test/benchmarks/test-rescue.js:35:34)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)
      at runNextTicks (internal/process/task_queues.js:66:3)
      at listOnTimeout (internal/timers.js:523:9)
      at processTimers (internal/timers.js:497:7)

[sveitser]

Interestingly running hardhat test --network hardhat does reproduce the original error

I'm also getting this. Don't know why it's happening at this point though.

[NimaVaziri]

is the gas setting for the hardhat config above not needed as well?

[sveitser]

I don't think we need it but removing it did not fix the issue with --network hardhat for me.

[NimaVaziri]

And related to this

The tests are quite slow.

if the plan is to integrate testnet-based testing into CI, is there an issue for making these tests run faster?

[sveitser]

And related to this

The tests are quite slow.

if the plan is to integrate testnet-based testing into CI, is there an issue for making these tests run faster?

There is

• Run the test on Rinkeby/Goerli inside the CI #56

but no ticket about speeding up tests.

I'm thinking we can just run a few integration tests with a timer trigger once a day or whatever. Then it's fine if it takes a while to run. I imagine running transactions in parallel would require quite some extra engineering with the current state of the rust test code.

[sveitser]

@NimaVaziri Thanks for the review and testing.