-
Notifications
You must be signed in to change notification settings - Fork 2
Add ArbFilteredTransactionsManager precompile #25
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
Add ArbFilteredTransactionsManager precompile #25
Conversation
3e44be1 to
682dbab
Compare
a3ff4ab to
33abc0f
Compare
101d31e to
3033065
Compare
diegoximenes
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, assigning to @MishkaRogachev to merge, or to assign to someone with merging power, when nitro PR is ready to be merged.
gzeoneth
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, small comments
ArbOwnerPublic.sol
Outdated
| address filterer | ||
| ) external view returns (bool); | ||
|
|
||
| /// @notice Retrieves the list of transaction filterers |
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.
| /// @notice Retrieves the list of transaction filterers | |
| /// @notice Retrieves the list of transaction filterers |
ArbOwner.sol
Outdated
| address filterer | ||
| ) external view returns (bool); | ||
|
|
||
| /// @notice Retrieves the list of transaction filterers |
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.
| /// @notice Retrieves the list of transaction filterers | |
| /// @notice Retrieves the list of transaction filterers |
| /// @notice Checks whether the given transaction hash is filtered | ||
| /// @param txHash The transaction hash to check | ||
| /// @return True if filtered, false otherwise | ||
| function isTransactionFiltered( |
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.
is this going to be public or only usable by authorized filterer?
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.
Since there is an event that allows this information to be obtained, I think the method should be public so that there is no ambiguity
|
@MishkaRogachev, is this one also ready for a final review? |
Part of NIT-4245
pulled in by OffchainLabs/nitro#4174
Changes: