Refactor tests and test EXTCODECOPY Failure Behavior#25
Merged
masonforest merged 9 commits intomasterfrom Mar 3, 2020
Merged
Conversation
Other changes: * Add helper method for making ovm calls
... when provided an invalid address
Contributor
There was a problem hiding this comment.
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.
| addressToBytes32Address(callContractAddress), | ||
| methodIds.makeStaticCallThenCall, | ||
| addressToBytes32Address(callContractAddress), | ||
| ]) |
Contributor
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Didn't review the whole PR, but looks good to me 👍
| 0, | ||
| dummyContractBytecode.length, | ||
| ] | ||
| ) |
Contributor
There was a problem hiding this comment.
Nice solid code size reduction
packages/ovm/package.json
Outdated
| "ethereum-waffle": "2.1.0", | ||
| "ethers": "^4.0.37", | ||
| "ethlint": "^1.2.5", | ||
| "lodash": "^4.17.15", |
Contributor
There was a problem hiding this comment.
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.
Author
There was a problem hiding this comment.
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
karlfloersch
approved these changes
Feb 28, 2020
Author
Good call, changed! 👍
|
snario
pushed a commit
that referenced
this pull request
Apr 14, 2021
* 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 <me@gakonst.com>
protolambda
pushed a commit
to protolambda/optimism
that referenced
this pull request
May 1, 2022
Update introduction.md Co-authored-by: Maurelian <maurelian@protonmail.ch> Update introduction.md Co-authored-by: Maurelian <maurelian@protonmail.ch> Update introduction.md Co-authored-by: Maurelian <maurelian@protonmail.ch> Update introduction.md Co-authored-by: Maurelian <maurelian@protonmail.ch> Update introduction.md Co-authored-by: Maurelian <maurelian@protonmail.ch> Update introduction.md Co-authored-by: Maurelian <maurelian@protonmail.ch> Address the rest of the feedback
Merged
Merged
max-sanchez
referenced
this pull request
in hemilabs/optimism
Apr 16, 2024
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 (#37) 1588cbf04 Add common user-specific files to gitignore (ethereum-optimism#51) 210aabe7a Update popm.go, fix typo (#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 (#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
ClaytonNorthey92
referenced
this pull request
in hemilabs/optimism
Apr 17, 2024
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 (#37) 1588cbf04 Add common user-specific files to gitignore (ethereum-optimism#51) 210aabe7a Update popm.go, fix typo (#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 (#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
blockchaindevsh
pushed a commit
to blockchaindevsh/optimism
that referenced
this pull request
Jun 24, 2024
merge from optimism develop
bap2pecs
pushed a commit
to babylonlabs-io/optimism
that referenced
this pull request
Jul 31, 2024
bap2pecs
pushed a commit
to babylonlabs-io/optimism
that referenced
this pull request
Jul 31, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 12, 2024
* 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 <ng@defi.sucks> Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> * fix: set an arbitrary address for superchain erc20 impl * fix: deploy a proxy for the beacon on genesis (#45) --------- Co-authored-by: 0xng <ng@defi.sucks> * 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 <ng@defi.sucks> Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>
samlaf
pushed a commit
to samlaf/optimism
that referenced
this pull request
Nov 10, 2024
* 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 <ng@defi.sucks> Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> * 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 <ng@defi.sucks> * 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 <ng@defi.sucks> Co-authored-by: 0xParticle <particle@defi.sucks> Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com> Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>
SozinM
pushed a commit
to NethermindEth/optimism
that referenced
this pull request
Feb 10, 2025
…r-payload-bytes
ClaytonNorthey92
referenced
this pull request
in hemilabs/optimism
Apr 4, 2025
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 (#37) 1588cbf04 Add common user-specific files to gitignore (ethereum-optimism#51) 210aabe7a Update popm.go, fix typo (#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 (#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
ClaytonNorthey92
referenced
this pull request
in hemilabs/optimism
Apr 7, 2025
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 (#37) 1588cbf04 Add common user-specific files to gitignore (ethereum-optimism#51) 210aabe7a Update popm.go, fix typo (#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 (#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
blockchaindevsh
pushed a commit
to blockchaindevsh/optimism
that referenced
this pull request
Jun 30, 2025
* 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
* Add cursor rules * Fix db
theochap
pushed a commit
that referenced
this pull request
Dec 10, 2025
* ♻ Clean up `OracleReader` * ♻ Cleanup
Zena-park
added a commit
to tokamak-network/optimism
that referenced
this pull request
Dec 30, 2025
commit 033b865 Author: Zena-park <zena@tokamak.network> Date: Wed Jul 10 19:37:02 2024 +0900 refactor: delete try-catch statement ethereum-optimism#25 commit d5627e4 Author: Zena-park <zena@tokamak.network> Date: Tue Jul 9 15:52:29 2024 +0900 test: setExplorer ethereum-optimism#24 commit f07c2db Author: Zena-park <zena@tokamak.network> Date: Tue Jul 9 14:34:24 2024 +0900 feat: setExplorer function ethereum-optimism#24
Zena-park
added a commit
to tokamak-network/optimism
that referenced
this pull request
Dec 30, 2025
theochap
pushed a commit
that referenced
this pull request
Jan 15, 2026
Include deposit in arbitrary testing
theochap
pushed a commit
that referenced
this pull request
Jan 21, 2026
Include deposit in arbitrary testing
sebastianst
added a commit
that referenced
this pull request
Apr 2, 2026
…nd oversized payloads Add two bounds checks to `read_tx_data` before indexing into the slice: 1. Reject payloads exceeding `MAX_SPAN_BATCH_ELEMENTS` with `TooBigSpanBatchSize`, matching op-node's `rlp.NewStream(r, MaxSpanBatchElementCount)` behavior. Without this, kona accepts oversized transactions that op-node rejects, causing a consensus divergence. (Cantina finding #28) 2. Reject truncated payloads where `payload_length_with_header` exceeds the remaining buffer, preventing a panic on malformed input. (Cantina finding #25, item 4) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 2, 2026
…nd oversized payloads (#19904) Add two bounds checks to `read_tx_data` before indexing into the slice: 1. Reject payloads exceeding `MAX_SPAN_BATCH_ELEMENTS` with `TooBigSpanBatchSize`, matching op-node's `rlp.NewStream(r, MaxSpanBatchElementCount)` behavior. Without this, kona accepts oversized transactions that op-node rejects, causing a consensus divergence. (Cantina finding #28) 2. Reject truncated payloads where `payload_length_with_header` exceeds the remaining buffer, preventing a panic on malformed input. (Cantina finding #25, item 4) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
executeOVMCall,encodeMethodIdandencodeRawArgumentstest helperstslintQuestions
noUnusedLocalsandnoUnusedParametersin ourtsconfigto prevent unused variables?Metadata
Fixes
Contributing Agreement