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

Stop using string for solution_id #3154

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
}
3 changes: 1 addition & 2 deletions crates/driver/src/infra/api/routes/reveal/dto/solution.rs
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 Solution {
/// 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
3 changes: 1 addition & 2 deletions crates/driver/src/infra/api/routes/settle/dto/solution.rs
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 Solution {
/// 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
Loading