Skip to content

Osmosis Integration Tests + Improvements#27

Open
lexzaiello wants to merge 46 commits into
mainfrom
feature-balancelogs
Open

Osmosis Integration Tests + Improvements#27
lexzaiello wants to merge 46 commits into
mainfrom
feature-balancelogs

Conversation

@lexzaiello

@lexzaiello lexzaiello commented Oct 3, 2024

Copy link
Copy Markdown
Contributor

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]

@lexzaiello lexzaiello self-assigned this Oct 3, 2024
@uditvira

uditvira commented Oct 3, 2024

Copy link
Copy Markdown

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.

@lexzaiello

Copy link
Copy Markdown
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.

@lexzaiello

Copy link
Copy Markdown
Contributor Author

This PR also updates the local-ic docker images.

@lexzaiello lexzaiello requested a review from bekauz October 4, 2024 17:02

@bekauz bekauz left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/scheduler.py
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment thread 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
Copy Markdown
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

@lexzaiello lexzaiello 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