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

dex: Extend arbitrage routing #4292

Merged
merged 3 commits into from
May 14, 2024
Merged

dex: Extend arbitrage routing #4292

merged 3 commits into from
May 14, 2024

Conversation

zbuc
Copy link
Member

@zbuc zbuc commented Apr 30, 2024

Describe your changes

This PR extends arbitrage routing to route against assets involved in swaps and positions opened during the current block.

Issue ticket number and link

#4177

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label.

@zbuc zbuc added the consensus-breaking breaking change to execution of on-chain data label Apr 30, 2024
@zbuc zbuc requested review from hdevalence and erwanor April 30, 2024 20:02
@zbuc zbuc force-pushed the 4177_extended_candidate_Set branch from 587bc40 to b949ec0 Compare April 30, 2024 20:04
.cloned()
// Limit the inclusion of recently accessed assets to 10 to avoid
// potentially blowing up routing time.
.chain(state.recently_accessed_assets().iter().take(10).cloned())
Copy link
Member

Choose a reason for hiding this comment

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

This could have duplicates right? should we accumulate into a BTreeSet instead until its size increases by the max limit?

Copy link
Member Author

Choose a reason for hiding this comment

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

The recently_accessed_assets set is also incredibly likely to feature assets that are already in the fixed list, perhaps they should be filtered out on insertion as well.

@cratelyn cratelyn added the A-dex Area: Relates to the dex label May 1, 2024
@cratelyn cratelyn added this to the Sprint 5 milestone May 1, 2024
@zbuc zbuc force-pushed the 4177_extended_candidate_Set branch from 78b7e3c to c6449fa Compare May 3, 2024 18:46
@zbuc zbuc force-pushed the 4177_extended_candidate_Set branch from c6449fa to 1425f61 Compare May 3, 2024 18:58
@zbuc zbuc force-pushed the 4177_extended_candidate_Set branch from 1425f61 to fcdb1e5 Compare May 3, 2024 19:12
@zbuc zbuc temporarily deployed to smoke-test May 3, 2024 19:12 — with GitHub Actions Inactive
@aubrika aubrika added the friction something made this fall into the following milestone & the reason should be noted in a comment label May 6, 2024
@aubrika aubrika modified the milestones: Sprint 5, Sprint 6 May 6, 2024
@cratelyn cratelyn removed the friction something made this fall into the following milestone & the reason should be noted in a comment label May 6, 2024
Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

LGTM, tested manually on a devnet

);
self.add_recently_accessed_asset(
position.phi.pair.asset_2(),
routing_params.fixed_candidates,
Copy link
Member

Choose a reason for hiding this comment

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

It was a minor surprise that this wasn't internalized by add_recently_accessed_asset but I guess we technically save a lookup.

@erwanor erwanor changed the title Extend arbitrage routing dex: Extend arbitrage routing May 7, 2024
@erwanor erwanor merged commit 8ed2ad7 into main May 14, 2024
13 checks passed
@erwanor erwanor deleted the 4177_extended_candidate_Set branch May 14, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dex Area: Relates to the dex consensus-breaking breaking change to execution of on-chain data
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants