-
Notifications
You must be signed in to change notification settings - Fork 462
feat: duration vaults #1671
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
feat: duration vaults #1671
Conversation
ypatil12
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.
Looks good! General state machine + readability improvements. Big question is still around guarantees we need around staker delegation at the time of deposit.
| require(!isBlacklisted[underlyingToken], BlacklistedToken()); | ||
| require(address(durationVaultBeacon) != address(0), DurationVaultBeaconNotSet()); | ||
|
|
||
| newVault = IDurationVaultStrategy( |
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.
Do we want these to be deterministic deploys?
| address newVaultAdmin | ||
| ) external; | ||
|
|
||
| function vaultAdmin() external view returns (address); |
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.
Instead of vaultAdmin, is it worth just inheriting Ownable? Reduces the surface area of code IMO.
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.
Do we want it to be transferable?
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.
Don't think we need it to be transferrable? Fine to just leave as an immutable at deploy
| address newVaultAdmin | ||
| ) external; | ||
|
|
||
| function vaultAdmin() external view returns (address); |
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.
Don't think we need it to be transferrable? Fine to just leave as an immutable at deploy
| * @notice Locks the vault, preventing new deposits and withdrawals until maturity. | ||
| */ | ||
| function lock() external override onlyVaultAdmin { | ||
| require(_state == VaultState.Deposits, VaultAlreadyLocked()); |
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.
Do we want a startTime for the vault? Unsure what the user story is for when the vault can begin allocation
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.
Having a unique start time per vault enables each vault to have a deterministic deployment... again unsure if that's a relevant user story
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.
We used to have defined deposit windows but removed to make things easier for AVSs. Insurance AVS can lock at an arbitrary time but it's expected they will say publicly
fb1301d to
8829ce2
Compare
elhajin
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.
overall , looks good
elhajin
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
Motivation:
Financial AVS's need stake constraints (duration, quantity) in order to use EigenLayer.
Modifications:
A new type of strategy,
DurationVaultStrategy, that enforces delegate stake caps and duration constraints, and registers as an operator. Also includes modifications to theStrategyManagerand default strategies, including hooks before shares are added/removed, to accommodate alternative stake models.Result:
Financial AVS's will have the tools to build on Eigenlayer more easily.