Skip to content

feat: substitute calcSlashedAmount for scaleForBurning #1502

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: substitute calcSlashedAmount for scaleForBurning #1502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/contracts/core/DelegationManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,8 +793,8 @@ contract DelegationManager is
// less than or equal to MIN_WITHDRAWAL_DELAY_BLOCKS ago. These shares are still slashable.
uint256 scaledSharesAdded = curQueuedScaledShares - prevQueuedScaledShares;

return SlashingLib.scaleForBurning({
scaledShares: scaledSharesAdded,
return SlashingLib.calcSlashedAmount({
operatorShares: scaledSharesAdded.mulWad(prevMaxMagnitude),
Comment on lines +796 to +797
Copy link
Member

@wadealexc wadealexc Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're here, the ideal "most readable" change would be to move the call to calcSlashedAmount up to slashOperatorShares. Maybe something like this:

    /// @inheritdoc IDelegationManager
    function slashOperatorShares(
        address operator,
        OperatorSet calldata operatorSet,
        uint256 slashId,
        IStrategy strategy,
        uint64 prevMaxMagnitude,
        uint64 newMaxMagnitude
    ) external onlyAllocationManager nonReentrant returns (uint256 totalDepositSharesToSlash) {
        uint256 operatorSharesSlashed = SlashingLib.calcSlashedAmount({
            operatorShares: operatorShares[operator][strategy],
            prevMaxMagnitude: prevMaxMagnitude,
            newMaxMagnitude: newMaxMagnitude
        });

        // Comment explaining conversion between scaled shares in queue and operator shares
        uint256 slashableQueuedShares = _getSlashableSharesInQueue(operator, strategy);
        uint256 queuedSharesSlashed = SlashingLib.calcSlashedAmount({
            operatorShares: slashableQueuedShares.mulWad(prevMaxMagnitude),
            prevMaxMagnitude: prevMaxMagnitude,
            newMaxMagnitude: newMaxMagnitude
        });

prevMaxMagnitude: prevMaxMagnitude,
newMaxMagnitude: newMaxMagnitude
});
Expand Down
18 changes: 4 additions & 14 deletions src/contracts/libraries/SlashingLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,20 +74,6 @@ library SlashingLib {
return scaledShares.mulWad(slashingFactor);
}

/**
* @notice Scales shares according to the difference in an operator's magnitude before and
* after being slashed. This is used to calculate the number of slashable shares in the
* withdrawal queue.
* NOTE: max magnitude is guaranteed to only ever decrease.
*/
function scaleForBurning(
uint256 scaledShares,
uint64 prevMaxMagnitude,
uint64 newMaxMagnitude
) internal pure returns (uint256) {
return scaledShares.mulWad(prevMaxMagnitude - newMaxMagnitude);
}

function update(
DepositScalingFactor storage dsf,
uint256 prevDepositShares,
Expand Down Expand Up @@ -178,6 +164,10 @@ library SlashingLib {
uint256 prevMaxMagnitude,
uint256 newMaxMagnitude
) internal pure returns (uint256) {
if (prevMaxMagnitude == 0) {
// TODO: consider throwing an error instead
return 0;
}
Comment on lines +167 to +170
Copy link
Member

@wadealexc wadealexc Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying my note from slack:

  1. Why did we need to add this, given the existing code does not check for this? Is there something unique to combining the methods that I'm missing?
  2. If we want to validate this, IMO we should keep the branch out of this library, and check explicitly in DM.slashOperatorShares. Where possible, this library's methods should be pure math expressions to make it easier to abstract away when reviewing.

Can add a comment to the library that it expects prevMaxMag != 0 (and any other invariants we expect, like that newMaxMag < prevMaxMag, operatorShares != 0, etc)

// round up mulDiv so we don't overslash
return operatorShares - operatorShares.mulDiv(newMaxMagnitude, prevMaxMagnitude, Math.Rounding.Up);
}
Expand Down
Loading