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

Conversation

marcovc
Copy link
Contributor

@marcovc marcovc commented Mar 11, 2021

No description provided.

Comment on lines +134 to +135
// .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))
Copy link
Contributor

@vkgnosis vkgnosis Mar 11, 2021

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

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

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

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 :)

@marcovc marcovc marked this pull request as ready for review March 11, 2021 14:22
@marcovc marcovc requested a review from a team March 11, 2021 14:22
@marcovc marcovc merged commit 4dcc6cf into main Mar 11, 2021
@marcovc marcovc deleted the sort-solution-according-to-exec-plan branch March 11, 2021 14:28
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