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

Multiple winners in autopilot #2996

Merged
merged 16 commits into from
Oct 4, 2024
Merged

Multiple winners in autopilot #2996

merged 16 commits into from
Oct 4, 2024

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Sep 16, 2024 •
edited
Loading

Description

Fixes #2994
Fixes #2995
Fixes #2998

  • Adds configuration parameter max_winners_per_auction to enable/disable multiple winners feature.
  • Chooses the winners based on the max_winners_per_auction.
  • Then issues multiple settle calls for each of the winners.

The only remaining part to implement is competition saving but this is captured with #3021

@sunce86 sunce86 added the E:6.2 Time to Happy Moo See https://github.com/cowprotocol/pm/issues/77 for details label Sep 16, 2024
@sunce86 sunce86 self-assigned this Sep 16, 2024
@sunce86 sunce86 requested a review from a team as a code owner September 16, 2024 12:29
@sunce86 sunce86 mentioned this pull request Sep 16, 2024
Closed
fleupold
fleupold reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

I don't think this PR is very useful by itself (it will get stale before it can be made useful by actually using the new parameter).

@sunce86
Copy link
Contributor Author

sunce86 commented Sep 16, 2024

I don't think this PR is very useful by itself (it will get stale before it can be made useful by actually using the new parameter).

If you think it's shouldn't be merged unless it's used, then I will implement #2995 inside this PR.

@sunce86 sunce86 changed the title [EASY] Add configuration for multiple winners [DRAFT] Multiple winners in autopilot Sep 18, 2024
@sunce86 sunce86 changed the title [DRAFT] Multiple winners in autopilot Multiple winners in autopilot Sep 27, 2024
@squadgazzz
Copy link
Contributor

Regarding the current PR, I don't see any blockers from the enabled parallel auction preparation/solving side.

sunce86
sunce86 commented Sep 30, 2024
View reviewed changes
crates/autopilot/src/infra/solvers/dto/solve.rs
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into a separate function because now it's called in two places (runloop and shadow)

@sunce86 sunce86 requested review from fleupold, squadgazzz and a team September 30, 2024 09:54
MartinquaXD
MartinquaXD reviewed Sep 30, 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.

It's not really clear why we reversed the solution ordering. Unless that is required for some reason I would prefer if we do not touch it unnecessarily.

crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved
crates/autopilot/src/shadow.rs Show resolved Hide resolved
crates/autopilot/src/shadow.rs Outdated Show resolved Hide resolved
crates/autopilot/src/shadow.rs Outdated Show resolved Hide resolved
crates/autopilot/src/run_loop.rs Show resolved Hide resolved
@sunce86
Copy link
Contributor Author

sunce86 commented Sep 30, 2024 •
edited
Loading

It's not really clear why we reversed the solution ordering. Unless that is required for some reason I would prefer if we do not touch it unnecessarily.

Few reasons why I changed this:

  1. Let's assume autopilot domain doesn't care how the solutions are stored in database. What would be the most logical ordering in the domain? I'd say winners.first() is the global winner, and for iterating we use winners.iter(). It's not natural to me to use winners.last() as the winner, nor I would expect this to be easily understood by external contributor looking at the code for the first time, for example.
  2. If we use worst->best ordering, then we have to iterate in reverse in a bunch of places which makes the code less readable.

Being a risky change is not a good reason IMO to not touch some part of the code.

Can you name reasons why we shouldn't touch it?

@sunce86 sunce86 mentioned this pull request Oct 3, 2024
Open
MartinquaXD
MartinquaXD reviewed Oct 3, 2024
View reviewed changes
crates/autopilot/src/run_loop.rs Show resolved Hide resolved
crates/autopilot/src/shadow.rs Outdated Show resolved Hide resolved
crates/autopilot/src/run_loop.rs
@@ -514,11 +525,14 @@ impl RunLoop {

// Shuffle so that sorting randomly splits ties.
solutions.shuffle(&mut rand::thread_rng());
solutions.sort_unstable_by_key(|participant| participant.solution.score().get().0);
solutions.sort_unstable_by_key(|participant| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like shuffling and sorting by score would also fit into the shared function to pick a winner.
That way the entire core decision making is shared correctly across shadow and regular autopilot.

MartinquaXD
MartinquaXD approved these changes Oct 3, 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.

It pains a bit to approve this without the autopilot refactor but at this point it makes more sense to merge this first even with the complex code duplication.

crates/autopilot/src/shadow.rs Outdated Show resolved Hide resolved
crates/autopilot/src/run_loop.rs Show resolved Hide resolved
@sunce86 sunce86 requested a review from squadgazzz October 4, 2024 07:42
squadgazzz
squadgazzz approved these changes Oct 4, 2024
View reviewed changes
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.

Lg. The logic behind reversing the solutions back and forth looks a bit confusing, but I couldn't currently come up with a suggestion on how to avoid that.

@sunce86
Copy link
Contributor Author

sunce86 commented Oct 4, 2024

Lg. The logic behind reversing the solutions back and forth looks a bit confusing, but I couldn't currently come up with a suggestion on how to avoid that.

True. However, ranking logic in the autopilot domain needs to be clear, and then all reversing needs to be abstracted under persistence layer. Will be tackled in #3021

@sunce86 sunce86 merged commit 416c767 into main Oct 4, 2024
11 checks passed
@sunce86 sunce86 deleted the add-config-for-multiple-winners branch October 4, 2024 08:38
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fhenneke fhenneke fhenneke left review comments

@squadgazzz squadgazzz squadgazzz approved these changes

@MartinquaXD MartinquaXD MartinquaXD approved these changes

@fleupold fleupold Awaiting requested review from fleupold

@m-lord-renkse m-lord-renkse Awaiting requested review from m-lord-renkse

Assignees

@sunce86 sunce86

Labels
E:6.2 Time to Happy Moo See https://github.com/cowprotocol/pm/issues/77 for details
Projects
None yet
Milestone
No milestone
6 participants
@sunce86 @squadgazzz @fleupold @fhenneke @MartinquaXD @m-lord-renkse