Skip to content

Conversation

bweick
Copy link
Contributor

@bweick bweick commented Oct 28, 2021

…culate default position issue amounts instead of position units.

This PR changes the getRequiredComponentIssuanceUnits getter to use the current component balances when calculating the amount of tokens required to issue a Default position instead of the Default positionUnit. We do this in order to provide better estimates for AAVE tokens which accrue tokens every block and are synced on every issuance and redemption for our FLI products.

We are treating this as a more one-off solution for dealing with AAVE. This module is now very specific to Sets that sync their balances on every issuance and redemption. getRequiredComponentIssuanceUnits will over estimate any Default positions that DO NOT sync every issuance and redemption AND have extra accrued tokens. If syncing is done through the Airdrop module and the manager has enabled an Airdrop fee then this will over-estimate the amount of tokens needed for issuance.

Code Review Processes

New Feature Review

Before submitting a pull request for new review, make sure the following is done:

  • Design doc is created and posted here: Not used here
  • Code cleanliness and completeness is addressed via guidelines

README Checks

  • README has proper context for the reviewer to understand what the code includes, any important design considerations, and areas to pay more attention to

Code Checks

  • Add explanatory comments. If there is complex code that requires specific context or understanding, note that in a comment
  • Remove unncessary comments. Any comments that do not add additional context, information, etc. should be removed
  • Add javadocs.
  • Scrub through the code for inconsistencies (e.g. removing extra spaces)
  • Ensure there are not any .onlys in spec files

Broader Considerations

  • Ensure variable, function and event naming is clear, consistent, and reflective for the scope of the code.
  • Consider if certain pieces of logic should be placed in a different library, module

…culate default position issue amounts instead of position units.
Copy link
Contributor

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

This looks good to me! Make sense.

To summarize, the main changes are:

  • how cumulativeEquity is calculated on L362
int256 cumulativeEquity = totalSupply
  .getDefaultPositionUnit(IERC20(component).balanceOf(address(_setToken)))
  .toInt256();
  • rounding up (equity) / rounding down (debt) when calculating the total units at L232

@bweick
Copy link
Contributor Author

bweick commented Nov 2, 2021

This looks good to me! Make sense.

To summarize, the main changes are:

  • how cumulativeEquity is calculated on L362
int256 cumulativeEquity = totalSupply
  .getDefaultPositionUnit(IERC20(component).balanceOf(address(_setToken)))
  .toInt256();
  • rounding up (equity) / rounding down (debt) when calculating the total units at L232

Yeah to your second bullet point, it's more that in the previous implementation the same code was used for issue and redeem so we needed an if statement to choose which way to round, here since this piece of code is only used for issuance calculations we don't need that if statement anymore. But the rounding should still be the same for issuance vs redeem.

@bweick bweick merged commit 99c5f91 into master Nov 3, 2021
@bweick bweick deleted the brian/fix-dimv2-getter branch November 3, 2021 00:38
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