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

Add order commitment check on verification #21

Merged
merged 5 commits into from
Feb 16, 2024
Merged

Conversation

fedgiac
Copy link
Collaborator

@fedgiac fedgiac commented Feb 15, 2024

We want to have at most a single valid AMM order on any batch while at the same time providing the flexibility to solvers to create arbitrary orders within the constraints of the AMM function.

This PR enforces this by a commitment mechanism.
If no commitment is set, the verification function assumes that there is an implicit commitment to the order obtained from getTradeableOrder and it checks that the input order parameters match.
Any other previously valid order o can be settled in a batch as follows.

  1. The solver calls commit(ammAddress, o.hash()) in a pre-interaction.
  2. As long as the order passes the AMM checks, signature verification also passes.
  3. The solver calls commit(ammAddress, EMPTY_COMMITMENT) in a post-interaction to unset the commitment and get back some gas as refund.

It would be really nice to use TSTORE to avoid point 3 and to make this process much cheaper but we're a month too early.

Drawbacks of these changes: calling getTradeableOrder or committing means an increased gas cost when settling the order.

To do (in another PR)

Update the docs to explain the role of commitments and their intended usage.

How to test

Existing and new unit tests. Here is a test order for this code.
You can get a feeling for the gas cost by resimulating without access list. The simulation highlights that the cost of the extra call to perform the extra checks is about 31k gas.

I also tested that, assuming a commit is set and an order is possible, the watchtower doesn't stop indexing the order. (See internal logs here.) It does become noisy however.

@fedgiac fedgiac requested a review from a team February 15, 2024 14:34
Copy link
Contributor

@mfw78 mfw78 left a comment

Choose a reason for hiding this comment

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

One test isn't being executed and a nit on some typos.

src/ConstantProduct.sol Outdated Show resolved Hide resolved
test/ConstantProduct/DeploymentParamsTest.sol Show resolved Hide resolved
test/ConstantProduct/ConstantProductTestHarness.sol Outdated Show resolved Hide resolved
@fedgiac fedgiac merged commit 85f68f6 into main Feb 16, 2024
3 checks passed
@fedgiac fedgiac deleted the add-order-commitment branch February 16, 2024 09:41
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.

2 participants