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

Include necessary data for simulations within the auction json sent to solvers #2620

Merged
merged 9 commits into from
Apr 25, 2024

Conversation

m-lord-renkse
Copy link
Contributor

Description

Currently, solvers need to use the /auction endpoint to recover missing info, such as hooks, for orders in a batch auction in order to be able to simulate their proposed solution themselves. We could instead ensure that all necessary info is included in the auction json.

Changes

Data added to the auction json:

  • all values signed as part of the order data: buy_amount, sell_amount, valid_to, owner, receiver
  • pre- and post-hooks

How to test

  1. e2e tests
  2. regression tests

Related Issues

Fixes #2606

@m-lord-renkse m-lord-renkse requested a review from a team as a code owner April 15, 2024 14:17
@m-lord-renkse
Copy link
Contributor Author

I need to open a PR for the gnosis solver too.

@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch from f62523c to a4aa3ed Compare April 15, 2024 14:18
@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch from a4aa3ed to 002927c Compare April 15, 2024 14:21
@m-lord-renkse m-lord-renkse marked this pull request as draft April 15, 2024 14:23
@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch 13 times, most recently from eb3f00d to 517d0b3 Compare April 16, 2024 13:54
@m-lord-renkse m-lord-renkse marked this pull request as ready for review April 16, 2024 14:05
@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch from 517d0b3 to 96c68f3 Compare April 16, 2024 14:09
crates/solvers-dto/src/auction.rs Outdated Show resolved Hide resolved
crates/solvers-dto/src/auction.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch 3 times, most recently from 43fad78 to d24e8de Compare April 17, 2024 08:07
@m-lord-renkse m-lord-renkse marked this pull request as draft April 17, 2024 09:04
@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch 3 times, most recently from 86a8878 to 0c00d50 Compare April 17, 2024 09:14
@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch 4 times, most recently from 71728c6 to e226535 Compare April 18, 2024 09:59
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

Just minor nits remaining

crates/solvers-dto/src/auction.rs Outdated Show resolved Hide resolved
crates/driver/src/tests/cases/internalization.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch 3 times, most recently from 14ecfa0 to 235d0f9 Compare April 19, 2024 11:44
@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch from 64363b5 to b0c51db Compare April 19, 2024 12:33
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

All my comments have been addressed.
@cowprotocol/backend could somebody else please give this a review with a fresh set of eyes.

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

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

LGTM

m-lord-renkse added a commit that referenced this pull request Apr 24, 2024
…2649)

# Description
Remove `deny_unknown_fields` for auction order in solvers-dto. This way
new changes in the payload does not break the solvers. It is needed to
merge #2620 without having
downtime for the solvers.

# Changes
Remove `deny_unknown_fields` for auction order in solvers-dto.

## How to test
1. Regression tests
@m-lord-renkse m-lord-renkse force-pushed the 2606/include-necessary-data-within-auction-json branch from b0c51db to 44ec390 Compare April 24, 2024 08:14
m-lord-renkse added a commit to gnosis/solvers that referenced this pull request Apr 24, 2024
…o solvers (#18)

Include necessary data for simulations within the auction json sent to
solvers. Follow up for cowprotocol/services#2620
@m-lord-renkse m-lord-renkse enabled auto-merge (squash) April 25, 2024 06:32
@m-lord-renkse m-lord-renkse merged commit 3927bf4 into main Apr 25, 2024
9 checks passed
@m-lord-renkse m-lord-renkse deleted the 2606/include-necessary-data-within-auction-json branch April 25, 2024 06:35
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Include necessary data for simulations within the auction json sent to solvers
3 participants