Skip to content
This repository has been archived by the owner on Apr 28, 2022. It is now read-only.

Sorts uniswaps according to execution plan. #366

Merged
merged 3 commits into from
Mar 11, 2021
Merged

Sorts uniswaps according to execution plan. #366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1cf2baa
Sorts uniswaps according to execution plan.
marcovc Mar 11, 2021
75995a2
Linting.
marcovc Mar 11, 2021
948ea57
Merge branch 'main' into sort-solution-according-to-exec-plan
Mar 11, 2021
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
10 changes: 10 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions solver/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ structopt = { version = "0.3" }
tokio = { version = "0.2", features =["macros", "rt-threaded", "time"] }
tracing = "0.1"
web3 = { version = "0.15", default-features = false, features = ["http-tls"] }
itertools = "0.10"

[dev-dependencies]
tracing-subscriber = "0.2"
Expand Down
2 changes: 1 addition & 1 deletion solver/src/http_solver/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ pub struct UpdatedUniswapModel {
pub exec_plan: ExecutionPlanCoordinatesModel,
}

#[derive(Debug, Deserialize)]
#[derive(Debug, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
pub struct ExecutionPlanCoordinatesModel {
pub sequence: u32,
pub position: u32,
Expand Down
3 changes: 3 additions & 0 deletions solver/src/http_solver/settlement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::{
settlement::Settlement,
};
use anyhow::{anyhow, ensure, Result};
use itertools::Itertools;
use model::order::OrderKind;
use primitive_types::{H160, U256};
use std::{
Expand Down Expand Up @@ -130,6 +131,8 @@ fn match_prepared_and_settled_amms(
settled_orders
.into_iter()
.filter(|(_, settled)| !(settled.balance_update1 == 0 && settled.balance_update2 == 0))
// .sorted_by_key(|su| {&su.1.exec_plan}) // How to make this work?
.sorted_by(|a, b| a.1.exec_plan.cmp(&b.1.exec_plan))
Comment on lines +134 to +135
Copy link
Contributor

@vkgnosis vkgnosis Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The reason the first line does not work is because because the closure is returning a reference and rust's strict lifetime rules prevent don't like this. I think this function from itertools could have been written in a way that allows references too but this is what we have to work with.
To fix it you can add Copy and Clone to the list of derived traits on top of struct ExecutionPlanCoordinatesModel and return the execution plan by value like .sorted_by_key(|su| su.1.exec_plan).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insight. It must be specific to closures, I suppose this could be workaround using a small helper function instead of the closure, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this in more detail it is actually a limitation of how Rust's trait system interacts with lifetimes and function traits at the moment so I don't think we can fix it with a real function and it's not the library's fault either https://stackoverflow.com/questions/47121985/why-cant-i-use-a-key-function-that-returns-a-reference-when-sorting-a-vector-wi .

Copy link
Contributor Author

@marcovc marcovc Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. I'm currently unable to parse the answer to the SO question :).

So here's the signature for the sort_by_key:

image

Is it true that everything would be fine if the type of F was instead

 F: FnMut(&Self::Item) -> &K

?

Copy link
Contributor

@fleupold fleupold Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding is that the generic K can resolve to a reference implicitly, however the error is related to lifetimes if F in sorted_by_key borrows &ExecutionPlanCoordinatesModel (which itself comes from another borrow, which may have a shorter lifetime).

Looking at ExecutionPlanCoordinatesModel since it's only a wrapper for two u32, you could make it derive Clone & Copy in which case the compiler can turn the reference into a owned value implicitly (I checked locally and .sorted_by_key(|su| su.1.exec_plan) compiles then). One way to avoid lifetime issues is not work with references 🙈.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally a generic type like this K can be a reference too. The problem here is some type system quirk of rust which is why one of the answers mentions generic associated types (GATs) and links rust-lang/rust#34162 . In that issue you can see how the author in the last example tried to correctly connect the lifetime of the parameter to the return type and then there is some discussion why that doesn't work at the moment. This is very involved and we have good alternatives (don't return reference, or use sort_by) so no big deal for this PR.

Copy link
Contributor Author

@marcovc marcovc Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. It seems the problem is indeed the return type of FnMut. This works:

fn sort_by_key_ref<T, K: Ord>(a: &mut [T], key: fn(&T) -> &K) {
    a.sort_by(|x, y| key(x).cmp(&key(y)));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even was able to add it as a mixin:

extern crate alloc;

type VecIntoIter<T> = alloc::vec::IntoIter<T>;

pub trait MyItertools: Iterator {
    fn sorted_by_key_ref<K, F>(self, f: F) -> VecIntoIter<Self::Item>
        where Self: Sized,
              K: Ord,
              F: Fn(&Self::Item) -> &K,
    {
        return self.sorted_by(|a, b| f(a).cmp(&f(b)))
    }
}

impl<T> MyItertools for T where T: Iterator {}

fn match_prepared_and_settled_amms(
    mut prepared_orders: HashMap<String, AmmOrder>,
    settled_orders: HashMap<String, UpdatedUniswapModel>,
) -> Result<Vec<ExecutedAmm>> {
    settled_orders
        .into_iter()
        .filter(|(_, settled)| !(settled.balance_update1 == 0 && settled.balance_update2 == 0))
        .sorted_by_key_ref(|su| {&su.1.exec_plan})  // <--- works now !!!
       etc...

Anyway, thanks for the help :)

.map(|(index, settled)| {
let prepared = prepared_orders
.remove(index.as_str())
Expand Down