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

fix: accept missing mix hashes in remote blocks #516

Merged
merged 4 commits into from
Jun 17, 2024

Conversation

fvictorio
Copy link
Member

See NomicFoundation/hardhat#5266 (comment)

I didn't add a test for this because neither Infura nor Alchemy support that creditcoin chain, and I don't think we should use public RPC URLs in our tests, since they don't tend to be very robust.

Copy link

changeset-bot bot commented Jun 14, 2024

🦋 Changeset detected

Latest commit: 2e40c98

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

This PR includes changesets to release 1 package
Name Type
@nomicfoundation/edr 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

@fvictorio fvictorio had a problem deploying to github-action-benchmark June 14, 2024 16:46 — with GitHub Actions Error
@fvictorio fvictorio temporarily deployed to github-action-benchmark June 14, 2024 16:48 — with GitHub Actions Inactive
@@ -79,7 +79,7 @@ impl RemoteBlock {
gas_used: block.gas_used,
timestamp: block.timestamp,
extra_data: block.extra_data,
mix_hash: block.mix_hash.ok_or(CreationError::MissingMixHash)?,
mix_hash: block.mix_hash.unwrap_or_default(),
Copy link
Member

@Wodann Wodann Jun 14, 2024

Choose a reason for hiding this comment

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

I think we should add a GitHub issue and TODO here to indicate that this should be reverted once we have multi-chain support.

Then we (or someone else) can add an implementation for this particular chain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks.

Can you re-review this? I had to solve a merge conflict and it was pretty straightforward, but it wouldn't hurt to double-check.

@Wodann Wodann self-requested a review June 14, 2024 17:48
Copy link
Member

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

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

@jeremyjams
Copy link

Thank you for the fix! Do you have any idea when you plan to release it?
When EDR is released, would going through this "process" be enough to benefit from this fix from a fresh hardhat dependency?

@fvictorio fvictorio temporarily deployed to github-action-benchmark June 17, 2024 09:56 — with GitHub Actions Inactive
@fvictorio fvictorio requested a review from Wodann June 17, 2024 09:57
@fvictorio
Copy link
Member Author

@jeremyjams I guess we'll merge this today, and might release it later this week.

When EDR is released, would NomicFoundation/hardhat#5087 (comment) be enough to benefit from this fix from a fresh hardhat dependency?

Correct.

@fvictorio fvictorio merged commit d87a8c4 into main Jun 17, 2024
36 checks passed
@fvictorio fvictorio deleted the handle-missing-mix-hash branch June 17, 2024 15:33
@jeremyjams
Copy link

Hi, do you have any update on that?

@fvictorio
Copy link
Member Author

@jeremyjams sorry for taking longer than I expected, we just released this: https://github.com/NomicFoundation/edr/releases/tag/%40nomicfoundation%2Fedr%400.4.1

To upgrade to this version immediately, you can remove your node_modules directory and npm/yarn/pnpm lock file and reinstall your dependencies. Otherwise you can wait for the next Hardhat release which will automatically use the latest EDR version.

You can verify that you’re using the latest version by running the following command: npm ls @nomicfoundation/edr.

@jeremyjams
Copy link

Hello @fvictorio no problem, thank you for this release and your previous message 🙏 (I'm sadly now facing another issue #536)

@Wodann Wodann added this to the EDR v0.4.1 milestone Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants