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 auto-closing orders withdrawal #282

Merged
merged 10 commits into from
Jan 17, 2025
Merged

Conversation

JasonMHasperhoven
Copy link
Contributor

@JasonMHasperhoven JasonMHasperhoven commented Jan 16, 2025

@JasonMHasperhoven JasonMHasperhoven marked this pull request as ready for review January 16, 2025 17:20
@JasonMHasperhoven JasonMHasperhoven requested a review from a team January 16, 2025 17:20
@erwanor erwanor self-requested a review January 16, 2025 18:55
TalDerei and others added 2 commits January 16, 2025 13:37
prevState?.state === PositionState_PositionStateEnum.OPENED &&
nextState?.state === PositionState_PositionStateEnum.CLOSED,
)
.map(({ positionId }) => positionId);
Copy link
Member

Choose a reason for hiding this comment

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

👍

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, can merge on resolving conflicts

@TalDerei
Copy link
Contributor

TalDerei commented Jan 17, 2025

few questions cc @erwanor

  1. Is this scope intended to handle both batch closes and batch withdrawals, or just batch withdrawals? It errors on performing a batch close.
Screenshot 2025-01-16 at 7 20 53 PM
  1. why can't I close my last liquidity position?
Screen.Recording.2025-01-16.at.7.24.15.PM.mov

  1. is the batch withdrawal feature not working because the last liquidity position isn't closed?
Screen.Recording.2025-01-16.at.7.24.25.PM.mov

@erwanor
Copy link
Member

erwanor commented Jan 17, 2025

Batch withdrawals can fail for approximately two reasons:

  • an auto closing position has been closed by the dex engine: this is an ordinary usecase that is hit by almost every user of close_on_fill positions.
  • we construct a TPR that contains an LP that belongs to a different subaccount, this is somewhat of an edgecase.

This PR fixes 1), and leaves 2) for later. It will be easy to solve 2) when we get to the beta because we will have made changes to the view service, beginning with penumbra-zone/penumbra#4992

This PR is called "Fix batch withdrawal" but it should rather be named, fix auto-closing orders withdrawal.

@erwanor erwanor changed the title Fix batch withdrawal Fix auto-closing orders withdrawal Jan 17, 2025
@cronokirby
Copy link
Contributor

(added a conflict-resolution commit)

Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

Seems good ; I think it would've been simpler to just close all positions, eliminating the need to thread the state around, but this works fine as well, and can save on gas.

@JasonMHasperhoven JasonMHasperhoven merged commit ac232f3 into main Jan 17, 2025
3 checks passed
@JasonMHasperhoven JasonMHasperhoven deleted the fix-batch-withdrawal branch January 17, 2025 14:49
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.

4 participants