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.

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

Fix DSU unwrapping in multi invoker and manager contract #461

Merged
merged 12 commits into from
Oct 10, 2024
36 changes: 24 additions & 12 deletions packages/perennial-extensions/contracts/MultiInvoker.sol
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ contract MultiInvoker is IMultiInvoker, Kept {
/// @dev Mapping of allowed operators for each account
mapping(address => mapping(address => bool)) public operators;

/// @dev Mapping of claimable DSU for each account
mapping(address => UFixed6) public claimable;

/// @notice Constructs the MultiInvoker contract
/// @param usdc_ USDC stablecoin address
/// @param dsu_ DSU address
Expand Down Expand Up @@ -134,6 +137,18 @@ contract MultiInvoker is IMultiInvoker, Kept {
_invoke(account, invocations);
}

/// @notice withdraw DSU or unwrap DSU to withdraw USDC from this address to `account`
/// @param account Account to claim fees for
/// @param unwrap Wheather to wrap/unwrap collateral on withdrawal
function withdrawClaimable(address account, bool unwrap) external {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe just claim?

Copy link
Contributor

Choose a reason for hiding this comment

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

done in 329265f

if (msg.sender != account && !operators[account][msg.sender]) revert MultiInvokerUnauthorizedError();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think since we use this pattern twice it might make sense to make it an onlyOperator modifier or helper function like below in Manager.sol?

Copy link
Contributor

Choose a reason for hiding this comment

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

done in f43bbf9


UFixed6 claimableAmount = claimable[account];
claimable[account] = UFixed6Lib.ZERO;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: delete claimable[account] slightly cleaner here

Copy link
Contributor

Choose a reason for hiding this comment

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

The mapping holds a UFixed6, seemingly incompatible with the unary delete operator.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ope didn't know that 🙏


_withdraw(account, claimableAmount, unwrap);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our pattern has been to push funds to the msg.sender rather than the account (see: https://github.com/equilibria-xyz/perennial-v2/blob/v2.3/packages/perennial/contracts/Market.sol#L322)

Copy link
Contributor

Choose a reason for hiding this comment

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

done in a6161d9

}

/// @notice Performs a batch of invocations for an account
/// @param account Account to perform invocations for
/// @param invocations List of actions to execute in order
Expand Down Expand Up @@ -189,9 +204,8 @@ contract MultiInvoker is IMultiInvoker, Kept {

_approve(target);
} else if (invocation.action == PerennialAction.CLAIM_FEE) {
(IMarket market, bool unwrap) = abi.decode(invocation.args, (IMarket, bool));

_claimFee(account, market, unwrap);
(IMarket market) = abi.decode(invocation.args, (IMarket));
_claimFee(account, market);
}
}
// ETH must not remain in this contract at rest
Expand All @@ -205,7 +219,7 @@ contract MultiInvoker is IMultiInvoker, Kept {
/// @param newLong New long position for account in `market`
/// @param newShort New short position for account in `market`
/// @param collateral Net change in collateral for account in `market`
/// @param wrap Wheather to wrap/unwrap collateral on deposit/withdrawal
/// @param wrap Wheather to wrap/unwrap collateral on deposit
/// @param interfaceFee1 Primary interface fee to charge
/// @param interfaceFee2 Secondary interface fee to charge
function _update(
Expand Down Expand Up @@ -235,7 +249,7 @@ contract MultiInvoker is IMultiInvoker, Kept {
);

Fixed6 withdrawAmount = Fixed6Lib.from(Fixed18Lib.from(DSU.balanceOf()).sub(balanceBefore));
if (!withdrawAmount.isZero()) _withdraw(account, withdrawAmount.abs(), wrap);
if (!withdrawAmount.isZero()) claimable[account] = claimable[account].add(withdrawAmount.abs());

// charge interface fee
_chargeFee(account, market, interfaceFee1);
Expand Down Expand Up @@ -284,7 +298,7 @@ contract MultiInvoker is IMultiInvoker, Kept {
UFixed6Lib.from(DSU.balanceOf().sub(balanceBefore));

if (!claimAmount.isZero()) {
_withdraw(account, claimAmount, wrap);
claimable[account] = claimable[account].add(claimAmount);
}
}

Expand All @@ -307,19 +321,17 @@ contract MultiInvoker is IMultiInvoker, Kept {
if (interfaceFee.amount.isZero()) return;
_marketWithdraw(market, account, interfaceFee.amount);

if (interfaceFee.unwrap) _unwrap(interfaceFee.receiver, UFixed18Lib.from(interfaceFee.amount));
else DSU.push(interfaceFee.receiver, UFixed18Lib.from(interfaceFee.amount));
claimable[interfaceFee.receiver] = claimable[interfaceFee.receiver].add(interfaceFee.amount);

emit InterfaceFeeCharged(account, market, interfaceFee);
}

/// @notice Claims market fees, unwraps DSU, and pushes USDC to fee earner
/// @notice Claims market fees
/// @param market Market from which fees should be claimed
/// @param account Address of the user who earned fees
/// @param unwrap Set true to unwrap DSU to USDC when withdrawing
function _claimFee(address account, IMarket market, bool unwrap) internal isMarketInstance(market) {
function _claimFee(address account, IMarket market) internal isMarketInstance(market) {
UFixed6 claimAmount = market.claimFee(account);
_withdraw(account, claimAmount, unwrap);
claimable[account] = claimable[account].add(claimAmount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Upon further thinking, we might just want to keep this as a atomic claim and withdraw rather than putting it in the claimable map.

Since these fees are already in DSU in the market, there's no real gain to claiming them into the multiinvoker and then withdrawing in a separate tx. The process of doing the claim+unwrap (if the unwrap is non-reverting) or a DSU claim (if the unwrap is reverting) is the same with or without the multiinvoker escrow

Copy link
Contributor

Choose a reason for hiding this comment

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

Undid in a23a45e.

}

/// @notice Pull DSU or wrap and deposit USDC from `account` to this address for market usage
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,4 @@ struct InterfaceFee {

/// @dev The address to send the fee to
address receiver;

/// @dev Whether or not to unwrap the fee
bool unwrap;
}
14 changes: 4 additions & 10 deletions packages/perennial-extensions/contracts/types/TriggerOrder.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,12 @@ struct StoredTriggerOrder {
/* slot 1 */
address interfaceFeeReceiver1;
uint48 interfaceFeeAmount1; // <= 281m
bool interfaceFeeUnwrap1;
bytes5 __unallocated1__;
bytes6 __unallocated1__;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: add a note that these contain dirty data until updated post v2.3 migration

Copy link
Contributor

Choose a reason for hiding this comment

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

done in bb91a17


/* slot 2 */
address interfaceFeeReceiver2;
uint48 interfaceFeeAmount2; // <= 281m
bool interfaceFeeUnwrap2;
bytes5 __unallocated2__;
bytes6 __unallocated2__;
}
struct TriggerOrderStorage { StoredTriggerOrder value; }
using TriggerOrderStorageLib for TriggerOrderStorage global;
Expand Down Expand Up @@ -97,13 +95,11 @@ library TriggerOrderStorageLib {
Fixed6.wrap(int256(storedValue.delta)),
InterfaceFee(
UFixed6.wrap(uint256(storedValue.interfaceFeeAmount1)),
storedValue.interfaceFeeReceiver1,
storedValue.interfaceFeeUnwrap1
storedValue.interfaceFeeReceiver1
),
InterfaceFee(
UFixed6.wrap(uint256(storedValue.interfaceFeeAmount2)),
storedValue.interfaceFeeReceiver2,
storedValue.interfaceFeeUnwrap2
storedValue.interfaceFeeReceiver2
)
);
}
Expand All @@ -129,11 +125,9 @@ library TriggerOrderStorageLib {
bytes6(0),
newValue.interfaceFee1.receiver,
uint48(UFixed6.unwrap(newValue.interfaceFee1.amount)),
newValue.interfaceFee1.unwrap,
bytes5(0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be bytes6

Copy link
Contributor

Choose a reason for hiding this comment

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

corrected in bb91a17

newValue.interfaceFee2.receiver,
uint48(UFixed6.unwrap(newValue.interfaceFee2.amount)),
newValue.interfaceFee2.unwrap,
bytes5(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be bytes6

Copy link
Contributor

Choose a reason for hiding this comment

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

corrected in bb91a17

);
}
Expand Down
22 changes: 20 additions & 2 deletions packages/perennial-order/contracts/Manager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ abstract contract Manager is IManager, Kept {
/// Market => Account => Nonce => Order
mapping(IMarket => mapping(address => mapping(uint256 => TriggerOrderStorage))) private _orders;

/// @dev Mapping of claimable DSU for each account
/// Account => Amount
mapping(address => UFixed6) public claimable;

/// @dev Creates an instance
/// @param dsu_ Digital Standard Unit stablecoin
/// @param marketFactory_ Contract used to validate delegated signers
Expand Down Expand Up @@ -137,6 +141,17 @@ abstract contract Manager is IManager, Kept {
if (interfaceFeeCharged) emit TriggerOrderInterfaceFeeCharged(account, market, order.interfaceFee);
}

/// @inheritdoc IManager
function withdrawClaimable(address account, bool unwrap) external {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments on naming, where to send funds as above 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

done in 329265f and a6161d9

_ensureValidOperator(account, msg.sender);

UFixed6 claimableAmount = claimable[account];
claimable[account] = UFixed6Lib.ZERO;

if (unwrap) _unwrapAndWithdaw(account, UFixed18Lib.from(claimableAmount));
else DSU.push(account, UFixed18Lib.from(claimableAmount));
}

/// @notice reads keeper compensation parameters from an action message
function _compensateKeeperAction(Action calldata action) internal {
Copy link
Collaborator

Choose a reason for hiding this comment

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

where are these two new function used? can't see them referenced in any of the other changes.

Copy link
Contributor

@EdNoepel EdNoepel Oct 9, 2024

Choose a reason for hiding this comment

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

Was a merge artifact; removed in bb91a17.

_compensateKeeper(action.market, action.common.account, action.maxFee);
Expand All @@ -153,6 +168,10 @@ abstract contract Manager is IManager, Kept {
if (account != signer && !marketFactory.signers(account, signer)) revert ManagerInvalidSignerError();
}

function _ensureValidOperator(address account, address operator) internal view {
if (account != operator && !marketFactory.operators(account, operator)) revert ManagerInvalidOperatorError();
}

/// @notice Transfers DSU from market to manager to compensate keeper
/// @param amount Keeper fee as calculated
/// @param data Identifies the market from and user for which funds should be withdrawn,
Expand Down Expand Up @@ -193,8 +212,7 @@ abstract contract Manager is IManager, Kept {

_marketWithdraw(market, account, feeAmount);

if (order.interfaceFee.unwrap) _unwrapAndWithdaw(order.interfaceFee.receiver, UFixed18Lib.from(feeAmount));
else DSU.push(order.interfaceFee.receiver, UFixed18Lib.from(feeAmount));
claimable[order.interfaceFee.receiver] = claimable[order.interfaceFee.receiver].add(feeAmount);

return true;
}
Expand Down
9 changes: 9 additions & 0 deletions packages/perennial-order/contracts/interfaces/IManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ interface IManager {
/// @custom:error Signer is not authorized to interact with markets for the specified user
error ManagerInvalidSignerError();

// sig: 0x63c7e7fd
/// @custom:error Operator is not authorized to interact with markets for the specified user
error ManagerInvalidOperatorError();

/// @notice Store a new trigger order or replace an existing trigger order
/// @param market Perennial market in which user wants to change their position
/// @param orderId Client-specific order identifier
Expand Down Expand Up @@ -100,4 +104,9 @@ interface IManager {
/// @param account Actor whose position is to be changed
/// @param orderId Uniquely identifies the order for an account
function executeOrder(IMarket market, address account, uint256 orderId) external;

/// @notice withdraw DSU or unwrap DSU to withdraw USDC from this address to `account`
/// @param account Account to claim fees for
/// @param unwrap Wheather to wrap/unwrap collateral on withdrawal
function withdrawClaimable(address account, bool unwrap) external;
}
Loading