Skip to content
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

Jump to bottom

DO NOT REVIEW Strict Custom Types #784

Closed
wants to merge 2 commits into from
Closed

DO NOT REVIEW Strict Custom Types #784

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcacb32
feat: types everywhere
ypatil12 Sep 26, 2024
e1844a1
feat: push
ypatil12 Sep 30, 2024
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
1 change: 1 addition & 0 deletions lib/openzeppelin-contracts
Open in desktop
Submodule openzeppelin-contracts added at 3b8b4b
1 change: 1 addition & 0 deletions lib/openzeppelin-contracts-upgradeable
Open in desktop
Submodule openzeppelin-contracts-upgradeable added at 6b9807
68 changes: 25 additions & 43 deletions src/contracts/core/DelegationManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ contract DelegationManager is
* @param strategy The strategy in which to increase the delegated shares.
* @param existingShares The number of deposit shares the staker already has in the strategy. This is the shares amount stored in the
* StrategyManager/EigenPodManager for the staker's shares.
* @param addedOwnedShares The number of shares to added to the staker's shares in the strategy
* @param addedShares The shares added to the staker's shares in the strategy
*
* @dev *If the staker is actively delegated*, then increases the `staker`'s delegated delegatedShares in `strategy`.
* Otherwise does nothing.
Expand All @@ -381,7 +381,7 @@ contract DelegationManager is
address staker,
IStrategy strategy,
Shares existingShares,
OwnedShares addedOwnedShares
Shares addedShares
) external onlyStrategyManagerOrEigenPodManager {
// if the staker is delegated to an operator
if (isDelegated(staker)) {
Expand All @@ -394,7 +394,7 @@ contract DelegationManager is
staker: staker,
strategy: strategy,
existingShares: existingShares,
addedOwnedShares: addedOwnedShares,
addedShares: addedShares,
totalMagnitude: totalMagnitude
});
}
Expand Down Expand Up @@ -516,7 +516,7 @@ contract DelegationManager is
operator: operator,
staker: staker,
strategy: strategies[i],
existingShares: uint256(0).wrapShares(),
existingShares: Shares(0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice, didn't know this was valid syntax.

addedOwnedShares: ownedShares[i],
totalMagnitude: totalMagnitudes[i]
});
Expand Down Expand Up @@ -589,27 +589,28 @@ contract DelegationManager is
* @param staker The staker to increase the depositScalingFactor for
* @param strategy The strategy to increase the delegated delegatedShares and the depositScalingFactor for
* @param existingShares The number of deposit shares the staker already has in the strategy.
* @param addedOwnedShares The shares added to the staker in the StrategyManager/EigenPodManager
* @param addedShares The shares added to the staker in the StrategyManager/EigenPodManager
* @param totalMagnitude The current total magnitude of the operator for the strategy
*/
function _increaseDelegation(
address operator,
address staker,
IStrategy strategy,
Shares existingShares,
OwnedShares addedOwnedShares,
Shares addedShares,
uint64 totalMagnitude
) internal {
_updateDepositScalingFactor({
staker: staker,
strategy: strategy,
existingShares: existingShares,
addedOwnedShares: addedOwnedShares,
addedShares: addedShares,
totalMagnitude: totalMagnitude
});

// based on total magnitude, update operators delegatedShares
DelegatedShares delegatedShares = addedOwnedShares.toDelegatedShares(totalMagnitude);
DelegatedShares delegatedShares = addedShares.toDelegatedShares(totalMagnitude);

// forgefmt: disable-next-line
operatorDelegatedShares[operator][strategy] = operatorDelegatedShares[operator][strategy].add(delegatedShares);

Expand Down Expand Up @@ -666,7 +667,7 @@ contract DelegationManager is
// check sharesToWithdraw is valid
// but for inputted strategies
Shares sharesWithdrawable = shareManager.stakerStrategyShares(staker, strategies[i]);
require(sharesToWithdraw.unwrap() <= sharesWithdrawable.unwrap(), WithdrawalExeedsMax());
require(sharesToWithdraw.lte(sharesWithdrawable), WithdrawalExeedsMax());

// Similar to `isDelegated` logic
if (operator != address(0)) {
Expand Down Expand Up @@ -742,37 +743,17 @@ contract DelegationManager is
IStrategy strategy,
uint64 totalMagnitude,
Shares existingShares,
OwnedShares addedOwnedShares
) internal {
Shares addedShares
) internal returns (uint256 newDepositScalingFactor) {
uint256 newDepositScalingFactor;
if (existingShares.unwrap() == 0) {
if (existingShares.isZero()) {
newDepositScalingFactor = WAD / totalMagnitude;
} else {
// otherwise since
//
// newShares
// = existingShares + addedShares
// = existingPrincipalShares.toDelegatedShares(stakerScalingFactors[staker][strategy).toOwnedShares(totalMagnitude) + addedShares
//
// and it also is
//
// newShares
// = newPrincipalShares.toDelegatedShares(stakerScalingFactors[staker][strategy).toOwnedShares(totalMagnitude)
// = newPrincipalShares * newDepositScalingFactor / WAD * totalMagnitude / WAD
// = (existingPrincipalShares + addedShares) * newDepositScalingFactor / WAD * totalMagnitude / WAD
//
// we can solve for
//
OwnedShares existingOwnedShares =
existingShares
.toDelegatedShares(depositScalingFactors[staker][strategy])
.toOwnedShares(totalMagnitude);
newDepositScalingFactor =
existingOwnedShares
.add(addedOwnedShares)
.unwrap()
.divWad(existingShares.unwrap() + addedOwnedShares.unwrap())
.divWad(totalMagnitude);
newDepositScalingFactor = existingShares.calculateNewScalingFactor(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, was planning on this 👍

addedShares: addedShares,
oldStakerScalingFactor: depositScalingFactors[staker][strategy],
totalMagnitude: totalMagnitude
);
}

// update the staker's depositScalingFactor
Expand Down Expand Up @@ -854,9 +835,9 @@ contract DelegationManager is
}

/// @notice a legacy function that returns the total delegated shares for an operator and strategy
function operatorShares(address operator, IStrategy strategy) public view returns (uint256) {
function operatorShares(address operator, IStrategy strategy) public view returns (DelegatedShares) {
uint64 totalMagnitude = allocationManager.getTotalMagnitude(operator, strategy);
return operatorDelegatedShares[operator][strategy].toOwnedShares(totalMagnitude).unwrap();
return operatorDelegatedShares[operator][strategy].toOwnedShares(totalMagnitude);
}

/**
Expand Down Expand Up @@ -884,9 +865,10 @@ contract DelegationManager is
uint64 totalMagnitude = allocationManager.getTotalMagnitude(operator, strategies[i]);

// forgefmt: disable-next-item
ownedShares[i] = shares
.toDelegatedShares(depositScalingFactors[staker][strategies[i]])
.toOwnedShares(totalMagnitude);
ownedShares[i] = shares.toOwnedShares(
stakerScalingFactor: depositScalingFactors[staker][strategies[i]],
operatorMagnitude: totalMagnitude
);
}
}
return ownedShares;
Expand All @@ -911,7 +893,7 @@ contract DelegationManager is
OwnedShares[] memory shares = getDelegatableShares(staker, strategies);

// if the last shares are 0, remove them
if (shares[strategies.length - 1].unwrap() == 0) {
if (shares[strategies.length - 1].isZero()) {
// resize the arrays
assembly {
mstore(strategies, sub(mload(strategies), 1))
Expand Down
48 changes: 25 additions & 23 deletions src/contracts/core/StrategyManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ contract StrategyManager is
IStrategy strategy,
IERC20 token,
uint256 amount
) external onlyWhenNotPaused(PAUSED_DEPOSITS) nonReentrant returns (OwnedShares shares) {
) external onlyWhenNotPaused(PAUSED_DEPOSITS) nonReentrant returns (Shares shares) {
shares = _depositIntoStrategy(msg.sender, strategy, token, amount);
}

Expand Down Expand Up @@ -135,7 +135,7 @@ contract StrategyManager is
address staker,
uint256 expiry,
bytes memory signature
) external onlyWhenNotPaused(PAUSED_DEPOSITS) nonReentrant returns (OwnedShares shares) {
) external onlyWhenNotPaused(PAUSED_DEPOSITS) nonReentrant returns (Shares shares) {
require(expiry >= block.timestamp, SignatureExpired());
// calculate struct hash, then increment `staker`'s nonce
uint256 nonce = nonces[staker];
Expand Down Expand Up @@ -183,6 +183,7 @@ contract StrategyManager is
IERC20 token,
OwnedShares ownedShares
) external onlyDelegationManager {
// TODO: add Shares type to StrategyBase
Copy link
Contributor

Choose a reason for hiding this comment

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

Can do this if needed.

strategy.withdraw(staker, token, ownedShares.unwrap());
}

Expand Down Expand Up @@ -237,36 +238,36 @@ contract StrategyManager is
* @param staker The address to add shares to
* @param token The token that is being deposited (used for indexing)
* @param strategy The Strategy in which the `staker` is receiving shares
* @param ownedShares The amount of ownedShares to grant to the `staker`
* @param shares The amount of shares to grant to the `staker`
* @dev In particular, this function calls `delegation.increaseDelegatedShares(staker, strategy, shares)` to ensure that all
* delegated shares are tracked, increases the stored share amount in `stakerStrategyShares[staker][strategy]`, and adds `strategy`
* to the `staker`'s list of strategies, if it is not in the list already.
*/
function _addOwnedShares(address staker, IERC20 token, IStrategy strategy, OwnedShares ownedShares) internal {
function _addShares(address staker, IERC20 token, IStrategy strategy, Shares sharesToAdd) internal {
// sanity checks on inputs
require(staker != address(0), StakerAddressZero());
require(ownedShares.unwrap() != 0, SharesAmountZero());
require(!sharesToAdd.isZero(), SharesAmountZero());

Shares existingShares = stakerStrategyShares[staker][strategy];

// if they dont have existing ownedShares of this strategy, add it to their strats
if (existingShares.unwrap()== 0) {
// if they dont have existing shares of this strategy, add it to their strats
if (existingShares.isZero()) {
require(stakerStrategyList[staker].length < MAX_STAKER_STRATEGY_LIST_LENGTH, MaxStrategiesExceeded());
stakerStrategyList[staker].push(strategy);
}

// add the returned ownedShares to their existing shares for this strategy
stakerStrategyShares[staker][strategy] = existingShares.add(ownedShares.unwrap()).wrapShares();
// add the returned shares to their existing shares for this strategy
stakerStrategyShares[staker][strategy] = existingShares.add(sharesToAdd);

// Increase shares delegated to operator, if needed
delegation.increaseDelegatedShares({
staker: staker,
strategy: strategy,
existingShares: existingShares,
addedOwnedShares: ownedShares
addedShares: sharesToAdd
});

emit Deposit(staker, token, strategy, ownedShares);
emit Deposit(staker, token, strategy, sharesToAdd);
}

/**
Expand All @@ -276,50 +277,51 @@ contract StrategyManager is
* @param strategy The Strategy contract to deposit into.
* @param token The ERC20 token to deposit.
* @param amount The amount of `token` to deposit.
* @return ownedShares The amount of *new* ownedShares in `strategy` that have been credited to the `staker`.
* @return shares The amount of *new* shares in `strategy` that have been credited to the `staker`.
*/
function _depositIntoStrategy(
address staker,
IStrategy strategy,
IERC20 token,
uint256 amount
) internal onlyStrategiesWhitelistedForDeposit(strategy) returns (OwnedShares ownedShares) {
) internal onlyStrategiesWhitelistedForDeposit(strategy) returns (Shares shares) {
// transfer tokens from the sender to the strategy
token.safeTransferFrom(msg.sender, address(strategy), amount);

// deposit the assets into the specified strategy and get the equivalent amount of shares in that strategy
ownedShares = strategy.deposit(token, amount).wrapOwned();
// TODO: should we use Shares type in StrategyBase?
uint256 depositedShares = strategy.deposit(token, amount);

// add the returned shares to the staker's existing shares for this strategy
_addOwnedShares(staker, token, strategy, ownedShares);
_addShares(staker, token, strategy, depositedShares);

return ownedShares;
return depositedShares.wrapShares();
}

/**
* @notice Decreases the shares that `staker` holds in `strategy` by `shareAmount`.
* @notice Decreases the shares that `staker` holds in `strategy` by `sharesToDecrement`.
* @param staker The address to decrement shares from
* @param strategy The strategy for which the `staker`'s shares are being decremented
* @param shareAmount The amount of shares to decrement
* @param sharesToDecrement The amount of shares to decrement
* @dev If the amount of shares represents all of the staker`s shares in said strategy,
* then the strategy is removed from stakerStrategyList[staker] and 'true' is returned. Otherwise 'false' is returned.
*/
function _removeShares(address staker, IStrategy strategy, Shares shareAmount) internal returns (bool) {
function _removeShares(address staker, IStrategy strategy, Shares sharesToDecrement) internal returns (bool) {
// sanity checks on inputs
require(shareAmount.unwrap() != 0, SharesAmountZero());
require(!sharesToDecrement.isZero(), SharesAmountZero());

//check that the user has sufficient shares
Shares userShares = stakerStrategyShares[staker][strategy];

require(shareAmount.unwrap() <= userShares.unwrap(), SharesAmountTooHigh());
require(sharesToDecrement.lte(userShares), SharesAmountTooHigh());

userShares = userShares.sub(shareAmount.unwrap()).wrapShares();
userShares = userShares.sub(sharesToDecrement);

// subtract the shares from the staker's existing shares for this strategy
stakerStrategyShares[staker][strategy] = userShares;

// if no existing shares, remove the strategy from the staker's dynamic array of strategies
if (userShares.unwrap() == 0) {
if (userShares.isZero()) {
_removeStrategyFromStakerStrategyList(staker, strategy);

// return true in the event that the strategy was removed from stakerStrategyList[staker]
Expand Down
Loading
Loading