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

Implement automatic on-chain arbitrage #833

Merged
merged 89 commits into from
Nov 18, 2022
Merged

Implement automatic on-chain arbitrage #833

merged 89 commits into from
Nov 18, 2022

Conversation

maltekliemann
Copy link
Member

@maltekliemann maltekliemann commented Oct 12, 2022

General Notes

Details found in ZIP-1.

The implementation of the bisection method is roughly based on the implementation in boost, which is licensed under the Boost License 1.0, which is GPL-3.0-or-later compatible according to this.

Concerns

  • I would feel more comfortable if we had a simple interface for minting/burning complete sets. The way things are right now, we have to be absolutely sure that the modifications I made here don't result in imbalances between complete sets of tokens and the available collateral in the prize pool. In hindsight, it may have been better to put the cache of pools for arbitrage in market-commons and execute the arbitrage in prediction-markets so that we can use the rudimentary prize pool API already present in prediction-markets.

    The question of "what goes where" in our code base is becoming a more and more serious painpoint to me. The mistake would probably not have happened if do_buy_complete_set and do_sell_complete_set were available in swaps. I see a major problem that prediction-markets and swaps are so intertwined because the scoring rule affects the market as much as it does the pool.

  • The arbitrage calculations are heavier than I anticipated. Still seems fine to me, but I think the maximum number of assets allowed in CPMM pools should be reduced.

Technical Addendum to ZIP-1

The test on_idle_arbitrages_pools_with_buy_burn demonstrates one particular inconvenience that you can run into when using bisection: The arbitrage amount should be 12.5 (i.e. 125_000_000_000 as fixed point number), and the starting interval is [0, 50], which means that the mid of bisection should it 12.5 exactly after two iterations. The Python prototype, which uses floating point math, does exactly that.

When using fixed points, you instead get the arbitrage amount 12.5007629394 (125_007_629_394). The reason is that when mid = 125_000_000_000, the total spot price is calculated to be 0.9999999999 (9_999_999_999) instead of 1, which doesn't trigger the break. Why not use a PRECISION parameter here to detect these small imprecisions?

Let's answer a different question first: Do we want the arbitrage amount to be precise as possible (so that the pool balances are as precise as possible), or do we want the price to be as close to 1 as possible? Of course, the more precise the amount is, the more precise the price will be. But depending on the derivative of f, it is possible for one of them to be "quite precise" and the other to actually be off pretty bad, so the question is important.

I think we need to go for a precise arbitrage amount, which will result in precise pool balances. Arbitrage is basically the result of moving funds, so the fewer funds can be moved, the better. If the balances are huge, the arbitrage amounts can be huge even if the price is very close to 1, so making sure that the amounts are actually correct is more important.

Going back to the original question, using a PRECISION parameter would solve the particular "problem", but introducting that parameter will also result in situations where arbitrage amounts are off pretty bad because we broke from the loop because the price was already "fairly correct". I think we're much better off without these types of micro optimizations.

Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

I didn't expect, that I will ever see an arbitrage amount inside a sandwich of lower and upper bounds.

Just an in-depth review of calc_preimage.

zrml/swaps/src/root.rs Show resolved Hide resolved
zrml/swaps/src/root.rs Show resolved Hide resolved
zrml/swaps/src/root.rs Show resolved Hide resolved
zrml/swaps/src/root.rs Show resolved Hide resolved
zrml/swaps/src/root.rs Outdated Show resolved Hide resolved
zrml/swaps/src/root.rs Show resolved Hide resolved
Chralt98
Chralt98 previously approved these changes Oct 31, 2022
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

Maybe @sea212 you find something. I am okay with it now.

Copy link
Member

@sea212 sea212 left a comment

Choose a reason for hiding this comment

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

Overall the implementation looks pretty solid.

zrml/swaps/src/root.rs Outdated Show resolved Hide resolved

// Return the sign of the (mathematical) difference x - y.
fn diff_sign<T: AtLeast32BitUnsigned>(x: T, y: T) -> i8 {
// Ignore clippy to prevent an sp-std dependency.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Ignore clippy to prevent an sp-std dependency.
// Ignore clippy to prevent a sp-std dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I believe this is correct: https://editorsmanual.com/articles/indefinite-article-a-an-with-abbreviations/ TL;DR: Pronounciation counts, so if you pronounce sp as ess-pee, then it should be an instead of a.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, one never stops learning.

zrml/swaps/src/root.rs Show resolved Hide resolved
zrml/swaps/src/root.rs Outdated Show resolved Hide resolved
Comment on lines 41 to 56
fn calc_total_spot_price(
&self,
balances: &BTreeMap<Asset<MarketId>, Balance>,
) -> Result<Fixed, &'static str>;

fn calc_arbitrage_amount_mint_sell(
&self,
balances: &BTreeMap<Asset<MarketId>, Balance>,
max_iterations: usize,
) -> Result<(Balance, usize), &'static str>;

fn calc_arbitrage_amount_buy_burn(
&self,
balances: &BTreeMap<Asset<MarketId>, Balance>,
max_iterations: usize,
) -> Result<(Balance, usize), &'static str>;
Copy link
Member

Choose a reason for hiding this comment

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

Docstrings are missing here

Copy link
Member Author

Choose a reason for hiding this comment

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

dd335f9

Also added some docstring to root.rs. I did not always add the Arguments section to the documentation. For example, how should I specifically document the arguments x and y of fn dist in root.rs? Maybe we can have a rule that the Arguments section can be skipped if the arguments are documented by the rest of the function description?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine. Adding docs for those would be redundant, since your example in the docstring exactly illustrates what is going on.

zrml/swaps/src/arbitrage.rs Show resolved Hide resolved
zrml/swaps/src/lib.rs Show resolved Hide resolved
zrml/swaps/src/lib.rs Show resolved Hide resolved
zrml/swaps/src/benchmarks.rs Show resolved Hide resolved
@maltekliemann maltekliemann added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Nov 17, 2022
@maltekliemann maltekliemann added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Nov 17, 2022
Copy link
Member

@sea212 sea212 left a comment

Choose a reason for hiding this comment

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

Well done, excited to see this in action ⭐

@Chralt98 Chralt98 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Nov 18, 2022
@maltekliemann maltekliemann merged commit 6fcdae8 into main Nov 18, 2022
@maltekliemann maltekliemann deleted the mkl-arbitrage branch November 18, 2022 13:17
@maltekliemann maltekliemann mentioned this pull request Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i:spec-changed ⚠️ Implies change in spec version s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The market can be easily arbitraged
3 participants