First pass at integrating ITX with Optimism by stonecoldpat · Pull Request #43 · ethereum-optimism/batch-submitter

stonecoldpat · 2021-03-02T16:10:28Z

It is now set up to send transactions via ITX.

If you add an Infura ID into the .env for "ITX_PROVIDER", it will enable it.
It reuses the sequencer key for signing messages that are sent to ITX (authenticate to the service)

We need to update the smart contracts of optimism to check ecrecover instead of msg.sender. Then we can test how well ITX works in this script.

yahgwai · 2021-03-02T16:27:03Z

src/batch-submitter/batch-submitter.ts

-      gasRetryIncrement: this.gasRetryIncrement
-    }
+    // Enable ITX by setting a provider. It'll re-use the sequencer key.
+    if (this.itxProvider !== '') {


This string value doesnt appear to be used

i've used it to set the provider now

yahgwai · 2021-03-02T16:27:28Z

src/batch-submitter/batch-submitter.ts

-    }
+    // Enable ITX by setting a provider. It'll re-use the sequencer key.
+    if (this.itxProvider !== '') {
+      const itx = new JsonRpcProvider(


Could use the provider that already exists on the signer - and check that it's an infura provider

this.signer.provider

Then there's need to connect a new one - in fact you dont have to pass one into sendTxWithITX and can pass this.signer.provider to itxWaitForTx

yahgwai · 2021-03-02T16:34:59Z

src/batch-submitter/batch-submitter.ts

-      this.log
-    )
+      const gas = await itx.estimateGas(
+        await signer.populateTransaction({ to, data })


I believe populateTransaction already calls estimateGas internally to populate the gasLimit. So if you do that probably dont need to call estimateGas. It may also be overkill, as it call getGasPrice, getTransactionCount, getChainId etc which you dont need just get a gasLimit estimate

yahgwai · 2021-03-02T16:40:58Z

src/utils.ts

+    await wait(2000)
+
+    // Let's try to send it again.
+    return sendTxWithITX(itx, signer, to, data, gas, iteration + 1)


Probably clearer just to do a while loop here instead of recursion - and you wouldnt need that extra iteration var in the function interface

yahgwai · 2021-03-02T16:41:48Z

src/utils.ts

+import { BigNumber, Signer, Wallet } from 'ethers'
+import { arrayify, defaultAbiCoder, keccak256 } from 'ethers/lib/utils'
+
+const RETRY_LIMIT = 3


This should probably have a better name - RETRY_LIMIT is very generic

renamed to RETRY_JSON_REQUEST_LIMIT

yahgwai · 2021-03-02T16:54:49Z

src/batch-submitter/batch-submitter.ts

+      )
+
+      this.log.info(`ITX relay transaction hash: ${relayTransactionHash}`)
+      return itxWaitForTx(itx, relayTransactionHash)


The else block logs:

this.log.debug('Transaction receipt:', receipt) this.log.info(successMessage)

I think you should log these things too

You can return the receipt from itxWaitForTx

yahgwai · 2021-03-02T16:55:54Z

src/utils.ts

+        if (receipt && receipt.confirmations && receipt.confirmations > 1) {
+          // The transaction is now on chain!
+          this.log.info(`Ethereum transaction hash: ${receipt.transactionHash}`)
+          return


As mentioned above, this should return the receipt

yahgwai · 2021-03-02T16:58:49Z

src/utils.ts

+
+      // check each of these hashes to see if their receipt exists and
+      // has confirmations
+      for (const response of statusResponse.length) {


I'm surprised the typescript compiler accepts this, I assume statusResponse is an array and length is a number.

If so you should have:

for (const response of statusResponse) {

Fixed. I originally had a normal forloop, but linter requires for-of. Looks like I wrote it wrong here.

yahgwai · 2021-03-02T17:00:31Z

src/utils.ts

+      // check each of these hashes to see if their receipt exists and
+      // has confirmations
+      for (const response of statusResponse.length) {
+        const hashes = response


Could set the name in the initial for/of, although I'd suggest broadcast as a better name since the object contains more than hashes.

for (const broadcast of statusResponse) {

yahgwai · 2021-03-02T17:02:03Z

src/utils.ts

+  } catch (e) {
+    // Take into account JSON RPC errors that may arise (e.g. rate limiting)
+
+    if (iteration === RETRY_LIMIT) {


Perhaps I'm paranoid, but I always use >= when comparing against a max, just in case someone does something funny with iteration.

snario · 2021-03-03T16:05:00Z

src/batch-submitter/batch-submitter.ts

+  resubmissionTimeout: number
+  minGasPriceInGwei: number
+  maxGasPriceInGwei: number


Unrelated to the core functionality implemented here, but there seem to be quite a few stylistic changes made here (like the removal of commas above). We don't have a linting / formatting CI step yet (which we ought to soon hopefully). Absent that could you try to strip them from these commits?

stonecoldpat added 2 commits March 2, 2021 16:08

First pass at integrating ITX with Optimism

65cdecc

fixed lint

826eeea

yahgwai reviewed Mar 2, 2021

View reviewed changes

stonecoldpat added 2 commits March 2, 2021 17:30

Resolved some comments by Chris

206cade

Fixed some build issues

2cbfa68

snario reviewed Mar 3, 2021

View reviewed changes

Conversation

stonecoldpat commented Mar 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants