-
Notifications
You must be signed in to change notification settings - Fork 83
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
Reject orders using too much gas #2551
Conversation
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.
LG. Regarding the considerations, maybe we should analyze how often the error occurs once the change is released.
@@ -1187,6 +1187,7 @@ components: | |||
ZeroAmount, | |||
IncompatibleSigningScheme, | |||
TooManyLimitOrders, | |||
TooMuchGas, |
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.
@mfw78 this introduces a new error type the watchtower needs to know about to not get stuck indexing these orders, right?
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.
Description
We are seeing inefficiency with some ERC1271 orders on Gnosis that are using a large amount of gas reducing settlement throughput for other traders. This PR limits the amount of gas an order can consume (including signature validation and hooks)
Changes
Considerations:
Both of these would make the PR more involved while not directly addressing the somewhat pressing issue we have on Gnosis Chain.
How to test
Test for hooks. I'm also working on a test for ERC1271 orders, however it's a bit more involved because we need a fake verifier contract that uses a lot of gas