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

Fix batch withdrawal to work with auto-closing positions #241

Open
erwanor opened this issue Jan 10, 2025 · 2 comments
Open

Fix batch withdrawal to work with auto-closing positions #241

erwanor opened this issue Jan 10, 2025 · 2 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@erwanor
Copy link
Member

erwanor commented Jan 10, 2025

Context
On the Penumbra DEX, liquidity position can be set to close_on_fill (aka. "auto-closing") which means that they will be automatically closed by the DEX engine after being filled.

Since those positions close without any user actions, this means that token balance state of that user can lag behind the actual state of the position: the user will own an lpnft_opened to a position that is closed.

Bug

The DEX interface has "batch close" and "batch withdraw" features, which are currently broken when the position gets filled. This means that a user exercising limit orders will most likely lock themselves out of being able to do batch withdraws. We should fix this.

Bug mechanics
The DEX interface fetches the latest for each liquidity position that it is displaying. If it detects a position as Closed it will try to include it as part of a batch withdraw action. If the user does not possess an equivalent lpnft_closed the transcation will fail to plan with the following error ("Insufficient balance"):

Screenshot 2025-01-10 at 12 59 15 PM

Solution

Since the DEX engine has all the context it needs to detect those discrepancies (local state: opened, remote state: closed), it can remediate the insufficient funds issue by adding PositionClose to the transaction planner request.

This is possible for two reasons:

  1. The position is already closed so we will always compute an accurate balance commitment in the PositionWithdraw action
  2. The planner will be able to balance the PositionWithdraw action with the output of the PositionClose action (-opened + closed - closed + withdraw)

Alternative solution
Exclude those mismatch-y positions from the batch withdraw. We should not do that. It's simpler to let the planner handle all of this for us.

@erwanor erwanor changed the title Close and withdraw if an auto-closing position has been fulfilled Fix batch withdrawal to work with auto-closing positions Jan 12, 2025
@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra web Jan 12, 2025
@erwanor erwanor added bug Something isn't working enhancement New feature or request labels Jan 12, 2025
@erwanor erwanor moved this from 🗄️ Backlog to 📝 Todo in Penumbra web Jan 12, 2025
@TalDerei TalDerei moved this to 🗄️ Backlog in Labs web Jan 12, 2025
@JasonMHasperhoven JasonMHasperhoven moved this from 🗄️ Backlog to 📝 Todo in Labs web Jan 13, 2025
@JasonMHasperhoven JasonMHasperhoven self-assigned this Jan 14, 2025
@JasonMHasperhoven JasonMHasperhoven moved this from 📝 Todo to 🏗 In progress in Labs web Jan 14, 2025
@erwanor
Copy link
Member Author

erwanor commented Jan 14, 2025

Note: a position can be closed by the DEX engine for arbitrary reasons (from the pov of the explorer). The explorer needs to support those cases. The core logic is the same: if we are trying to withdraw a position that is closed, then we just add PositionClose to the transaction planner request.

Specifically, we shouldn't assume that the closed-withdraw mismatch happens exclusively for positions that are close_on_fill.

@erwanor
Copy link
Member Author

erwanor commented Jan 14, 2025

For now we can assume that the user has all their LPs in the main account, since we do not support subaccount filtering of positions. When penumbra-zone/penumbra#4985 gets merged we will be able to make it work for that case as well.

@erwanor erwanor moved this from 📝 Todo to 🏗 In progress in Penumbra web Jan 14, 2025
@JasonMHasperhoven JasonMHasperhoven moved this from 🏗 In progress to 🔎 In review in Labs web Jan 16, 2025
@TalDerei TalDerei moved this from 🔎 In review to 🏗 In progress in Labs web Jan 16, 2025
@TalDerei TalDerei moved this from 🏗 In progress to 🔎 In review in Labs web Jan 17, 2025
@JasonMHasperhoven JasonMHasperhoven moved this from 🔎 In review to ✅ Done in Labs web Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Status: 🏗 In progress
Development

No branches or pull requests

3 participants
@erwanor @JasonMHasperhoven and others