-
Notifications
You must be signed in to change notification settings - Fork 80
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
Enable async withdrawals on superOETHb #2275
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2275 +/- ##
==========================================
+ Coverage 53.26% 53.36% +0.09%
==========================================
Files 79 79
Lines 4089 4089
Branches 1074 823 -251
==========================================
+ Hits 2178 2182 +4
+ Misses 1908 1904 -4
Partials 3 3 ☔ View full report in Codecov by Sentry. |
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.
Left a couple of comments inline. Looks great 👍
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
@@ -287,12 +288,16 @@ contract OETHVaultCore is VaultCore { | |||
internal | |||
returns (uint256 amount) | |||
{ | |||
// Note: When `withdrawalClaimDelay` is set to 0, users can |
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 think we will want to eventualy change the request.timestamp
to become an end stamp rather than a start time stamp.
The current code has three awkward things:
- If we change the claim duration, it retroactively changes the end date of inflight requests.
- If we disable async claims by setting it to zero, it actually allows instant claims of all inflight claims.
- If someone wrapped the governance execute() of a zero set, then they could flash loan a mint and a redeem in the same block.
However, our current upgrade on OETH will be atomically setting a value for this in the upgrade, and we don't have to use the zero value for now.
Given that the current zero value is dangerous, how about we also have the require nonzero in the claim? In this way we never accidentally give someone the ability to flashloan an 1:1 mint/redeem. We could then remove it at some future date once we've changed the code to make it safe.
*/ | ||
function setWithdrawalClaimDelay(uint256 _delay) external onlyGovernor { | ||
require( | ||
_delay == 0 || (_delay >= 10 minutes && _delay <= 7 days), |
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 think it makes sense to set the max delay to something that's greater than the max validator withdraw time. Perhaps 10 days? (At least one LST has a 15 day timed withdraw)
_delay == 0 || (_delay >= 10 minutes && _delay <= 7 days), | ||
"Invalid claim delay period" | ||
); | ||
emit WithdrawalClaimDelayUpdated(_delay); |
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 usually like doing events last, when possible.
Changes Overview
CLAIM_DELAY
constant in favour of awithdrawalClaimDelay
variablewithdrawalClaimDelay
has an hardcoded limit of 10m to 7d (to avoid accidentally or intentionally setting a huge withdrawal claim period)withdrawalClaimDelay
can be set to zero to disable async withdrawals. This will prevent users from making any new withdrawal requests. However, they'd be able to claim any pending withdrawals instantly after disabling itCode Change Checklist
To be completed before internal review begins:
Internal review: