search

gnosis / dex-services

Off-chain services for the Gnosis Protocol v1.
33 stars 9 forks source link

Use query hops param for price estimation #1501

Closed fleupold closed 4 years ago

fleupold commented 4 years ago

This PR implements part of #1369 namely the integration of the hops Query parameter into the price estimation API (I couldn't reuse the other PR because the merge conflicts were too severe). It reuses the idea of creating a TokenPairRange type that can be passed along when both the pair and hops are important.

Unlike the other PR it continues to use the unbound price estimator in the services (instead of setting it to 30 hops). This is mainly for performance considerations and with the assumption that our orderbook in practice wouldn't use more than 30 hops anyways (we might eventually want to pick a realistic small number but this is beyond the scope of this PR).

This PR also leaves the test cases mostly untouched (using unbounded estimation wherever possible) as the actually bounding logic is covered by unit tests inside the bellman ford module.

I also stripped off the benchmarking code as this seems suitable for another PR.

Test Plan

Unit tests + loading a local web and making sure we can see the impact of changing the hops parameter in the orderbook view.

fleupold commented 4 years ago

I'm not sure it's a good idea to leave out the tests with a finite number of hops, since there is quite a bit of logic that could be changed without anyone noticing any issue in this case.

@fedgiac To me these tests seemed more like integration tests (as they are testing that bellman-ford works, which itself has unit tests) and adding this for hops in [None, Some(1), Some(5)...] made the tests less single purpose.

Given the work you put into coming up with the cases, I will add the extra test cases for the one in which the expected behavior differs from the unbounded ones...