-
Notifications
You must be signed in to change notification settings - Fork 12
Rollover vault #112
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
Rollover vault #112
Conversation
2f93f0c to
b15925c
Compare
b15925c to
7875d90
Compare
9a9a753 to
11dcabd
Compare
11dcabd to
515a658
Compare
4a1c8de to
8e98164
Compare
37ea215 to
57bb76c
Compare
57bb76c to
f5ac016
Compare
| if (!bond.isMature()) { | ||
| bond.mature(); | ||
| } | ||
| bond.redeemMature(address(tranche), tranche.balanceOf(address(this))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to be cognizant of is that this can revert when the entire supply is redeemed and brick our recoverAndRedeploy workflow.
https://github.com/buttonwood-protocol/tranche/blob/main/contracts/BondController.sol#L225
Options:
- We could wrap around try-catch
- Add logic to compute amounts discounting the min condition
- Handle manually if we ever run into this (deposit small amount of funds into the bond)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's right, good point. Could happen on deposit too if we don't deploy enough. I would say (2) compute the amounts, but since MINIMUM_VALID_DEBT isn't exposed, (1) try-catch works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e2979d2 to
8ba96c7
Compare
9788855 to
bda1ce4
Compare
brandoniles
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This covers up through deposit() in RolloverVault.sol
| _redeemTranches(); | ||
| } | ||
|
|
||
| /// @notice Deposits the provided asset from {msg.sender} into the vault and mints notes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably add a note saying it could be a good idea to call recover and deploy afterward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Co-authored-by: Brandon Iles <brandon@fragments.org>
brandoniles
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Covers up to right before the private write methods
| /// @return The amount of notes. | ||
| function deposit(IERC20Upgradeable token, uint256 amount) external nonReentrant whenNotPaused returns (uint256) { | ||
| // NOTE: The vault only accepts the underlying asset tokens. | ||
| if (token != underlying) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I was thinking about was making underlying immutable to give another layer of protection. Anytime a caller can specify a token to interact with, there's some danger of malicious code injection.
We'd have to add a constructor, though, in addition to the init function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we can have constructors in the proxy?
We could still get rid of the token parameter, don't think we need to generalize now. I'm okay with underlying not being strictly immutable, if the code doesn't have methods change it.
https://forum.openzeppelin.com/t/upgradable-contracts-instantiating-an-immutable-value/28763/2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, that all sounds good to me 👍
Co-authored-by: Brandon Iles <brandon@fragments.org>
2aafa2e to
e22180b
Compare
brandoniles
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_rollover
545ba47 to
2c16c13
Compare
2c16c13 to
baadd9d
Compare
Co-authored-by: Brandon Iles <brandon@fragments.org>
8fc8985 to
04c3640
Compare
e60adee to
feff303
Compare
brandoniles
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
At any given time the vault could hold, the underlying asset (deposit asset AMPL), the earned asset (SPOT) and the deployed assets (tranches).