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 is_winner to solver competition endpoints #3127

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

Conversation

m-lord-renkse
Copy link
Contributor

Description

Add is_winner to solver competition endpoints

Changes

Add is_winner boolean to each solution json object.

How to test

  1. e2e test
  2. Regression test

Related Issues

Fixes #3070

@m-lord-renkse m-lord-renkse force-pushed the 3070/add-is-winer-to-solver-competition-endpoint branch from c7cde9c to 4272ac4 Compare November 14, 2024 15:45
@m-lord-renkse m-lord-renkse force-pushed the 3070/add-is-winer-to-solver-competition-endpoint branch from 4272ac4 to fcf323b Compare November 15, 2024 08:58
@m-lord-renkse m-lord-renkse marked this pull request as ready for review November 15, 2024 09:03
@m-lord-renkse m-lord-renkse requested a review from a team as a code owner November 15, 2024 09:03
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.

I think we are missing some kind of migration to populate the value for historic entries - otherwise those will be false for all solutions in an auction.

This migration can be done manually after this PR is merged as well. Simply, go over entries in solver_competition and set is_winner=true for all last solutions.

@@ -54,6 +54,7 @@ pub struct SolverSettlement {
#[serde_as(as = "BTreeMap<_, HexOrDecimalU256>")]
pub clearing_prices: BTreeMap<H160, U256>,
pub orders: Vec<Order>,
pub is_winner: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add #[serde(default)] above the new field to make it explicit that is_winner might be missing for some entries and to make the deserialization more robust.

@@ -1504,6 +1504,9 @@ components:
description: Maps from solver name to object describing that solver's settlement.
items:
$ref: "#/components/schemas/SolverSettlement"
isWinner:
type: boolean
description: whether the solution is a winner or not
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong place, should go under SolverSettlement

Copy link

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

@squadgazzz
Copy link
Contributor

Can this PR be marked as Draft to avoid redundant notifications until all the comments are addressed?

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.

feat: Add is_winner flag to solver competition endpoint
3 participants