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

Add totalAvailableAssets methods which account for withdrawable/redeemable funds #58

Merged
merged 2 commits into from
Oct 24, 2022
Merged

Add totalAvailableAssets methods which account for withdrawable/redeemable funds #58

merged 2 commits into from
Oct 24, 2022

Conversation

venables
Copy link
Contributor

No description provided.

@@ -35,6 +35,7 @@ interface IERC4626 is IERC20 {

/**
* @dev Calculate the total amount of underlying assets held by the vault.
* NOTE: This method includes assets that are marked for withdrawal.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think is more in the spirit of 4626? To include or exclude earmarked assets? I honestly don't know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking this would more closely map to totalSupply (which would include not-yet-redeemed tokens), which is my guess for what 4626 was going for.

// 48 shares should be available due to 25% withdraw gate.
expect(await pool.totalRedeemableBalance()).to.equal(41);
// 170 assets, 25% withdraw gate = 42 assets.
expect(await pool.totalWithdrawableAssets()).to.equal(41); // 42?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the 41 unexpected? Just looking at the 42?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah. I believe it should be 41, but was previously 42 due to some of the rounding + fx rate calculation changes. Will update in follow up.

@venables venables merged commit f2555b5 into circlefin:master Oct 24, 2022
@venables venables deleted the matt/total-assets-available branch October 24, 2022 19:39
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.

2 participants