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.

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

Conversation

nadir-akhtar
Copy link
Contributor

@nadir-akhtar nadir-akhtar commented Jun 30, 2025

Motivation:

When developing the slashing release, two different functions for calculating slashed shares were ideated: calcSlashedAmount and scaleForBurning. The former is used to calculated slashed "operator shares," or shares currently delegated to an operator, and the latter for "queued shares," or shares in the withdrawal queue that are currently slashable.

Despite the functionality effectively being equivalent (determine how many of these shares are to be slashed), two different functions were maintained due to uncertainty as to how to deduplicate them.

During the redistribution release, manual analysis yielded the conclusion that there is a way to consolidate these two functions. In order to reduce disparate logic, this PR removes scaleForBurning and replaces it with calcSlashedAmount.

Context:

calcSlashedAmount and scaleForBurning both attempt to do the same thing: return how many shares to slash as a result of an operator's magnitude change. However, they each go about it in different ways:

  • calcSlashedAmount reduces an operator's shares by a certain proportion, specifically by newMaxMag / prevMaxMag, then returns that number of shares.
  • scaleForBurning calculates the effective shares at the current and expected max mags, then returns the difference.

Note that the "type" of share entering each equation is not the same: calcSlashedAmount takes in operator shares, whereas scaleForBurning takes in "scaled shares," i.e. shares that are scaled based on staker deposit scaling factors (DSFs), but do not change based on operator slashings.

Once this type discrepancy was evident, the next step was to determine how to convert from one type to the other. Choosing calcSlashedAmount as the natural base function, it became evident that "scaled shares" can be translated to operator shares by simply multiplying by the current operator magnitude. With that simple change, calcSlashedAmount can be used to calculate shares slashed from both operator shares as well as queued shares.

Modifications:

  • Removes scaleForBurning from SlashingLib.sol and replaces its use with calcSlashedAmount

Result:

  • Cleaned up code
  • Clearer logic

@nadir-akhtar nadir-akhtar force-pushed the nadir/remove-scale-for-burning branch 2 times, most recently from 35c94c2 to 061c318 Compare June 30, 2025 20:58
@nadir-akhtar nadir-akhtar requested a review from ypatil12 June 30, 2025 22:44
@nadir-akhtar nadir-akhtar assigned wadealexc and unassigned wadealexc Jun 30, 2025
@nadir-akhtar nadir-akhtar requested a review from wadealexc June 30, 2025 22:44
@nadir-akhtar nadir-akhtar force-pushed the nadir/remove-scale-for-burning branch 2 times, most recently from fe0efd1 to 170e3dc Compare June 30, 2025 23:23
Comment on lines +167 to +170
if (prevMaxMagnitude == 0) {
// TODO: consider throwing an error instead
return 0;
}
Copy link
Member

@wadealexc wadealexc Jul 1, 2025

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)

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

@wadealexc wadealexc Jul 1, 2025

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
        });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants