-
Notifications
You must be signed in to change notification settings - Fork 92
automated withdrawals from the staking strategy and validators #2685
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## nicka/pectra #2685 +/- ##
================================================
- Coverage 42.66% 40.78% -1.89%
================================================
Files 121 122 +1
Lines 5670 5750 +80
Branches 1502 1526 +24
================================================
- Hits 2419 2345 -74
- Misses 3248 3402 +154
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
contracts/tasks/tasks.js
Outdated
types.int | ||
) | ||
.addParam( | ||
"execute", |
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 could also be called dry-run
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.
Good point. I'll change to dryrun
to be consistent with the other Hardhat tasks
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.
Change is in commit a5430f6
"Automatically withdraws funds from a validator" | ||
) | ||
.addParam( | ||
"buffer", |
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.
BufferBp could be a better name?
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 like to keep the Hardhat option short. bufferBp
would become --buffer-bp
. The option description should say its in basis points
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.
good point 👍
contracts/tasks/validatorCompound.js
Outdated
|
||
// Iterate over the pending partial withdrawals | ||
let totalPendingPartialWithdrawals = BigNumber.from(0); | ||
let countPendingPartialWithdrawals = 0; |
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 could benefit in the future if we put these calculations in a separate script. So one could plug any Vault that supports async withdrawals and a native staking strategy and it would output the various balance metrics
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.
Good idea. I've put in a separate totalPartialWithdrawals
function in the same js file for now. See commit 70e1bef
Fixed summing of partial withdrawal amount as its in Gwei
Merging as I want to contract changes in the the feature branch. |
Changes
autoValidatorWithdrawals
that automatically withdraws from the staking strategy and the validatorssignMessage
to sign messages by the Defender RelayersCode Change Checklist
To be completed before internal review begins:
Internal review: