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

Fixed bug in Omnipool arbitrager #145

Merged
merged 2 commits into from
Jul 14, 2023
Merged

Fixed bug in Omnipool arbitrager #145

merged 2 commits into from
Jul 14, 2023

Conversation

poliwop
Copy link
Collaborator

@poliwop poliwop commented Jul 12, 2023

No description provided.

Copy link
Contributor

@jepidoptera jepidoptera left a comment

Choose a reason for hiding this comment

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

I see what you did there, and that looks right. Do you think there should be a test for this? (Basically verifying that the arbitrageur doesn't take unprofitable trades.)

I see that one of the checks is failing. After looking into it, it's an edge case in test_add_liquidity where the liquidity goes over the 10^12 limit. Given that we've heard from Martin that that isn't actually part of the production implementation, should we just remove that limit?

@poliwop
Copy link
Collaborator Author

poliwop commented Jul 14, 2023

There is a test for unprofitable arb trades, that's how we caught this issue. We didn't catch it until now because the fuzzer didn't hit the right case.

@poliwop poliwop requested a review from jepidoptera July 14, 2023 19:33
@poliwop poliwop merged commit f81aa59 into main Jul 14, 2023
2 checks passed
@poliwop poliwop deleted the fix-arbitrager branch July 14, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants