Skip to content

Commit 4724489

Browse files
committed
refactor: decreaseBurnOrRedistributableShares (#1414)
**Motivation:** With the addition of non-arrified versions of `decreaseBurnableShares` and `releaseSlashEscrow` we want to avoid the use of `EnumerableSet.at()` as it reverts when attempting to retrieve indices that do not exist (> length). **Modifications:** - Use `EnumerableSet.tryGet` and `EnumerableSet.keys` instead which does not revert (but simply does nothing) if the index doesn't exist. - Rename `decreaseBurnOrRedistributableShares` -> `clearBurnOrRedistributableShares` to better reflect the functions purpose. **Result:** Protocol guarantees are maintained, and better naming.
1 parent 741bf78 commit 4724489

File tree

5 files changed

+39
-33
lines changed

5 files changed

+39
-33
lines changed

src/contracts/core/SlashEscrowFactory.sol

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,11 +100,11 @@ contract SlashEscrowFactory is Initializable, SlashEscrowFactoryStorage, Ownable
100100
// Assert that the escrow delay has elapsed
101101
require(block.number >= getEscrowCompleteBlock(operatorSet, slashId), EscrowDelayNotElapsed());
102102

103-
// Calling `decreaseBurnOrRedistributableShares` will transfer the underlying tokens to the `SlashEscrow`.
104-
// NOTE: While `decreaseBurnOrRedistributableShares` may have already been called, we call it again to ensure that the
103+
// Calling `clearBurnOrRedistributableShares` will transfer the underlying tokens to the `SlashEscrow`.
104+
// NOTE: While `clearBurnOrRedistributableShares` may have already been called, we call it again to ensure that the
105105
// underlying tokens are actually in escrow before processing and removing storage (which would otherwise prevent
106106
// the tokens from being released).
107-
strategyManager.decreaseBurnOrRedistributableShares(operatorSet, slashId);
107+
strategyManager.clearBurnOrRedistributableShares(operatorSet, slashId);
108108

109109
// Release the slashEscrow. The `SlashEscrow` is deployed in `initiateSlashEscrow`.
110110
_processSlashEscrow(operatorSet, slashId, getSlashEscrow(operatorSet, slashId), redistributionRecipient);

src/contracts/core/StrategyManager.sol

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -170,41 +170,44 @@ contract StrategyManager is
170170
}
171171

172172
/// @inheritdoc IStrategyManager
173-
function decreaseBurnOrRedistributableShares(
173+
function clearBurnOrRedistributableShares(
174174
OperatorSet calldata operatorSet,
175175
uint256 slashId
176176
) external nonReentrant {
177177
// Iterate over burnable shares backwards. Iterating with an increasing index can cause
178178
// elements to be missed as the item to remove is swapped and popped with the last element.
179-
uint256 length = _burnOrRedistributableShares[operatorSet.key()][slashId].length();
179+
address[] memory strategies = _burnOrRedistributableShares[operatorSet.key()][slashId].keys();
180+
uint256 length = strategies.length;
180181

181-
for (uint256 i = length; i > 0; --i) {
182-
decreaseBurnOrRedistributableShares(operatorSet, slashId, i - 1);
182+
for (uint256 i = 0; i < length; ++i) {
183+
clearBurnOrRedistributableSharesByStrategy(operatorSet, slashId, IStrategy(strategies[i]));
183184
}
184185
}
185186

186187
/// @inheritdoc IStrategyManager
187-
function decreaseBurnOrRedistributableShares(
188+
function clearBurnOrRedistributableSharesByStrategy(
188189
OperatorSet calldata operatorSet,
189190
uint256 slashId,
190-
uint256 index
191+
IStrategy strategy
191192
) public returns (uint256) {
192193
EnumerableMap.AddressToUintMap storage burnOrRedistributableShares =
193194
_burnOrRedistributableShares[operatorSet.key()][slashId];
194195

195-
(address strategy, uint256 sharesToRemove) = burnOrRedistributableShares.at(index);
196-
197-
// Withdraw the shares to the slash escrow.
198-
IStrategy(strategy).withdraw({
199-
recipient: address(slashEscrowFactory.getSlashEscrow(operatorSet, slashId)),
200-
token: IStrategy(strategy).underlyingToken(),
201-
amountShares: sharesToRemove
202-
});
196+
(, uint256 sharesToRemove) = burnOrRedistributableShares.tryGet(address(strategy));
203197

204198
burnOrRedistributableShares.remove(address(strategy));
205199

206-
// Emit an event to notify the that burnable shares have been decreased.
207-
emit BurnOrRedistributableSharesDecreased(operatorSet, slashId, IStrategy(strategy), sharesToRemove);
200+
if (sharesToRemove != 0) {
201+
// Withdraw the shares to the slash escrow.
202+
IStrategy(strategy).withdraw({
203+
recipient: address(slashEscrowFactory.getSlashEscrow(operatorSet, slashId)),
204+
token: IStrategy(strategy).underlyingToken(),
205+
amountShares: sharesToRemove
206+
});
207+
208+
// Emit an event to notify the that burnable shares have been decreased.
209+
emit BurnOrRedistributableSharesDecreased(operatorSet, slashId, strategy, sharesToRemove);
210+
}
208211

209212
return sharesToRemove;
210213
}

src/contracts/interfaces/IStrategyManager.sol

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,18 +140,19 @@ interface IStrategyManager is IStrategyManagerErrors, IStrategyManagerEvents, IS
140140
* @param operatorSet The operator set to burn shares in.
141141
* @param slashId The slash ID to burn shares in.
142142
*/
143-
function decreaseBurnOrRedistributableShares(OperatorSet calldata operatorSet, uint256 slashId) external;
143+
function clearBurnOrRedistributableShares(OperatorSet calldata operatorSet, uint256 slashId) external;
144144

145145
/**
146146
* @notice Removes a single strategy's shares from storage and transfers the underlying tokens for the slashId to the slash escrow.
147147
* @param operatorSet The operator set to burn shares in.
148148
* @param slashId The slash ID to burn shares in.
149-
* @param index The index of the strategy to burn shares in. Returns the amount of shares that were burned.
149+
* @param strategy The strategy to burn shares in.
150+
* @return The amount of shares that were burned.
150151
*/
151-
function decreaseBurnOrRedistributableShares(
152+
function clearBurnOrRedistributableSharesByStrategy(
152153
OperatorSet calldata operatorSet,
153154
uint256 slashId,
154-
uint256 index
155+
IStrategy strategy
155156
) external returns (uint256);
156157

157158
/**

src/test/mocks/StrategyManagerMock.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ contract StrategyManagerMock is Test {
109109
return (existingShares, addedShares);
110110
}
111111

112-
function decreaseBurnOrRedistributableShares(IStrategy strategy, uint sharesToBurn) external {}
112+
function clearBurnOrRedistributableShares(IStrategy strategy, uint sharesToBurn) external {}
113113

114114
function getBurnOrRedistributableCount(OperatorSet calldata operatorSet, uint slashId) external view returns (uint) {
115115
return _burnOrRedistributableCount;

src/test/unit/StrategyManagerUnit.t.sol

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1190,7 +1190,7 @@ contract StrategyManagerUnitTests_increaseBurnOrRedistributableShares is Strateg
11901190
}
11911191
}
11921192

1193-
contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is StrategyManagerUnitTests {
1193+
contract StrategyManagerUnitTests_clearBurnOrRedistributableShares is StrategyManagerUnitTests {
11941194
function _increaseBurnOrRedistributableShares(IStrategy strategy, uint sharesToAdd) internal {
11951195
cheats.prank(address(delegationManagerMock));
11961196
strategyManager.increaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId, strategy, sharesToAdd);
@@ -1209,7 +1209,7 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
12091209

12101210
cheats.expectEmit(true, true, true, true, address(strategyManager));
12111211
emit BurnOrRedistributableSharesDecreased(defaultOperatorSet, defaultSlashId, strategy, shares);
1212-
strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
1212+
strategyManager.clearBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
12131213

12141214
(IStrategy[] memory escrowStrats, uint[] memory escrowShares) =
12151215
strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
@@ -1231,7 +1231,7 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
12311231

12321232
cheats.expectEmit(true, true, true, true, address(strategyManager));
12331233
emit BurnOrRedistributableSharesDecreased(defaultOperatorSet, defaultSlashId, strategy, shares);
1234-
strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId, 0);
1234+
strategyManager.clearBurnOrRedistributableSharesByStrategy(defaultOperatorSet, defaultSlashId, strategy);
12351235

12361236
(IStrategy[] memory escrowStrats, uint[] memory escrowShares) =
12371237
strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
@@ -1260,7 +1260,7 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
12601260

12611261
_increaseBurnOrRedistributableShares(strategies, sharesToAdd);
12621262

1263-
strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
1263+
strategyManager.clearBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
12641264

12651265
(IStrategy[] memory escrowStrats, uint[] memory escrowShares) =
12661266
strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
@@ -1292,11 +1292,13 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
12921292
_increaseBurnOrRedistributableShares(strategies, sharesToAdd);
12931293

12941294
// Remove shares in random order
1295-
for (uint i = 0; i < strategies.length; ++i) {
1296-
// Create a random index to remove shares
1297-
uint index = r.Uint256(0, strategies.length - i - 1);
1295+
uint[] memory indices = new uint[](3);
1296+
indices[0] = 1; // dummyStrat2
1297+
indices[1] = 0; // dummyStrat
1298+
indices[2] = 2; // dummyStrat3
12981299

1299-
strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId, index);
1300+
for (uint i = 0; i < strategies.length; ++i) {
1301+
strategyManager.clearBurnOrRedistributableSharesByStrategy(defaultOperatorSet, defaultSlashId, strategies[indices[i]]);
13001302

13011303
(IStrategy[] memory strats, uint[] memory sharesToBurnOrRedistribute) =
13021304
strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
@@ -1340,7 +1342,7 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
13401342

13411343
cheats.expectRevert("SafeERC20: low-level call failed");
13421344
cheats.prank(address(delegationManagerMock));
1343-
strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
1345+
strategyManager.clearBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
13441346

13451347
assertEq(
13461348
strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId, strategy),

0 commit comments

Comments
 (0)