Add is_winner to solver competition endpoints #3127
Conversation
m-lord-renkse
force-pushed
the
3070/add-is-winer-to-solver-competition-endpoint
branch
from
c7cde9c to
4272ac4
Compare
m-lord-renkse
force-pushed
the
3070/add-is-winer-to-solver-competition-endpoint
branch
from
4272ac4 to
fcf323b
Compare
There was a problem hiding this comment.
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, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why not just do the migration in this PR as well to have al the data correct in one go?
There was a problem hiding this comment.
@MartinquaXD see my message below: #3127 (comment)
I would love to hear your opinion. I am open to do a migration in this PR, but I assume a SQL migration is off the table due to the json characteristics 🤔
|
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. |
|
Can this PR be marked as |
|
Reminder: Please consider backward compatibility when modifying the API specification.
Caused by: |
hmm I don't really like these type of migrations on tables which are just json, because an undetected error can be fatal 😅 have we considered moving this to a normal table, or at least move the json to a versioned json? With a versioned json we can have a custom deserialization, and it will deserialize back according to the json version stored. If this is not an option, how do we usually do these type of migrations? this would need a binary. |
| @@ -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, | |||
There was a problem hiding this comment.
Why not just do the migration in this PR as well to have al the data correct in one go?
In this specific case it should be fairly straight forward to test.
Actually yes. @sunce86 did a bunch of refactoring work to store this information in regular tables. I'm not why the need of adding this suddenly arose but technically we could also focus on reading the relevant data from the new tables and only add the value there.
So far we didn't have tables which needed to be deserialized from different formats. Meaning we basically forwarded the JSON inside the column as is and didn't parse it in the backend after reading it out. If we actually require more guarantees on the stability of the format versioning would be a good idea. 👍 |
|
Yes, we have Actual populating of historic data for Important notice is that after this PR we wont have properly populated |
|
Pending to merge until @anxolin gives me the green light. |
m-lord-renkse
deleted the
3070/add-is-winer-to-solver-competition-endpoint
branch
Description
Add is_winner to solver competition endpoints
Changes
Add is_winner boolean to each solution json object.
How to test
Related Issues
Fixes #3070