Skip to content

feat: add config for impersonating accounts to BS#864

Merged
tynes merged 7 commits intodevelopfrom
feat/batch-submitter-impersonation
May 13, 2021
Merged

feat: add config for impersonating accounts to BS#864
tynes merged 7 commits intodevelopfrom
feat/batch-submitter-impersonation

Conversation

@karlfloersch
Copy link
Copy Markdown
Contributor

Useful for testing against hardhat forks.

Description
When testing changes to the batch submitter it is useful to impersonate mainnet accounts. This can be done with this new config!

Just use:

$ npx hardhat node --fork https://mainnet.infura.io/...

@karlfloersch karlfloersch requested a review from tynes May 13, 2021 03:28
@karlfloersch karlfloersch requested a review from annieke as a code owner May 13, 2021 03:29
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented May 13, 2021

🦋 Changeset detected

Latest commit: eb4e362

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/batch-submitter Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@karlfloersch karlfloersch force-pushed the feat/batch-submitter-impersonation branch from 800a7bf to 034406a Compare May 13, 2021 03:32
RUN_OVM_TEST_GAS,
} from './test/helpers/constants'

import '@nomiclabs/hardhat-ethers'
Copy link
Copy Markdown
Contributor Author

@karlfloersch karlfloersch May 13, 2021

Choose a reason for hiding this comment

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

Is there anything wrong with adding this import? Does it make the production version of our software use the hardhat version of ethers?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe it just mutates the hre

Useful for testing against hardhat forks.
@tynes tynes force-pushed the feat/batch-submitter-impersonation branch from 58f6753 to dfa9e2b Compare May 13, 2021 06:02
karlfloersch and others added 5 commits May 12, 2021 23:03
* feat: make block offset configurable

* Add a USE_HARDHAT config

* batch-submitter: must pass both impersonate options (#866)

Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 13, 2021

Codecov Report

Merging #864 (4de14fb) into develop (96a586e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #864   +/-   ##
========================================
  Coverage    82.21%   82.21%           
========================================
  Files           48       48           
  Lines         1895     1895           
  Branches       303      303           
========================================
  Hits          1558     1558           
  Misses         337      337           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96a586e...4de14fb. Read the comment docs.

Copy link
Copy Markdown
Contributor

@annieke annieke left a comment

Choose a reason for hiding this comment

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

it bothers me a little having testing / debug logic in the main code path, but can't think of a better way to do it than this for now! just one typo + nit

logger = new Logger({ name })
}

const useHardhat = config.bool('use-hardhat', !!env.USE_HARDAT)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const useHardhat = config.bool('use-hardhat', !!env.USE_HARDAT)
const useHardhat = config.bool('use-hardhat', !!env.USE_HARDHAT)

is the double negative here to get the variable to be true even if undefined? i think an || would be more readable here for that

Copy link
Copy Markdown
Contributor

@tynes tynes May 13, 2021

Choose a reason for hiding this comment

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

Using the double negative syntax is shorthand for a typecast to a boolean. A single ! type casts it to the negation and the second ! cancels out the negation and returns it to the original truthiness value. It is a relatively common pattern in JS, a replacement for using Boolean(...) to type cast. I don't really see anybody ever use Boolean(...), probably because its more verbose?

is the double negative here to get the variable to be true even if undefined?

No its not, that would be a single negation (undefined is falsey so a single ! would make it truthy). We only wan't this mode to operate when the env var USE_HARDHAT is defined

@tynes
Copy link
Copy Markdown
Contributor

tynes commented May 13, 2021

it bothers me a little having testing / debug logic in the main code path

I agree, definitely open to suggestions
A getProvider abstraction makes sense

@tynes tynes merged commit c79dc8b into develop May 13, 2021
@tynes tynes deleted the feat/batch-submitter-impersonation branch May 13, 2021 19:37
InoMurko pushed a commit to omgnetwork/optimism that referenced this pull request May 25, 2021
)

* feat: add config for impersonating accounts to BS

Useful for testing against hardhat forks.

* Make block offset configurable (ethereum-optimism#865)

* feat: make block offset configurable

* Add a USE_HARDHAT config

* batch-submitter: must pass both impersonate options (ethereum-optimism#866)

Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>

* batch-submitter: update example env

* batch-submitter: lint fix

* batch-submitter: clean up old comments

* batch-submitter: USE_HARDHAT

* batch-submitter: add error messages

Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-op-batcher Area: op-batcher

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants