Exclude too-small trades from optimizer

nagakingg commented 1 year ago

Description

Fixes #223 -- trades are excluded from optimization in multipair_optimal_arbitrage if their volume limit is <= pool.get_min_trade_size().

Additionally, trades are excluded from optimization in get_arb_trades if the optimal trade size is less than pool.get_min_trade_size() (i.e., the minimum trade size would cause the pool price to become less than the target market price). In this case, a trade in the right direction, but with size 0 is returned. This prevents similar errors as those discussed in #223, and prevents trades that are smaller than a pool's min trade size, but within the single-trade optimization in get_arb_trades.

Important change with downstream effects: Previously, if no profitable arb was available for a particular pair, get_arb_trades would return a trade with size 0 in whatever direction the coin symbols were listed in. E.g., if market prices were indexed as (ETH, BTC), it would return a 0 trade with ETH as coin in and BTC as coin out. This had downstream effects for multipair_optimal_arbitrage (because only trades in that direction were allowed in the multi-trade optimization) and subsequent price error calculations (because price error was always computed in that arbitrary direction).

With these changes, if no profitable arb is available (give the minimum trade size), a trade with size 0 in the more profitable direction is returned. This allows 'multipair_optimal_arbitrage' to make trades in that direction if they become available after previous trades in each optimizer iteration, and causes price error to always be computed in the direction of the best possible trade.

Additional changes:

Added ArbTrade class to templates.trader. It is a Trade object with a target price field, and a method to change the amount_in.
Price errors are now returned as a dict with format (base, quote): error

Hygiene checklist

[X] Changelog entry
[X] Everything public has a Numpy-style docstring (modules, public functions, classes, and public methods)
[X] Commit history is cleaned-up with minor changes squashed together and descriptive commit messages following Tim Pope's style

Cute Animal Picture

github-actions[bot] commented 1 year ago

Coverage report

The coverage rate went from 76.03% to 75.64% :arrow_down: The branch rate is 62%.

29.62% of new lines are covered.

Diff Coverage details (click to unfold)

### curvesim/metrics/metrics.py `100%` of new lines are covered (`83.76%` of the complete file). ### curvesim/pipelines/common/__init__.py `100%` of new lines are covered (`91.3%` of the complete file). ### curvesim/pipelines/simple/trader.py `100%` of new lines are covered (`100%` of the complete file). ### curvesim/pipelines/vol_limited_arb/strategy.py `0%` of new lines are covered (`42.85%` of the complete file). Missing lines: `44`, `46` ### curvesim/pipelines/vol_limited_arb/trader.py `8.77%` of new lines are covered (`12.17%` of the complete file). Missing lines: `76`, `77`, `79`, `80`, `81`, `83`, `84`, `86`, `88`, `89`, `92`, `94`, `96`, `99`, `100`, `101`, `117`, `122`, `124`, `126`, `129`, `130`, `131`, `133`, `142`, `143`, `144`, `145`, `146`, `147`, `149`, `150`, `152`, `154`, `159`, `160`, `168`, `169`, `171`, `172`, `174`, `204`, `205`, `206`, `207`, `208`, `210`, `211`, `212`, `213`, `214`, `216` ### curvesim/templates/trader.py `70%` of new lines are covered (`84.12%` of the complete file). Missing lines: `31`, `32`, `46`

nagakingg commented 1 year ago

The simple pipeline CI is unaffected by these changes, except for the price_error values, which differ slightly due to the new computation.

For the volume limited pipeline, the results change slightly. Qualitatively, they look pretty similar but perhaps slightly denoised.

3pool on current main branch:

3pool_old_opt

3pool on this branch:

3pool_new_opt

nagakingg commented 1 year ago

The comparison is simlar for TriCRV, except that the price errors come out quite noisy. Since the price errors are reduced using the new computation, this may reflect a noise floor. But I wonder if its better to record price errors as percect deviation from each target price. Currently they are absolute, which probably makes less sense for v2 pools.

TriCRV on current main branch:

triCRV_old_opt

TriCRV on this branch:

triCRV_new_opt

nagakingg commented 1 year ago

I wonder if its better to record price errors as percect deviation from each target price

Indeed, dividing each price error by the target price cleans up the price error plots considerably:

curveresearch / curvesim