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: evict excess of liquidity positions #4511

Merged
merged 10 commits into from
Jun 1, 2024
Merged

Conversation

erwanor
Copy link
Member

@erwanor erwanor commented May 30, 2024

Description

This PR:

  • break out StateWriteExt and trim the dex component's public interface
  • block-based tracking of "active" trading pairs (= has had an LP opened)
  • implement the end block eviction logic

Issue ticket number and link

Final step to implement #4077

Checklist before requesting a review

  • 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

@erwanor erwanor added consensus-breaking breaking change to execution of on-chain data A-dex Area: Relates to the dex labels May 30, 2024
@erwanor erwanor added this to the Sprint 7 milestone May 30, 2024
@erwanor erwanor self-assigned this May 30, 2024
@erwanor erwanor marked this pull request as ready for review May 31, 2024 14:10
@erwanor
Copy link
Member Author

erwanor commented May 31, 2024

I have tested this manually on a devnet using a tweaked version of tx replicate. Reviews are welcome now, but I won't merge until I have an app test that spins up a chain with a low max position parameter. I will do this as a second PR, but wait to close the ticket.

@cratelyn cratelyn self-requested a review May 31, 2024 15:51
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

this looks good to me. nice work! i found a bug in our "recently accessed assets" logic, but i see that it comes from preexisting code, so i don't consider it blocking this change, despite it being a part of the diff.

the logic looks good, and the refactorings along the way have great side-effects. 🙂

crates/core/component/dex/src/component/dex.rs Outdated Show resolved Hide resolved
crates/core/component/dex/src/state_key.rs Show resolved Hide resolved
crates/core/component/dex/src/state_key.rs Show resolved Hide resolved
crates/core/component/dex/src/component/dex.rs Outdated Show resolved Hide resolved
Co-authored-by: katelyn martin <[email protected]>
Signed-off-by: Erwan Or <[email protected]>
Co-authored-by: katelyn martin <[email protected]>
Signed-off-by: Erwan Or <[email protected]>
erwanor and others added 2 commits May 31, 2024 14:49
this is a small change. this is a nice property about our data model,
i.e. that an owned copy of the dex parameters may be used to make
routing parameters.

let's make that relationship explicit by putting it into a `From<T>`
impl, and pulling it out of the `StateReadExt` logic.
@erwanor erwanor merged commit 849992f into main Jun 1, 2024
13 checks passed
@erwanor erwanor deleted the erwan/end_block_eviction branch June 1, 2024 00:20
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.

3 participants