Skip to content

Commit

Permalink
πŸ‘·πŸ»β€β™‚οΈ H-2: Rework delegate permission logic
Browse files Browse the repository at this point in the history
  • Loading branch information
JaredBorders committed Sep 15, 2023
1 parent a040ecb commit b4ddb7d
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/Engine.sol
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ contract Engine is IEngine, Multicallable, EIP712 {
returns (bool)
{
return PERPS_MARKET_PROXY.hasPermission(

This comment has been minimized.

Copy link
@kingofclubstroyDev

kingofclubstroyDev Sep 15, 2023

Consider changing this line to:
return PERPS_MARKET_PROXY.isAuthorized(

Since there may be a case where a caller has the admin role but not the PERPS_COMMIT_ASYNC_ORDER_PERMISSION role, and should be able considered a valid delegate

This comment has been minimized.

Copy link
@kingofclubstroyDev

kingofclubstroyDev Sep 15, 2023

_isAccountOwnerOrDelegate could just call PERPS_MARKET_PROXY.isAuthorized() if you want to make this change as isAuthorized() will check if the caller is the owner of the account, an admin, or the permission provided (PERPS_COMMIT_ASYNC_ORDER_PERMISSION), so that would reduce the number of external calls. That is if you want to allow users with the admin permission to be allowed to commit async orders.

_accountId, Constants.ADMIN_PERMISSION, _caller
_accountId, Constants.PERPS_COMMIT_ASYNC_ORDER_PERMISSION, _caller
);
}

Expand Down
2 changes: 2 additions & 0 deletions src/interfaces/IEngine.sol
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ interface IEngine {

/// @notice check if the msg.sender is a delegate of the account
/// identified by the accountId
/// @dev a delegate is an address that has been given
/// PERPS_COMMIT_ASYNC_ORDER_PERMISSION permission
/// @param _accountId the id of the account to check
/// @param _caller the address to check
/// @return true if the msg.sender is a delegate of the account
Expand Down
8 changes: 5 additions & 3 deletions test/Authentication.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ contract AccountDelegate is AuthenticationTest {

perpsMarketProxy.grantPermission({
accountId: accountId,
permission: ADMIN_PERMISSION,
permission: PERPS_COMMIT_ASYNC_ORDER_PERMISSION,
user: NEW_ACTOR
});

Expand All @@ -70,7 +70,7 @@ contract AccountDelegate is AuthenticationTest {

perpsMarketProxy.grantPermission({
accountId: accountId,
permission: ADMIN_PERMISSION,
permission: PERPS_COMMIT_ASYNC_ORDER_PERMISSION,
user: NEW_ACTOR
});

Expand All @@ -96,9 +96,11 @@ contract AccountDelegate is AuthenticationTest {
)
);

// only admin and owner can grant permission

perpsMarketProxy.grantPermission({
accountId: accountId,
permission: ADMIN_PERMISSION,
permission: PERPS_COMMIT_ASYNC_ORDER_PERMISSION,
user: NEW_ACTOR
});
}
Expand Down
10 changes: 9 additions & 1 deletion test/ConditionalOrder.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -384,9 +384,17 @@ contract VerifyConditions is ConditionalOrderTest {
function test_verifyConditions_public_non_condition_isAccountDelegate()
public
{
vm.prank(signer);

perpsMarketProxy.grantPermission({
accountId: accountId,
permission: PERPS_COMMIT_ASYNC_ORDER_PERMISSION,
user: DELEGATE_1
});

bytes[] memory conditions = new bytes[](1);
conditions[0] = abi.encodeWithSelector(
IEngine.isAccountDelegate.selector, accountId, address(engine)
IEngine.isAccountDelegate.selector, accountId, DELEGATE_1
);

IEngine.OrderDetails memory orderDetails;
Expand Down
2 changes: 1 addition & 1 deletion test/NonceBitmap.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ contract NonceBitmap is Bootstrap, ConditionalOrderSignature {

perpsMarketProxy.grantPermission({
accountId: accountId,
permission: ADMIN_PERMISSION,
permission: PERPS_COMMIT_ASYNC_ORDER_PERMISSION,
user: NEW_ACTOR
});

Expand Down
3 changes: 3 additions & 0 deletions test/utils/Constants.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ contract Constants {

bytes32 internal constant ADMIN_PERMISSION = "ADMIN";

bytes32 internal constant PERPS_COMMIT_ASYNC_ORDER_PERMISSION =
"PERPS_COMMIT_ASYNC_ORDER";

address internal constant MOCK_MARGIN_ENGINE = address(0xE1);

address internal constant ACTOR = address(0xa1);
Expand Down

0 comments on commit b4ddb7d

Please sign in to comment.