Refactor tests and test EXTCODECOPY Failure Behavior #25

masonforest · 2020-02-28T13:21:32Z

Description

Add a test to ensure EXTCODECOPY returns zeroed bytes when the provided range is out of bounds or the provided address is invalid.

Other Changes

Add executeOVMCall, encodeMethodId and encodeRawArguments test helpers
Use those helpers where applicable
Removed unused variables with tslint

Questions

Do we want to enable noUnusedLocals and noUnusedParameters in our tsconfig to prevent unused variables?

Metadata

Fixes

Fixes # YAS-119

Contributing Agreement

I have read and understood the Optimism Contributing Guide and Code of Conduct and am following those guidelines in this pull request.

Other changes: * Add helper method for making ovm calls

... when provided an invalid address

karlfloersch

Wowza! This is a real refactor! Super dope! The actual EXTCODECOPY tests look good but hilariously I feel like it might even be worth changing the title of the PR to "Refactor tests & test EXTCODECOPY Failure Behavior" or something.

It looks to me like a bunch of code was removed which is awesome! It adds a test but the overall code size is negative which is great. I had a couple comments with opinions weakly held on the style but overall think this looks good!

I'd give it an approve but I'm curious if @willmeister has any more sophisticated code style opinions. But if not imo this should be good to go & I'll change to approve! Dope!

Oh random question -- is it possible to add the "no unused imports / variables" to our automatic lint procedure? This was one lint that I really feel we're missing as I love knowing what I am and am not using in a file.

karlfloersch · 2020-02-28T19:21:41Z

packages/ovm/test/contracts/execution-manager.call-opcodes.spec.ts

+          addressToBytes32Address(callContractAddress),
+          methodIds.makeStaticCallThenCall,
+          addressToBytes32Address(callContractAddress),
+        ])


I don't feel qualified to really comment on much of this file because it's largely a matter of code style. @willmeister curious if you have any feedback here instead.

My gut reaction is that this update is slightly more readable but at the same time style is such a tricky thing to judge & there's totally cases to be made for the explicit string concatenation as well. Anyway I'm pretty laid back on this sort of stuff though so curious if anyone involved has a real strong opinion.

Didn't review the whole PR, but looks good to me 👍

karlfloersch · 2020-02-28T20:03:40Z

packages/ovm/test/contracts/execution-manager.code-opcodes.spec.ts

+          0,
+          dummyContractBytecode.length,
+        ]
+      )


Nice solid code size reduction

karlfloersch · 2020-02-28T20:15:36Z

packages/ovm/package.json

    "ethereum-waffle": "2.1.0",
    "ethers": "^4.0.37",
    "ethlint": "^1.2.5",
+    "lodash": "^4.17.15",


I kiiinda am a fan of not requiring lodash because it technically doesn't have to be used & adds weight to the package. That said, I also do really enjoy programming with lodash. Maybe if we only used lodash for testing functions & added it only to devDependencies then it'd be a no brainer? Either way, this is not a strong opinion so I'm personally fine with keeping lodash usage exactly as is.

Yeah agreed we shouldn't add dependencies unless their benefit outweighs the cost. I was using native reduce but fromPairs made it a bit more readable. I moved it to a devDependency so we can punt on that decision for now. I also added it to a list of things we can discuss as a team while we're all in person next week! 😄 -> TypeScript/Solidity Style Guide Discussion

masonforest · 2020-02-28T22:21:32Z

Wowza! This is a real refactor! Super dope! The actual EXTCODECOPY tests look good but hilariously I feel like it might even be worth changing the title of the PR to "Refactor tests & test EXTCODECOPY Failure Behavior" or something.

Good call, changed! 👍

Oh random question -- is it possible to add the "no unused imports / variables" to our automatic lint procedure? This was one lint that I really feel we're missing as I love knowing what I am and am not using in a file.

no-unused-variable was deprecated in tslint since TypeScript 2.9 in favor of it being built into the TypeScript compiler itself. 😕 Maybe we enable it in the compiler configuration in a separate PR?

* Upgraded to new RingBuffer * First pass at verification function * Removed BaseChain as a contract * Update OVM_CanonicalTransactionChain.spec.ts * Removed unused chain variable * Added overwriting logic to CTC * Corrected CTC event emit * Remove timebound ring buffer tests * Integrated bond manager PR Co-authored-by: Georgios Konstantopoulos <[email protected]>

TEST ONLY DO NOT MERGE

Update introduction.md Co-authored-by: Maurelian <[email protected]> Update introduction.md Co-authored-by: Maurelian <[email protected]> Update introduction.md Co-authored-by: Maurelian <[email protected]> Update introduction.md Co-authored-by: Maurelian <[email protected]> Update introduction.md Co-authored-by: Maurelian <[email protected]> Update introduction.md Co-authored-by: Maurelian <[email protected]> Address the rest of the feedback

88047e707 Add tbcd, a small bitcoin daemon that participates on bitcoin p2p (ethereum-optimism#50) 1146a08b5 localnet reorg fix (ethereum-optimism#76) 87f18a191 build(deps): bump github.com/docker/docker (ethereum-optimism#64) 9073baeaf localnet (ethereum-optimism#37) 1588cbf04 Add common user-specific files to gitignore (ethereum-optimism#51) 210aabe7a Update popm.go, fix typo (ethereum-optimism#40) a5e689493 make: automate copyright headers (#31) 1c3bfc9bc Use `maps.Clone(m)` to copy returned map in `APICommands()` (#33) 1be4df2a3 Use 'errors.Is' to compare errors (#32) 3f6bc5f8e e2e: sync ElectrumX environment variables with infra (ethereum-optimism#36) c5b0fea01 electrumx: add connection reuse and pooling (#26) cfc1293e9 Update README.md (#29) 8896259f0 retry mine keystone on failure (#18) a10e3bb29 Use '%w' verb in fmt.Errorf to wrap errors (fixes #13) (#27) 6cd677611 deps: update google.golang.org/protobuf to v1.33.0 (#28) ed7eb8e97 ci: fix concurrency cancel-in-progress for pull requests (#16) ac3b7eacb docker: update golang image to v1.22.1 (#25) d6b0ac8af returning response errors if they exist from bfg -> popm (#24) d450b787a Network test start height + no panic (#22) b390805c5 allowing BTC Block and L2 Keystone generation rates to be configurable in local network (#19) bfd3b1dc0 make: add -local flag to goimports (#9) e0e8964fc Move internal error into protocol package (#10) 7875a897c l2 keystone mining fixes (#3) git-subtree-dir: heminetwork git-subtree-split: 88047e707e2db8522e2ad77c5f849e55bc94cd10

merge from optimism develop

…ism#25)

* test: add L2 standard bridge interop unit tests (#13) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: unit tests fixes * fix: super to legacy tests failing * fix: mock and expect mint and burn * fix: add generic factory interface (#14) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert (#17) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert * fix: use only a public function for create3 * feat: rollback interop factory, modify legacy one * fix: delete local token return variable * fix: PR fixes * feat: add superchain erc20 factory implementation (#23) * feat: add superchain erc20 factory implementation * fix: remove createX comments * test: add superchain erc20 factory tests (#25) * test: add superchain erc20 factory tests * test: add erc20 asserts * test: fix expect emit * fix: remove comments * feat: add constructor to superchain ERC20 beacon (#34) * test: remove factory predeploy etch ---------- Co-authored-by: 0xng <[email protected]> Co-authored-by: 0xParticle <[email protected]> Co-authored-by: gotzenx <[email protected]> * fix: set an arbitrary address for superchain erc20 impl * fix: deploy a proxy for the beacon on genesis (#45) --------- Co-authored-by: 0xng <[email protected]> * fix: conflicts and imports * fix: interfaces * chore: add .testdata * fix: adding back .testdata to gitignore * fix: new conflicts from ci improvements --------- Co-authored-by: 0xng <[email protected]> Co-authored-by: 0xParticle <[email protected]> Co-authored-by: gotzenx <[email protected]> Co-authored-by: Disco <[email protected]>

* test: add L2 standard bridge interop unit tests (ethereum-optimism#13) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: unit tests fixes * fix: super to legacy tests failing * fix: mock and expect mint and burn * fix: add generic factory interface (ethereum-optimism#14) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert (ethereum-optimism#17) * test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert * fix: use only a public function for create3 * feat: rollback interop factory, modify legacy one * fix: delete local token return variable * fix: PR fixes * feat: add superchain erc20 factory implementation (ethereum-optimism#23) * feat: add superchain erc20 factory implementation * fix: remove createX comments * test: add superchain erc20 factory tests (ethereum-optimism#25) * test: add superchain erc20 factory tests * test: add erc20 asserts * test: fix expect emit * fix: remove comments * feat: add constructor to superchain ERC20 beacon (ethereum-optimism#34) * test: remove factory predeploy etch ---------- Co-authored-by: 0xng <[email protected]> Co-authored-by: 0xParticle <[email protected]> Co-authored-by: gotzenx <[email protected]> * fix: set an arbitrary address for superchain erc20 impl * fix: deploy a proxy for the beacon on genesis (ethereum-optimism#45) --------- Co-authored-by: 0xng <[email protected]> * fix: conflicts and imports * fix: interfaces * chore: add .testdata * fix: adding back .testdata to gitignore * fix: new conflicts from ci improvements --------- Co-authored-by: 0xng <[email protected]> Co-authored-by: 0xParticle <[email protected]> Co-authored-by: gotzenx <[email protected]> Co-authored-by: Disco <[email protected]>

…r-payload-bytes

88047e707 Add tbcd, a small bitcoin daemon that participates on bitcoin p2p (ethereum-optimism#50) 1146a08b5 localnet reorg fix (ethereum-optimism#76) 87f18a191 build(deps): bump github.com/docker/docker (ethereum-optimism#64) 9073baeaf localnet (ethereum-optimism#37) 1588cbf04 Add common user-specific files to gitignore (ethereum-optimism#51) 210aabe7a Update popm.go, fix typo (ethereum-optimism#40) a5e689493 make: automate copyright headers (#31) 1c3bfc9bc Use `maps.Clone(m)` to copy returned map in `APICommands()` (#33) 1be4df2a3 Use 'errors.Is' to compare errors (#32) 3f6bc5f8e e2e: sync ElectrumX environment variables with infra (ethereum-optimism#36) c5b0fea01 electrumx: add connection reuse and pooling (#26) cfc1293e9 Update README.md (#29) 8896259f0 retry mine keystone on failure (#18) a10e3bb29 Use '%w' verb in fmt.Errorf to wrap errors (fixes #13) (#27) 6cd677611 deps: update google.golang.org/protobuf to v1.33.0 (#28) ed7eb8e97 ci: fix concurrency cancel-in-progress for pull requests (#16) ac3b7eacb docker: update golang image to v1.22.1 (#25) d6b0ac8af returning response errors if they exist from bfg -> popm (#24) d450b787a Network test start height + no panic (#22) b390805c5 allowing BTC Block and L2 Keystone generation rates to be configurable in local network (#19) bfd3b1dc0 make: add -local flag to goimports (#9) e0e8964fc Move internal error into protocol package (#10) 7875a897c l2 keystone mining fixes (#3) git-subtree-dir: heminetwork git-subtree-split: 88047e707e2db8522e2ad77c5f849e55bc94cd10

* update gas * fix no suffix * revert * follow the naming convention for SGT * fix comments

* Add cursor rules * Fix db

* ♻ Clean up `OracleReader` * ♻ Cleanup

commit 033b865 Author: Zena-park <[email protected]> Date: Wed Jul 10 19:37:02 2024 +0900 refactor: delete try-catch statement ethereum-optimism#25 commit d5627e4 Author: Zena-park <[email protected]> Date: Tue Jul 9 15:52:29 2024 +0900 test: setExplorer ethereum-optimism#24 commit f07c2db Author: Zena-park <[email protected]> Date: Tue Jul 9 14:34:24 2024 +0900 feat: setExplorer function ethereum-optimism#24

Include deposit in arbitrary testing

masonforest added 8 commits February 25, 2020 17:12

Add an EXTCODECOPY test when the code range is out of bounds

fd3f491

Other changes: * Add helper method for making ovm calls

Use helpers in call opcode tests

fbdac5d

Use new test helpers

ac33b26

Remove unused code

dca287d

Remove unused variables

bcbed77

Use precomputed methodIds

1fc27ad

Add a test to ensure ovmEXTCODECOPY returns zeroed bytes

6e574d7

... when provided an invalid address

Lint

2740483

karlfloersch reviewed Feb 28, 2020

View reviewed changes

masonforest changed the title ~~Test EXTCODECOPY Failure Behavior~~ Refactor tests and test EXTCODECOPY Failure Behavior Feb 28, 2020

Change lodash to a devDependency

a7ffb92

karlfloersch approved these changes Feb 28, 2020

View reviewed changes

masonforest merged commit 8f7e8cc into master Mar 3, 2020

masonforest deleted the YAS-119/HardenTheOvm/FixEXTCODECOPYFailureBehavior branch March 23, 2020 13:22

InoMurko referenced this pull request in omgnetwork/optimism May 25, 2021

Merge pull request #25 from enyalabs/nft2

1334c95

TEST ONLY DO NOT MERGE

InoMurko referenced this pull request in omgnetwork/optimism May 25, 2021

Merge pull request #25 from enyalabs/nft2

ea5ef94

TEST ONLY DO NOT MERGE

mslipper mentioned this pull request May 16, 2022

Bedrock -> Develop #2563

Merged

github-actions bot mentioned this pull request Sep 20, 2022

Version Packages #3509

Merged

blockchaindevsh pushed a commit to blockchaindevsh/optimism that referenced this pull request Jun 24, 2024

Merge pull request ethereum-optimism#25 from ethereum-optimism/develop

8366449

merge from optimism develop

bap2pecs pushed a commit to babylonlabs-io/optimism that referenced this pull request Jul 31, 2024

fix finalizer_test.go (ethereum-optimism#25)

0d2a0da

bap2pecs pushed a commit to babylonlabs-io/optimism that referenced this pull request Jul 31, 2024

fix: Proper usage message for daemon create-validator (ethereum-optim…

98d3a1d

…ism#25)

SozinM pushed a commit to NethermindEth/optimism that referenced this pull request Feb 10, 2025

Merge pull request ethereum-optimism#25 from flashbots/metrics-builde…

8928824

…r-payload-bytes

blockchaindevsh pushed a commit to blockchaindevsh/optimism that referenced this pull request Jun 30, 2025

CircleCI: fix test_cannotReinitialize_succeeds (ethereum-optimism#25)

2b19f44

* update gas * fix no suffix * revert * follow the naming convention for SGT * fix comments

cuiweixie pushed a commit to cuiweixie/optimism that referenced this pull request Oct 22, 2025

Housekeeping: Add xlayer cursor rules (ethereum-optimism#25)

b14a245

* Add cursor rules * Fix db

theochap pushed a commit that referenced this pull request Dec 10, 2025

♻️ Cleanup (#25)

ad2b366

* ♻ Clean up `OracleReader` * ♻ Cleanup

Zena-park added a commit to tokamak-network/optimism that referenced this pull request Dec 30, 2025

refactor: delete try-catch statement ethereum-optimism#25

033b865

theochap pushed a commit that referenced this pull request Jan 15, 2026

Merge pull request #25 from alloy-rs/emhane/fix-arbitrary-op-tx-ty

c05f055

Include deposit in arbitrary testing

theochap pushed a commit that referenced this pull request Jan 21, 2026

Merge pull request #25 from alloy-rs/emhane/fix-arbitrary-op-tx-ty

9fa3fde

Include deposit in arbitrary testing

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor tests and test EXTCODECOPY Failure Behavior #25

Refactor tests and test EXTCODECOPY Failure Behavior #25

Uh oh!

masonforest commented Feb 28, 2020

Uh oh!

karlfloersch left a comment •

edited

Loading

Uh oh!

karlfloersch Feb 28, 2020

Uh oh!

willmeister Feb 28, 2020

Uh oh!

karlfloersch Feb 28, 2020

Uh oh!

karlfloersch Feb 28, 2020

Uh oh!

masonforest Feb 28, 2020

Uh oh!

masonforest commented Feb 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Refactor tests and test EXTCODECOPY Failure Behavior #25

Refactor tests and test EXTCODECOPY Failure Behavior #25

Uh oh!

Conversation

masonforest commented Feb 28, 2020

Description

Other Changes

Questions

Metadata

Fixes

Contributing Agreement

Uh oh!

karlfloersch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karlfloersch Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

willmeister Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

karlfloersch Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

karlfloersch Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

masonforest Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

masonforest commented Feb 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karlfloersch left a comment •

edited

Loading