Add liquidity-based position eviction to the DEX #4077
Assignees
Milestone
Comments
|
Replaces #2928 |
This was referenced Mar 22, 2024
|
This needs design work - meeting for the team forthcoming |
1 task
erwanor
added a commit
that referenced
this issue
Apr 4, 2024
## Describe your changes This adds a `max_positions_per_pair` parameter to the DEX component, in preparation for #4077. ## Checklist before requesting a review - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > This is backward compatible with `v0.71.0` since the field will be ignored/default to zero if not found in the message
This was referenced Apr 5, 2024
erwanor
added a commit
that referenced
this issue
Apr 8, 2024
## Describe your changes Working towards achieving #4077. This PR adds a `PositionCounter` extension trait which implements a `TradingPair` scoped position counter. I am not totally sold on a `u16` for the counter type, yet, but it's useful for testing anyway. ## Checklist before requesting a review - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > This is consensus-breaking because it introduces a new implicit validation rule: a hard-limit on the number of position opened for each trading pair (via counter overflow). It could also be consensus breaking if there was a bug in the DEX that allowed closed positions to be updated, but my assumption is that it's not the case.
erwanor
added a commit
that referenced
this issue
Apr 9, 2024
## Describe your changes This PR implements an inventory index, together with #4167, this PR works toward #4077 which contain the full eviction mechanism along with the adequate protocol specification update. The state key added: - $\text{I}_{A \rightarrow B} \coloneqq \text{prefix} \Vert BE(R_A) \Vert \text{id}$ - $\text{I}_{B \rightarrow A} \coloneqq \text{prefix} \Vert BE(R_B) \Vert \text{id}$ each corresponding to an index of position `Id`s ordered by reserves (ascending). I plan to immediately follow-up this PR with a proposal to refactor the inner `PositionManager` index implementations. ## Checklist before requesting a review - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > It adds a state key to nonverifiable storage, and absent the actual eviction mechanism, this isn't actually consensus breaking.
aubrika
added
the
friction
cratelyn
removed
the
friction
|
@erwanor can you share here the current status of work on this? |
4 tasks
|
Yes, sorry well overdue update - this ticket was split into two smaller PRs that I completed before switching gear towards the auction effort:
I just opened a draft PR to complete the last bit, it should be ready for review tomorrow: |
erwanor
added a commit
that referenced
this issue
Jun 1, 2024
## Description This PR: - [x] break out `StateWriteExt` and trim the dex component's public interface - [x] block-based tracking of "active" trading pairs (= has had an LP opened) - [x] implement the end block eviction logic ## Issue ticket number and link Final step to implement #4077 ## Checklist before requesting a review - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > Consensus breaking --------- Signed-off-by: Erwan Or <[email protected]> Co-authored-by: katelyn martin <[email protected]>
|
This is done on the protocol front, I have an app test that I would like to include before closing the issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem? Please describe.
The DEX engine does not bucket individual positions into bins with quantized ticks. Each position is filled against separately and can be whatever price it wants. This poses a potential problem: what if someone created a large number of very small positions, and the DEX then had to fill against all of them in sequence, forever?
Our answer to this has been to (1) make filling against a position very fast (2) to have nonzero gas fees required to create a position (3) to ensure that the DEX engine only considers the tip of the order book. For volatile pairs (where the price moves), this seems sufficient: the price of the position is fixed, but the price of the pair moves, so dust positions cannot remain at the tip of the order book without active management (causing fees on the part of the position creator). However, this doesn't work for stable pairs, where the price doesn't move. We've put off dealing with this issue for a while, but we should add a mitigation before mainnet.
Describe the solution you'd like
Add a chain parameter that sets a maximum number of positions per pair. A position can remain open only if it is in the top
Npositions, sorted by amount of reservesR_1OR if it is in the topNpositions, sorted by amount of reservesR_2. (This means that in principle there could be up to2*Npositions per pair).This should be implemented in such a way that we don't do expensive iteration over all positions, if possible.
The text was updated successfully, but these errors were encountered: