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

Reveal and settle to receive integer solution id #3152

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Dec 9, 2024

Description

A follow up for #3071

This PR allows reference driver to receive both integer and strings for solution id, on /reveal and /settle endpoints. Note how this PR implements task 2 from the linked pull request. This is done in this PR as a separate step to give time to colocated solvers using reference driver to update on time.

If we were to just switch to using integer, colocated drivers would incur downtime.

Next step is final step to use integer ONLY in both autopilot and driver, once we made sure all colocated solvers updated their driver.

Changes

  • Driver able to receive both integer and string on /reveal and /settle
  • ...

How to test

The code is copy paste of already tested code.

@sunce86 sunce86 self-assigned this Dec 9, 2024
@sunce86 sunce86 requested a review from a team as a code owner December 9, 2024 14:50
@@ -15,3 +15,34 @@ pub(super) use {
settle::settle,
solve::{solve, AuctionError},
};

pub fn deserialize_solution_id<'de, D>(deserializer: D) -> Result<u64, D::Error>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicated code, couldn't we move it to some utils crate? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not worth the effort since it will be removed again.

@@ -15,3 +15,34 @@ pub(super) use {
settle::settle,
solve::{solve, AuctionError},
};

pub fn deserialize_solution_id<'de, D>(deserializer: D) -> Result<u64, D::Error>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn deserialize_solution_id<'de, D>(deserializer: D) -> Result<u64, D::Error>
pub(crate) fn deserialize_solution_id<'de, D>(deserializer: D) -> Result<u64, D::Error>

So the compiler helps us to remember cleaning up this function when we stop using it.

@sunce86 sunce86 enabled auto-merge (squash) December 10, 2024 10:15
@sunce86 sunce86 merged commit 2bb146f into main Dec 10, 2024
10 of 11 checks passed
@sunce86 sunce86 deleted the update-driver-to-receive-integer-solution-id branch December 10, 2024 10:21
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 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.

3 participants