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: shares for amount calculation should use freeFunds #425

Merged
3 commits merged into from Jun 30, 2021
Merged

fix: shares for amount calculation should use freeFunds #425

3 commits merged into from Jun 30, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jun 28, 2021

fixes #390

@ghost ghost requested review from jmonteer and fubuloubu June 28, 2021 13:52
Copy link
Contributor

@jmonteer jmonteer left a comment

Choose a reason for hiding this comment

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

added a comment for naming

otherwise LGTM

contracts/Vault.vy Outdated Show resolved Hide resolved
contracts/Vault.vy Outdated Show resolved Hide resolved
contracts/Vault.vy Outdated Show resolved Hide resolved
contracts/Vault.vy Outdated Show resolved Hide resolved
contracts/Vault.vy Outdated Show resolved Hide resolved
@fubuloubu
Copy link
Member

Should a test be added to cacth this behavior?

@ghost
Copy link
Author

ghost commented Jun 29, 2021

Should a test be added to cacth this behavior?

Should we add a function that exposes _sharesForAmount similar to _shareValue that's exposed by pricePerShare

Otherwise, the test will need to be performed on a withdraw

@fubuloubu
Copy link
Member

Should a test be added to cacth this behavior?

Should we add a function that exposes _sharesForAmount similar to _shareValue that's exposed by pricePerShare

Otherwise, the test will need to be performed on a withdraw

On withdrawal sounds appropriate

@ghost ghost merged commit 8334d86 into yearn:master Jun 30, 2021
poolpitako pushed a commit that referenced this pull request Jul 9, 2021
* fix: shares for amount calculation should use freeFunds

* refactor: free funds

* fix: if check
fubuloubu pushed a commit that referenced this pull request Jul 9, 2021
* fix: shares for amount calculation should use freeFunds

* refactor: free funds

* fix: if check
orbxball pushed a commit to orbxball/yearn-vaults that referenced this pull request Jul 14, 2021
* fix: shares for amount calculation should use freeFunds

* refactor: free funds

* fix: if check
orbxball pushed a commit to orbxball/yearn-vaults that referenced this pull request Aug 8, 2021
* fix: shares for amount calculation should use freeFunds

* refactor: free funds

* fix: if check
orbxball pushed a commit to orbxball/yearn-vaults that referenced this pull request Aug 8, 2021
* fix: shares for amount calculation should use freeFunds

* refactor: free funds

* fix: if check
sambacha pushed a commit to sambacha/yearn-vaults that referenced this pull request Sep 7, 2021
* fix: shares for amount calculation should use freeFunds

* refactor: free funds

* fix: if check
This pull request was closed.
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.

Question about recalculation of shares in withdraw
2 participants