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

Vault fixes #129

Merged
merged 7 commits into from
Nov 21, 2024
Merged

Vault fixes #129

merged 7 commits into from
Nov 21, 2024

Conversation

corddry
Copy link
Collaborator

@corddry corddry commented Nov 19, 2024

No description provided.

function maxWithdraw(address) external view returns (uint256 maxAssets) {
maxAssets = VAULT.maxWithdraw(address(this));
function maxWithdraw(address owner) external view returns (uint256 maxAssets) {
return SoladyMath.min(VAULT.previewWithdraw(balanceOf[owner]), VAULT.maxWithdraw(address(this)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The first fix that modified maxMint and maxDeposit to pass on the receiver parameter to the underlying vault was the right one, if we follow the standard.

Note that the ethereum.org description of the standard has an error for maxWithdraw and maxRedeem. It's not "by the receiver":

image

It's "for the receiver":

image

The idea is to allow enabling blocklists at the vault level. If you are going to restrict holding vault shares by certain addresses, you would stop it both at transfer and deposit/mint. The maxDeposit/maxMint should reflect that.

It is a bit arbitrary, because blocklists could also be implemented on the function caller, and not just the token receiver, but that's what it is.

On limiting the return of maxRedeem/maxWithdraw to the minimum of the wrapped vault or wrapped vault holder is something I don't understand. The wrapped vault holder doesn't hold any shares of the underlying vault, so how would that work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Re this:

On limiting the return of maxRedeem/maxWithdraw to the minimum of the wrapped vault or wrapped vault holder is something I don't understand. The wrapped vault holder doesn't hold any shares of the underlying vault, so how would that work?

My understanding is that since shares in the wrapped vault are assigned 1:1 with the shares created in the underlying vault when a deposit occurs, the user's balanceOf is the maximum number of underlying vault shares they are entitled to redeem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

However, it's only correct in the case of maxRedeem as previewWithdraw expects an argument that is assets, not shares.

Copy link
Collaborator

@kmbarry1 kmbarry1 Nov 20, 2024

Choose a reason for hiding this comment

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

maxWithdraw needs to do something like VAULT.previewWithdraw(VAULT.convertToAssets(balanceOf[owner])) for the first arg to the min instead.

EDIT: in fact maybe just min(balanceOf[owner], VAULT.maxWithdraw(address(this))) is sufficient or even superior (should be the same up to rounding error).

@corddry corddry merged commit 5414c04 into main Nov 21, 2024
1 check passed
@corddry corddry deleted the vault-fixes branch November 21, 2024 21:26
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