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

Osmosis Integration Tests + Improvements #27

Open
wants to merge 46 commits into
base: main
Choose a base branch
from

Conversation

dowlandaiello
Copy link
Contributor

@dowlandaiello dowlandaiello commented Oct 3, 2024

This PR finalizes Osmosis local-ic integration test implementation. This was achieved by centralizing Skip queries in the Context, allowing for injection of IBC paths and denom info. Furthermore, several bug fixes were made in the process, allowing for these tests to pass, including the addition of route reevaluation (updating quantities in the execution plan in accordance with slight slippage at any step of the plan that would cause it to be invalidated), and a binary search based execution planner (allowing for tighter execution plan-wallet balance differences, and maximizing profit per trade). Furthermore, several other quality of life improvements were made, including leg balance prefixing in route logs, which assists in debugging.

An example of this is as follows:

2024-10-04 15:18:21 INFO balance[neutron-1](untrn): 15503863 balance[neutron-1](factory/neut): 5681 r2- Route has possible execution plan: [5681]

@dowlandaiello dowlandaiello self-assigned this Oct 3, 2024
@uditvira
Copy link

uditvira commented Oct 3, 2024

2024-10-03 21:21:33 INFO balance[ibc/376222D6]: 8556303 balance[untrn]: 15513403 r1- Route has possible execution plan: [267384, 0]

Is it possible to see balances of wallets on both the chains? At the minimum we need to make evident which chain this wallet belongs to.

@dowlandaiello
Copy link
Contributor Author

2024-10-03 21:21:33 INFO balance[ibc/376222D6]: 8556303 balance[untrn]: 15513403 r1- Route has possible execution plan: [267384, 0]

Is it possible to see balances of wallets on both the chains? At the minimum we need to make evident which chain this wallet belongs to.

Just made the change. Here's what the logs look like now:

2024-10-04 15:18:21 INFO     balance[neutron-1](untrn): 15503863 balance[neutron-1](factory/neut): 5681 r2- Route has possible execution plan: [5681]

Shows the balance for each in/out asset in all legs in the route, with the chain of the provider indicated.

@dowlandaiello
Copy link
Contributor Author

This PR also updates the local-ic docker images.

@dowlandaiello dowlandaiello requested a review from bekauz October 4, 2024 17:02
Copy link
Contributor

@bekauz bekauz left a comment

Choose a reason for hiding this comment

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

overall this looks good! it would help a lot with the review if you could add a few comments around some of the python-unique syntax oneliners, and general logic flow (e.g. early return conditions, prefix logic, etc).

I think I understand what's happening, but would be great to be a bit more certain.

assets = [leg.in_asset(), leg.out_asset()]

return [
x for x in (asset_balance_prefix(leg, asset) for asset in assets) if x
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to get some comments around oneliners like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

src/scheduler.py Outdated
async def query_denom_route(
self, query: DenomRouteQuery
) -> Optional[list[DenomRouteLeg]]:
if self.denom_routes and query in self.denom_routes:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we exit early here in case we already have a response stored? would be nice to add a comment

@dowlandaiello dowlandaiello changed the title Prefix Route Logs With Balance Information Osmosis Integration Tests + Improvements Oct 14, 2024
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.

3 participants