test: slashing integration todos #934
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- still failing with `WithdrawalNotQueued` when slashing added
also fixes logging toggle
- adds time machine logging + cleanup - roll to completion removed from modify allocations to simplify check flow
0xClandestine
force-pushed
the
test/slashing-integration-todos
branch
from
4b024ff to
60442a0
Compare
8sunyuan
reviewed
Dec 10, 2024
8sunyuan
reviewed
Dec 10, 2024
8sunyuan
reviewed
Dec 10, 2024
8sunyuan
reviewed
Dec 10, 2024
8sunyuan
reviewed
Dec 10, 2024
8sunyuan
reviewed
Dec 10, 2024
ypatil12
requested changes
Dec 10, 2024
8sunyuan
reviewed
Dec 10, 2024
| } | ||
| } | ||
|
|
||
| function assert_HasUnderlyingTokenBalances_AfterSlash( |
There was a problem hiding this comment.
For non native ETH Strategies, we should also have an assertion on the token balance of the Strategy being burned/updated after the slashOperator call and it should be sent to the burn address.
Perhaps it makes sense to be in this check or a separate assert function
There was a problem hiding this comment.
will include in follow up pr
8sunyuan
reviewed
Dec 10, 2024
8sunyuan
reviewed
Dec 10, 2024
8sunyuan
reviewed
Dec 10, 2024
| _calculateExpectedTokens(withdrawals[i].strategies, withdrawals[i].scaledShares); | ||
| staker.completeWithdrawalAsTokens(withdrawals[i]); | ||
| // FIXME: check_Withdrawal_AsTokens_State_AfterSlash(staker, operator, withdrawals[i], allocateParams, slashingParams, expectedTokens); |
There was a problem hiding this comment.
Whats broken in this check atm?
There was a problem hiding this comment.
currently failing for staker shares, which should also be conditionally adjusted
There was a problem hiding this comment.
will fix in follow up pr
8sunyuan
reviewed
Dec 10, 2024
refactor: review changes refactor: final commit fix: `assert_Snap_Allocations_Modified` bandaid will improve in second pr fix: `assert_Snap_Allocations_Modified` made need further improvement... test(wip): add `assert_HasUnderlyingTokenBalances_AfterSlash` refactor: snap checks fix: `EigenPod` gwei bug test(wip): add `assert_Snap_Magnitudes_Slashed`
0xClandestine
force-pushed
the
test/slashing-integration-todos
branch
from
d7a5d61 to
78ddf56
Compare
Merged
|
Closing in favor of #945 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes:
HOLDS_MAXassetType (denotes a user that holds the maximum allowed strategies to test gas constraints).print.gasUsed()(prints the gas consumed from the last call).print.gasUsed().PermissionControllersupport toAVSandUserintegration user types.assertApproxEqAbs(1 single instance remains for beaconchain).TODOS: