Skip to content

Commit

Permalink
Stop using string for solution_id (#3154)
Browse files Browse the repository at this point in the history
# Description
Follow up for #3152, and a
final step to switch to using INTEGER for solution id only.

Will be merged on Friday, once all solvers confirm they updated their
drivers to include #3152.

OPENAPI is already up to date.

Fixes #3072

# Changes
<!-- List of detailed changes (how the change is accomplished) -->

- [ ] Stop accepting string type for solution id

## How to test
e2e tests

<!--
## Related Issues

Fixes #
-->
  • Loading branch information
sunce86 authored Dec 19, 2024
1 parent fe99739 commit 796e7bd
Show file tree
Hide file tree
Showing 14 changed files with 37 additions and 110 deletions.
1 change: 0 additions & 1 deletion crates/autopilot/src/infra/solvers/dto/reveal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use {
#[serde(rename_all = "camelCase")]
pub struct Request {
/// Unique ID of the solution (per driver competition), to reveal.
#[serde_as(as = "serde_with::DisplayFromStr")]
pub solution_id: u64,
/// Auction ID in which the specified solution ID is competing.
#[serde_as(as = "Option<serde_with::DisplayFromStr>")]
Expand Down
1 change: 0 additions & 1 deletion crates/autopilot/src/infra/solvers/dto/settle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use {
#[serde(rename_all = "camelCase")]
pub struct Request {
/// Unique ID of the solution (per driver competition), to settle.
#[serde_as(as = "serde_with::DisplayFromStr")]
pub solution_id: u64,
/// The last block number in which the solution TX can be included
pub submission_deadline_latest_block: u64,
Expand Down
32 changes: 0 additions & 32 deletions crates/autopilot/src/infra/solvers/dto/solve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ pub enum Side {
pub struct Solution {
/// Unique ID of the solution (per driver competition), used to identify
/// it in subsequent requests (reveal, settle).
#[serde(deserialize_with = "deserialize_solution_id")]
pub solution_id: u64,
#[serde_as(as = "HexOrDecimalU256")]
pub score: U256,
Expand All @@ -182,37 +181,6 @@ pub struct Solution {
pub gas: Option<u64>,
}

fn deserialize_solution_id<'de, D>(deserializer: D) -> Result<u64, D::Error>
where
D: serde::Deserializer<'de>,
{
struct SolutionIdVisitor;

impl serde::de::Visitor<'_> for SolutionIdVisitor {
type Value = u64;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
formatter.write_str("a string or integer representing a solution ID")
}

fn visit_u64<E>(self, value: u64) -> Result<u64, E>
where
E: serde::de::Error,
{
Ok(value)
}

fn visit_str<E>(self, value: &str) -> Result<u64, E>
where
E: serde::de::Error,
{
value.parse::<u64>().map_err(serde::de::Error::custom)
}
}

deserializer.deserialize_any(SolutionIdVisitor)
}

#[derive(Clone, Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct Response {
Expand Down
31 changes: 0 additions & 31 deletions crates/driver/src/infra/api/routes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,34 +15,3 @@ pub(super) use {
settle::settle,
solve::{solve, AuctionError},
};

pub(crate) fn deserialize_solution_id<'de, D>(deserializer: D) -> Result<u64, D::Error>
where
D: serde::Deserializer<'de>,
{
struct SolutionIdVisitor;

impl serde::de::Visitor<'_> for SolutionIdVisitor {
type Value = u64;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
formatter.write_str("a string or integer representing a solution ID")
}

fn visit_u64<E>(self, value: u64) -> Result<u64, E>
where
E: serde::de::Error,
{
Ok(value)
}

fn visit_str<E>(self, value: &str) -> Result<u64, E>
where
E: serde::de::Error,
{
value.parse::<u64>().map_err(serde::de::Error::custom)
}
}

deserializer.deserialize_any(SolutionIdVisitor)
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
use {super::super::super::deserialize_solution_id, serde::Deserialize, serde_with::serde_as};
use {serde::Deserialize, serde_with::serde_as};

#[serde_as]
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct RevealRequest {
/// Unique ID of the solution (per driver competition), to reveal.
#[serde(deserialize_with = "deserialize_solution_id")]
pub solution_id: u64,
/// Auction ID in which the specified solution ID is competing.
#[serde_as(as = "Option<serde_with::DisplayFromStr>")]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
use {super::super::super::deserialize_solution_id, serde::Deserialize, serde_with::serde_as};
use {serde::Deserialize, serde_with::serde_as};

#[serde_as]
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct SettleRequest {
/// Unique ID of the solution (per driver competition), to settle.
#[serde(deserialize_with = "deserialize_solution_id")]
pub solution_id: u64,
/// The last block number in which the solution TX can be included
pub submission_deadline_latest_block: u64,
Expand Down
2 changes: 1 addition & 1 deletion crates/driver/src/tests/cases/buy_eth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,5 @@ async fn test() {
.await;

let id = test.solve().await.ok().orders(&[order]).id();
test.settle(&id).await.ok().await.eth_order_executed().await;
test.settle(id).await.ok().await.eth_order_executed().await;
}
12 changes: 6 additions & 6 deletions crates/driver/src/tests/cases/merge_settlements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ async fn possible() {
.await;

let id = test.solve().await.ok().orders(&[ab_order, cd_order]).id();
test.reveal(&id).await.ok().calldata();
test.settle(&id)
test.reveal(id).await.ok().calldata();
test.settle(id)
.await
// Even though the solver returned two solutions, the executed settlement is a
// combination of the two, meaning the settlements were merged successfully.
Expand Down Expand Up @@ -91,8 +91,8 @@ async fn impossible() {

// Only the first A-B order gets settled.
let id = test.solve().await.ok().orders(&[order]).id();
test.reveal(&id).await.ok().calldata();
test.settle(&id).await.ok().await.ab_order_executed().await;
test.reveal(id).await.ok().calldata();
test.settle(id).await.ok().await.ab_order_executed().await;
}

/// Test that mergable solutions don't get merged if feature was not enabled.
Expand All @@ -114,6 +114,6 @@ async fn possible_but_forbidden() {
// Even though the solutions could be combined (see test "possible") they were
// not because solution merging is not enabled by default.
let id = test.solve().await.ok().orders(&[ab_order]).id();
test.reveal(&id).await.ok().calldata();
test.settle(&id).await.ok().await.ab_order_executed().await;
test.reveal(id).await.ok().calldata();
test.settle(id).await.ok().await.ab_order_executed().await;
}
4 changes: 2 additions & 2 deletions crates/driver/src/tests/cases/multiple_solutions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ async fn valid() {
.await;

let id = test.solve().await.ok().orders(&[order]).id();
test.reveal(&id).await.ok().calldata();
test.reveal(id).await.ok().calldata();
}

/// Test that the invalid solution is discarded when the /solve endpoint
Expand All @@ -36,5 +36,5 @@ async fn invalid() {
.await;

let id = test.solve().await.ok().orders(&[order]).id();
test.reveal(&id).await.ok().calldata();
test.reveal(id).await.ok().calldata();
}
35 changes: 15 additions & 20 deletions crates/driver/src/tests/cases/parallel_auctions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,25 @@ async fn driver_handles_solutions_based_on_id() {

// calling `/reveal` or `/settle` with incorrect solution ids
// results in an error.
test.settle("99").await.err().kind("SolutionNotAvailable");
test.reveal("99").await.err().kind("SolutionNotAvailable");
test.settle(99).await.err().kind("SolutionNotAvailable");
test.reveal(99).await.err().kind("SolutionNotAvailable");

// calling `/reveal` or `/settle` with a reasonable id works
// but wrong auction id results in an error.
test.set_auction_id(100);
test.reveal(&solution_id)
test.reveal(solution_id)
.await
.err()
.kind("SolutionNotAvailable");
test.settle(&solution_id)
test.settle(solution_id)
.await
.err()
.kind("SolutionNotAvailable");

// calling `/reveal` or `/settle` with a reasonable id works.
test.set_auction_id(1);
test.reveal(&solution_id).await.ok();
test.settle(&solution_id)
test.reveal(solution_id).await.ok();
test.settle(solution_id)
.await
.ok()
.await
Expand All @@ -52,11 +52,11 @@ async fn driver_handles_solutions_based_on_id() {

// calling `/reveal` or `/settle` with for a legit solution that
// has already been settled also fails.
test.settle(&solution_id)
test.settle(solution_id)
.await
.err()
.kind("SolutionNotAvailable");
test.reveal(&solution_id)
test.reveal(solution_id)
.await
.err()
.kind("SolutionNotAvailable");
Expand Down Expand Up @@ -89,12 +89,7 @@ async fn driver_can_settle_old_solutions() {
// Technically this is not super convincing since all remembered solutions
// are identical but this is the best we are going to get without needing
// to heavily modify the testing framework.
test.settle(&id1)
.await
.ok()
.await
.eth_order_executed()
.await;
test.settle(id1).await.ok().await.eth_order_executed().await;
}

/// Tests that the driver only remembers a relatively small number of solutions.
Expand All @@ -118,12 +113,12 @@ async fn driver_has_a_short_memory() {
let id6 = test.solve().await.ok().id();

// recalling the 5 most recent solutions works
test.reveal(&id2).await.ok();
test.reveal(&id3).await.ok();
test.reveal(&id4).await.ok();
test.reveal(&id5).await.ok();
test.reveal(&id6).await.ok();
test.reveal(id2).await.ok();
test.reveal(id3).await.ok();
test.reveal(id4).await.ok();
test.reveal(id5).await.ok();
test.reveal(id6).await.ok();

// recalling an older solution doesn't work
test.reveal(&id1).await.err().kind("SolutionNotAvailable");
test.reveal(id1).await.err().kind("SolutionNotAvailable");
}
8 changes: 4 additions & 4 deletions crates/driver/src/tests/cases/settle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ async fn matrix() {
.await;

let id = test.solve().await.ok().id();
test.settle(&id).await.ok().await.ab_order_executed().await;
test.settle(id).await.ok().await.ab_order_executed().await;
}
}
}
Expand All @@ -47,7 +47,7 @@ async fn solution_not_available() {
.done()
.await;

test.settle("99").await.err().kind("SolutionNotAvailable");
test.settle(99).await.err().kind("SolutionNotAvailable");
}

/// Checks that settlements with revert risk are not submitted via public
Expand All @@ -71,7 +71,7 @@ async fn private_rpc_with_high_risk_solution() {

let id = test.solve().await.ok().id();
// Public cannot be used and private RPC is not available
test.settle(&id).await.err().kind("FailedToSubmit");
test.settle(id).await.err().kind("FailedToSubmit");
}

#[tokio::test]
Expand Down Expand Up @@ -108,5 +108,5 @@ async fn high_gas_limit() {
.execute("evm_setBlockGasLimit", vec![serde_json::json!(9_000_000)])
.await
.unwrap();
test.settle(&id).await.ok().await;
test.settle(id).await.ok().await;
}
2 changes: 1 addition & 1 deletion crates/driver/src/tests/cases/solver_balance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ async fn test_just_enough_funded() {
.await;

let id = test.solve_with_solver("barely_funded").await.ok().id();
test.settle_with_solver("barely_funded", &id)
test.settle_with_solver("barely_funded", id)
.await
.ok()
.await;
Expand Down
4 changes: 2 additions & 2 deletions crates/driver/src/tests/setup/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ pub fn solve_req(test: &Test) -> serde_json::Value {
}

/// Create a request for the driver /reveal endpoint.
pub fn reveal_req(solution_id: &str, auction_id: &str) -> serde_json::Value {
pub fn reveal_req(solution_id: u64, auction_id: &str) -> serde_json::Value {
json!({
"solutionId": solution_id,
"auctionId": auction_id,
Expand All @@ -157,7 +157,7 @@ pub fn reveal_req(solution_id: &str, auction_id: &str) -> serde_json::Value {
/// Create a request for the driver /settle endpoint.
pub fn settle_req(
submission_deadline_latest_block: u64,
solution_id: &str,
solution_id: u64,
auction_id: &str,
) -> serde_json::Value {
json!({
Expand Down
9 changes: 4 additions & 5 deletions crates/driver/src/tests/setup/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1039,7 +1039,7 @@ impl Test {
}

/// Call the /reveal endpoint.
pub async fn reveal(&self, solution_id: &str) -> Reveal {
pub async fn reveal(&self, solution_id: u64) -> Reveal {
let res = self
.client
.post(format!(
Expand Down Expand Up @@ -1089,11 +1089,11 @@ impl Test {
}

/// Call the /settle endpoint.
pub async fn settle(&self, solution_id: &str) -> Settle {
pub async fn settle(&self, solution_id: u64) -> Settle {
self.settle_with_solver(solver::NAME, solution_id).await
}

pub async fn settle_with_solver(&self, solver_name: &str, solution_id: &str) -> Settle {
pub async fn settle_with_solver(&self, solver_name: &str, solution_id: u64) -> Settle {
/// The maximum number of blocks to wait for a settlement to appear on
/// chain.
const SUBMISSION_DEADLINE: u64 = 3;
Expand Down Expand Up @@ -1213,13 +1213,12 @@ impl SolveOk<'_> {

/// Extracts the solution id from the response. Since response can contain
/// multiple solutions, it takes the id from the first solution.
pub fn id(&self) -> String {
pub fn id(&self) -> u64 {
let solution = self.solution();
solution
.get("solutionId")
.unwrap()
.as_u64()
.map(|id| id.to_string())
.unwrap()
.to_owned()
}
Expand Down

0 comments on commit 796e7bd

Please sign in to comment.