-
Notifications
You must be signed in to change notification settings - Fork 319
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
add minShares to vault withdrawals #477
Comments
You end up spending a lot of gas and at the end it will revert, does that make sense? |
Added changes here https://github.com/ankitiitb1069/yearn-vaults/tree/add-minshares-to-vault-withdrawal, can I raise a PR for this now? |
yes, because metamask will tell you it's a reverted tx, and you won't end up sending that tx at all. also @ankitiitb1069 please add tests and submit a PR and we'll gladly review it :) |
@ankitiitb1069 did you submit a PR for this? |
@parseb he didn't, do you want to take were he left and add tests? |
@pandadefi on it, if PR not linked in 48h, assume the worst. |
Hey @parseb, i'll try to give this a shot if you're okay? |
yes. please do. |
@pandadefi, sent this PR #517. |
We are working on a new version of the vault that will not be based on this code. We aren't going to make a new release on top of that. |
got it, I'll close the PR. |
@pandadefi any help you need on the new version of vaults? |
We are still in the planning phase |
it's not the best UX when you desire to withdraw 100 shares that it will only consume 50.
should add
minShares
as a parameter and revert if it cannot fill the withdrawThe text was updated successfully, but these errors were encountered: