Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add chain-specific Block, LocalBlock, and BlockReceipt types #741

Merged
merged 25 commits into from
Dec 16, 2024

Conversation

Wodann
Copy link
Member

@Wodann Wodann commented Dec 10, 2024

Resolves #735

The primary change in this PR is the addition of chain-specific Block, LocalBlock, and BlockReceipt types; along with supporting types, such as a BlockBuilder and BlockReceiptFactory.

In addition, for parts of the codebase that I touched (or required it), I made the following changes:

  • use the newly added types to simplify existing function parameter and struct member types
  • remove ChainSpecT generics and instead replace them with more granular generics
  • add helper/convenience type aliases for chain-specific versions of the changed types

Follow-up

@Wodann Wodann requested a review from agostbiro December 10, 2024 12:09
Copy link

changeset-bot bot commented Dec 10, 2024

⚠️ No Changeset found

Latest commit: 265400a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Wodann Wodann had a problem deploying to github-action-benchmark December 10, 2024 12:09 — with GitHub Actions Failure
@Wodann Wodann self-assigned this Dec 10, 2024
@Wodann Wodann added the no changeset needed This PR doesn't require a changeset label Dec 10, 2024
@Wodann Wodann marked this pull request as draft December 10, 2024 14:52
@Wodann Wodann had a problem deploying to github-action-benchmark December 11, 2024 10:11 — with GitHub Actions Failure
@Wodann Wodann had a problem deploying to github-action-benchmark December 11, 2024 16:59 — with GitHub Actions Error
@Wodann Wodann had a problem deploying to github-action-benchmark December 11, 2024 17:09 — with GitHub Actions Failure
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 75.89520% with 276 lines in your changes missing coverage. Please review.

Please upload report for BASE (feat/multichain@6efbd64). Learn more about missing BASE report.

Files with missing lines Patch % Lines
crates/edr_napi_core/src/logger.rs 0.00% 27 Missing ⚠️
crates/edr_provider/src/pending.rs 16.12% 26 Missing ⚠️
crates/edr_provider/src/data.rs 79.80% 19 Missing and 2 partials ⚠️
crates/edr_optimism/src/receipt/block.rs 68.75% 15 Missing ⚠️
crates/tools/src/scenario.rs 0.00% 15 Missing ⚠️
crates/edr_evm/src/test_utils.rs 70.21% 10 Missing and 4 partials ⚠️
crates/edr_provider/src/data/gas.rs 6.66% 14 Missing ⚠️
...rates/edr_evm/src/blockchain/storage/contiguous.rs 0.00% 13 Missing ⚠️
crates/edr_optimism/src/block/builder.rs 86.36% 10 Missing and 2 partials ⚠️
...rates/edr_evm/src/blockchain/storage/reservable.rs 65.62% 10 Missing and 1 partial ⚠️
... and 43 more
Additional details and impacted files
@@                Coverage Diff                 @@
##             feat/multichain     #741   +/-   ##
==================================================
  Coverage                   ?   54.38%           
==================================================
  Files                      ?      243           
  Lines                      ?    28237           
  Branches                   ?    28237           
==================================================
  Hits                       ?    15358           
  Misses                     ?    11952           
  Partials                   ?      927           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Wodann Wodann temporarily deployed to github-action-benchmark December 15, 2024 17:34 — with GitHub Actions Inactive
@Wodann
Copy link
Member Author

Wodann commented Dec 16, 2024

The benchmark CI job was failing. I've been investigating it and learned the following (skipping debugging steps for brevity):

  1. Previously, both in HH+EthereumJS and HH+EDR, two test were failing. They failed for different reasons, but we accepted it since there was no regression.
    • The failing test is a test that checks whether a transaction reverts with an ABI encoded error reading InvalidSigner().
  2. Scenarios were recorded with these failing tests. The snapshot.json expects 305 failures.
  3. In my branch, the snapshot reports 306 failures; 1 too many.
    • The extra failure is from the test mentioned in step 1. and is a result of transaction revert with the expected ABI encoded error InvalidSigner()
  4. I tested the latest seaport commit with my version of HH+EDR and all tests are now passing.
    • I ran yarn test:ref instead of yarn test (which I think was previously used to create the scenario), as yarn test is failing with the following error:
    Error: Command failed: /usr/local/share/nvm/versions/node/v22.11.0/bin/node /workspaces/hardhat/packages/hardhat-core/internal/solidity/compiler/solcjs-runner.js /home/vscode/.cache/hardhat-nodejs/compilers-v2/wasm/soljson-v0.8.24+commit.e11b9ed9.js
    RuntimeError: memory access out of bounds
        at wasm://wasm/053b6712:wasm-function[330]:0x2a475
        at wasm://wasm/053b6712:wasm-function[42946]:0x135e4e6
        at ccall (/home/vscode/.cache/hardhat-nodejs/compilers-v2/wasm/soljson-v0.8.24+commit.e11b9ed9.js:110:28936)
        at /home/vscode/.cache/hardhat-nodejs/compilers-v2/wasm/soljson-v0.8.24+commit.e11b9ed9.js:110:29327
        at runWithCallbacks (/workspaces/hardhat/node_modules/.pnpm/[email protected][email protected]/node_modules/solc/bindings/compile.js:169:18)
        at Object.boundFunctionStandard [as compileStandard] (/workspaces/hardhat/node_modules/.pnpm/[email protected][email protected]/node_modules/solc/bindings/compile.js:83:20)
        at compileStandardWrapper (/workspaces/hardhat/node_modules/.pnpm/[email protected][email protected]/node_modules/solc/wrapper.js:66:24)
        at main (/workspaces/hardhat/packages/hardhat-core/internal/solidity/compiler/solcjs-runner.js:26:23)
        at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
    
    This issue might be related, but the suggested fix - of upgrading the Solidity version - didn't work for me.
    This might affect benchmark scenario times

@Wodann Wodann marked this pull request as ready for review December 16, 2024 08:52
@Wodann
Copy link
Member Author

Wodann commented Dec 16, 2024

With the fix for the benchmarks, this PR now only has issues with the Mac CI. I'll merge main into feat/multichain in a follow-up to fix this, as it has already been fixed in that branch.

@agostbiro agostbiro self-requested a review December 16, 2024 12:28
Copy link
Member

@agostbiro agostbiro left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@Wodann Wodann had a problem deploying to github-action-benchmark December 16, 2024 12:49 — with GitHub Actions Error
@Wodann Wodann temporarily deployed to github-action-benchmark December 16, 2024 13:12 — with GitHub Actions Inactive
@Wodann Wodann merged commit d7b7353 into feat/multichain Dec 16, 2024
28 of 32 checks passed
@Wodann Wodann deleted the fix/op-rpc branch December 16, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changeset needed This PR doesn't require a changeset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimisim RPC receipts have null in their l1-related fields, and some are missing
2 participants