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

Conditional Limit Order type #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conditional Limit Order type #82

wants to merge 1 commit into from

Conversation

fleupold
Copy link
Contributor

@fleupold fleupold commented Jan 5, 2024

Description

This PR introduces a conditional limit order type. It's not so much conditional, rather partially unconditionally, namely it doesn't care about what appDataHash the suggested trade has set. Instead it only defines some other key parameter of the order (tokens, amounts, validity, receiver) to provide basically the same security guarantees as regular CoW orders.

The advantage of not committing to an app data hash up front, is that it allows the order placer to specify arbitrary pre and post hooks on behalf of the user.
This can be useful if for instance the pre-hook depends on the address of the safe, which may depend the app data used (e.g. if the safe is only being created as part of the pre hook and wants to create the order as part of the initializer call).
In such a case we cannot know the pre-hook before knowing the address of the safe, but need would have to commit to a concrete app data before knowing the address of the Safe.

This order type breaks this circular dependency by removing the app data from the order parameters that need to be committed to up front and instead allowing any app data to be valid (as long as the other important parameters are respected).

Security wise this shouldn't be of huge concern as hooks aren't executed in a privileged context. The only griefing vector I see would be expensive pre-hook invocations benefiting the actor that placed the order on behalf of the safe. This would result in high fees, but since fees are part of the limit order check already, I don't think this is an issue in practice (any other party could place an order without such interactions which should be preferred by solvers as it generates more surplus).

Changes

  • Add Order type
  • Add test coverage for new order type

How to test

Unit tests

Copy link

github-actions bot commented Jan 5, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@fleupold
Copy link
Contributor Author

fleupold commented Jan 5, 2024

I have read the CLA Document and I hereby sign the CLA

@mfw78 mfw78 requested a review from a team January 12, 2024 18:48
github-actions bot added a commit that referenced this pull request Jan 12, 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.

1 participant