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: we can withdraw auto-closing positions #295

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

erwanor
Copy link
Member

@erwanor erwanor commented Jan 18, 2025

This PR fix withdrawals of auto-closing positions (#241).

Unfortunately, the approach we workshopped in #282 does not work because the PositionStore is already given a list of positions directly fetched from the DEX RPC service (latest state). This was an oversight on my part.

The code is hacky, maybe one could say that it sucks (?), but it works and we should spend no time trying to make it structurally better. We can fix linting, but let's keep it minimal. Our attention should be directed to the opened TODOs.

@TalDerei
Copy link
Contributor

linted the changeset. unrelated, but why can't I still close this pesky position?

Screen.Recording.2025-01-20.at.2.18.58.PM.mov

@erwanor
Copy link
Member Author

erwanor commented Jan 21, 2025

Thanks, @TalDerei can you share the output of pcli v balance

}

// Query the balance, ignoring the subaccount index for now.
const balances = await asyncIterableToArray(penumbra.service(ViewService).balances({}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can do:

Suggested change
const balances = await asyncIterableToArray(penumbra.service(ViewService).balances({}));
const balances = await Array.fromAsync(penumbra.service(ViewService).balances({}));

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we have a useBalances hook in src/shared/api/balances.ts. I feel like we should probably reuse that, that one also accept the account index to fetch the balances by the selected sub account.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can ignore this last comment, as the positions table currently doesn't do filtering by sub accounts yet.

}

// TODO(jason): not sure if this is duplicating code, feel free to move it out somewhere less disruptive.
async function asyncIterableToArray<T>(asyncIterable: AsyncIterable<T>): Promise<T[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this in favor of the comment below:

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you take the PR over, I'm working on something else and seems like you would be effective at pushing this across the finish line

@TalDerei
Copy link
Contributor

Thanks, @TalDerei can you share the output of pcli v balance

Screenshot 2025-01-21 at 11 25 39 AM

@erwanor
Copy link
Member Author

erwanor commented Jan 21, 2025

@TalDerei ah yes, the LP is in account 1. The closing flow is broken in that case. It will be easy to fix once we ship the alpha and implement penumbra-zone/penumbra#4992

@erwanor erwanor merged commit cfa3825 into main Jan 22, 2025
3 checks passed
@erwanor erwanor deleted the erwan/fix_limit_withdrawals branch January 22, 2025 17:35
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.

3 participants