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.

Sign up for GitHub

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

Jump to bottom

Driver: Include non-surplus-capturing-jit-orders in the driver /solve #3048

Merged
merged 17 commits into from
Oct 15, 2024
Merged

Driver: Include non-surplus-capturing-jit-orders in the driver /solve #3048

merged 17 commits into from
Oct 15, 2024

Conversation

m-lord-renkse
Copy link
Contributor

@m-lord-renkse m-lord-renkse commented Oct 10, 2024 •
edited
Loading

Description

Include the non-surplus-capturing JIT orders in the driver /solve response. For more context, please see #3004

Changes

  • Include non-surplus-capturing JIT orders in the driver /solve response
  • Do not use the clearing prices for such orders, but the JIT order executed priced (since it is also used in the encoding)

How to test

  1. Driver tests

Related Issues

Fixes #3004

m-lord-renkse
m-lord-renkse commented Oct 10, 2024
View reviewed changes
crates/driver/src/domain/competition/solution/settlement.rs Outdated
};
acc.insert(trade.uid(), order);
}
Trade::Jit(_) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we are skipping the clearing prices for ALL JIT orders, I am not sure it is correct, or it is has to be only for non-surplus-capturing ones.

@m-lord-renkse m-lord-renkse force-pushed the 3004/driver-include-no-capturing-jit-order-in-solved branch from 46d980d to 8cbe5ec Compare October 10, 2024 11:19
@m-lord-renkse m-lord-renkse marked this pull request as ready for review October 10, 2024 11:24
@m-lord-renkse m-lord-renkse requested a review from a team as a code owner October 10, 2024 11:24
@sunce86 sunce86 added the E:6.2 Time to Happy Moo See https://github.com/cowprotocol/pm/issues/77 for details label Oct 10, 2024
@sunce86
Copy link
Contributor

sunce86 commented Oct 10, 2024

Added time-to-happy-moo label as this fix is important for proper selection of winners in autopilot.

MartinquaXD
MartinquaXD reviewed Oct 10, 2024
View reviewed changes
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.

AFAICS the issue did not mention to alter the meaning of what it means for a solution to be non-empty which seems like the source of at least a portion of the diff.
My understanding is that the only goal is to make solvers report JIT orders even if they don't increase the score and not propagate solutions with potentially 0 score.

crates/driver/src/domain/competition/solution/mod.rs Outdated Show resolved Hide resolved
crates/driver/src/tests/setup/solver.rs Show resolved Hide resolved
@squadgazzz
Copy link
Contributor

Would it be possible to specify the owners list while building a fake auction for the quotes?

services/crates/driver/src/domain/quote.rs

Line 186 in 8cbe5ec

HashSet::default(),

This is required for another PR(#3038) to avoid some ugly workaround.

@m-lord-renkse
Copy link
Contributor Author

Would it be possible to specify the owners list while building a fake auction for the quotes?

services/crates/driver/src/domain/quote.rs

Line 186 in 8cbe5ec

HashSet::default(),

This is required for another PR(#3038) to avoid some ugly workaround.

@squadgazzz Yeah, definitely, but it is totally unrelated to this PR. I can address it in another PR if you want 👌

@m-lord-renkse m-lord-renkse requested a review from sunce86 October 11, 2024 10:39
@squadgazzz
Copy link
Contributor

Yeah, definitely, but it is totally unrelated to this PR. I can address it in another PR if you want 👌

Ok, NVM, I'll rebase my PR once this one is merged.

@m-lord-renkse m-lord-renkse requested a review from sunce86 October 11, 2024 12:11
@m-lord-renkse m-lord-renkse force-pushed the 3004/driver-include-no-capturing-jit-order-in-solved branch from a83e017 to fc59ea6 Compare October 11, 2024 12:14
@m-lord-renkse m-lord-renkse marked this pull request as draft October 14, 2024 07:10
@m-lord-renkse m-lord-renkse marked this pull request as ready for review October 14, 2024 07:59
sunce86
sunce86 reviewed Oct 14, 2024
View reviewed changes
Copy link
Contributor

@sunce86 sunce86 left a comment

Choose a reason for hiding this comment

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

Almost there

crates/driver/src/domain/competition/solution/trade.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/settlement.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 3004/driver-include-no-capturing-jit-order-in-solved branch from f384174 to 48c5aad Compare October 14, 2024 11:36
@m-lord-renkse m-lord-renkse force-pushed the 3004/driver-include-no-capturing-jit-order-in-solved branch from 48c5aad to 302d53f Compare October 14, 2024 11:43
@sunce86 sunce86 mentioned this pull request Oct 14, 2024
Merged
sunce86 added a commit that referenced this pull request Oct 14, 2024
@sunce86
Remove Liquidity orders from Driver (#3058)
ee6423e 
# Description
With #2961, we made sure
only Limit orders are sent to driver for solving. For quoting, driver
still uses Market orders, and removing this is captured with
#2543, and resolving this
would allow us to completely remove `class` type in driver.

But for now, I think we don't use Liquidity type for anything. Liquidity
orders are not part of the Auction, so solvers also cant refer to them
in their solutions, so I think removing them is not even a breaking
change.

Still checking if we want to remove them from the solvers API as well.
edit: went down a rabit hole of removing from solvers API and since the
code change is huge, will do it in a separate PR.

Would allow for cleaner implementation of
#3048

## How to test
Existing tests.
sunce86
sunce86 approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@sunce86 sunce86 left a comment

Choose a reason for hiding this comment

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

LGTM. Nice

crates/driver/openapi.yml Show resolved Hide resolved
crates/driver/src/domain/competition/solution/settlement.rs Outdated Show resolved Hide resolved
MartinquaXD
MartinquaXD approved these changes Oct 15, 2024
View reviewed changes
crates/driver/src/tests/setup/mod.rs Outdated Show resolved Hide resolved
crates/driver/src/tests/setup/mod.rs Outdated Show resolved Hide resolved
crates/driver/src/tests/setup/solver.rs Outdated Show resolved Hide resolved
crates/driver/src/domain/competition/solution/settlement.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 3004/driver-include-no-capturing-jit-order-in-solved branch 2 times, most recently from 06596b8 to 9069596 Compare October 15, 2024 09:52
@m-lord-renkse m-lord-renkse force-pushed the 3004/driver-include-no-capturing-jit-order-in-solved branch from 9069596 to b31da69 Compare October 15, 2024 09:54
@m-lord-renkse m-lord-renkse requested a review from sunce86 October 15, 2024 09:57
@m-lord-renkse m-lord-renkse enabled auto-merge (squash) October 15, 2024 14:22
@m-lord-renkse m-lord-renkse merged commit a17e419 into main Oct 15, 2024
11 checks passed
@m-lord-renkse m-lord-renkse deleted the 3004/driver-include-no-capturing-jit-order-in-solved branch October 15, 2024 14:26
@github-actions github-actions bot locked and limited conversation to collaborators Oct 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MartinquaXD MartinquaXD MartinquaXD approved these changes

@sunce86 sunce86 sunce86 approved these changes

@squadgazzz squadgazzz Awaiting requested review from squadgazzz

Assignees
No one assigned
Labels
E:6.2 Time to Happy Moo See https://github.com/cowprotocol/pm/issues/77 for details
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Report and store all executed orders during bidding
4 participants
@m-lord-renkse @sunce86 @squadgazzz @MartinquaXD