Skip to content

Conversation

@fullkomnun
Copy link
Contributor

@fullkomnun fullkomnun commented Aug 27, 2023

What does this implement/fix? Explain your changes.

  • Currently CONFIRMATIONS from 'config' is set to the default value of '12' or according to the matching env var if defined. It is referenced in common-files/utils/event-queue.mjs which is used by 'nightfall-optimist' and 'nightfall-client' to wait for a configured number of block confimrations before a L1 event is propagated and processed allowing for L1 re-org.
    During tests you usually want to either use a lower number of confirmations (such as 1) or when tests rely on this behaviour using a higher number of confimrations (like when testing L1 re-org) but leveraging Ganache like that:

test/utils.mjs

  async evmMine(n = 1) {
    await this.web3.currentProvider.send({
      jsonrpc: '2.0',
      method: 'evm_mine',
      params: [{ blocks: n }],
      id: 0,
    });
  }

some test file

await nf3Proposer.makeBlockNow();
await web3Client.waitForEvent(eventLogs, ['blockProposed']);
await web3Client.evmMine(CONFIRMATIONS + CONFIRMATIONS_BUFFER);
  • In .github/workflows/on-pull-request-master.yml most e2e test are setup with CONFIRMATIONS: 1, however value of env vars are not automatically propagated to containers and therefore the default value of '12' applies. Fixed that by editing docker-compose.*.yml files.
  • test/utils.mjs also waits for a number of block confirmations in 'submitTransaction' and in 'waitForEvent'. Curently it is hardcoded and set to '12', fixed it to grab the confirmations number from 'config'
  • Nf3 (from /cli) used by tests and 'proposer' / 'challenger' currently sets web3.eth.transactionConfirmationBlocks to the hardcoded value of '12'. Fixed that to use process.env.CONFIRMATIONS || 12 instead ('config' is not currently used by Nf3 / CLI so I did not use node 'config').
  • By default Ganache which is used for dev/test mines a block every second, so running tests when effectively client/optimist or test utils waits for 12 confirmations leads to excessive waiting and increases tests runtime.

Does this close any currently open issues?

Not that I'm aware of

What commands can I run to test the change?

Run all tests as usuall both locally and in CI (focus on these e2e tests that have CONFIRMATIONS: 1 which is all e2e tests except for 'client-authentication-test', 'test-apps')

Any other comments?

  • 'adversary-test' total runtime has decreased from '55m 4s' to '35m 19s' - minus 20 minutes!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant