-
Notifications
You must be signed in to change notification settings - Fork 3.9k
YAS 430 OVM gasLimit-proportional burn for rate limiting #217
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
Conversation
This reverts commit 3ecebe8.
willmeister
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
| return keccak256(abi.encodePacked( | ||
| _batchHeader.timestamp, | ||
| _batchHeader.isL1ToL2Tx, | ||
| _batchHeader.isL1ToL2Tx, // TODO REPLACE WITH QUEUE ORIGIN (if you are a PR reviewer please lmk!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️
| // TODO: figure out how we're going to authenticate this | ||
| return true; | ||
| // return _sender != tx.origin; | ||
| require(msg.sender == address(resolveCanonicalTransactionChain()), "Only the canonical transaction chain can dequeue L1->L2 queue transactions."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
For future reference, discussion on the |
* migration: Add tests for state migration * migration: Fix issues shown by tests * migration: pass allowlist into state migration Allows for easier testing * migration: Add test with allowlist * Correct overwrite counter * Use in memory DB
* migration: Add tests for state migration * migration: Fix issues shown by tests * migration: pass allowlist into state migration Allows for easier testing * migration: Add test with allowlist * Correct overwrite counter * Use in memory DB
* feat: make L1ERC721Bridge configurable in L1Block * feat: remove initializable from ERC721Bridge & L2ERC721Bridge * chore: update L2Genesis script * feat: update L1ERC721Bridge to make it initializable * chore: update Setup * chore: pre-pr ready * test: move common assertions out of conditional
* fix: pipeline builder * feat: pipeline builder * fix: use online provider impls * fix: feature flag builder example
related issues paradigmxyz/reth#12275 <!-- Thank you for your Pull Request. Please provide a description above and review the requirements below. Bug fixes and new features should include tests. Contributors guide: https://github.com/alloy-rs/core/blob/main/CONTRIBUTING.md The contributors guide includes instructions for running rustfmt and building the documentation. --> <!-- ** Please select "Allow edits from maintainers" in the PR Options ** --> ## Motivation <!-- Explain the context and why you're making that change. What is the problem you're trying to solve? In some cases there is not a problem and this can be thought of as being the motivation for your change. --> ## Solution <!-- Summarize the solution and provide any necessary context needed to understand the code change. --> ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes --------- Co-authored-by: Matthias Seitz <[email protected]>
Closes #217 Also fixed a bug where the `set_earliest_block_number_hash` was using `append` instead of `upsert`. --------- Co-authored-by: Arun Dhyani <[email protected]>
Description
This PR covers YAS 430, which adds a gas rate limiter to the Safety and L1->L2 queues. It accomplishes this by requiring that you burn a proportional amount of gas on L1 based on the transaction's OVM gasLimit.
NOTE: This PR builds off #212 , so I have made the PR against that branch--diff will be much more readable this way as merging #212 is currently blocked.
Questions
Metadata
Fixes
Contributing Agreement