Skip to content

Commit

Permalink
Remove execution view functions and extraneous else branches
Browse files Browse the repository at this point in the history
  • Loading branch information
@adamegyed
adamegyed committed Jan 18, 2024
1 parent 560b4e4 commit 8eaac00
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 99 deletions.
13 changes: 0 additions & 13 deletions src/plugins/session/ISessionKeyPlugin.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,19 +107,6 @@ interface ISessionKeyPlugin {
/// @param sessionKey The session key to reset.
function resetSessionKeyGasLimitTimestamp(address account, address sessionKey) external;

// ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
// ┃ Execution view functions ┃
// ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

/// @notice Get the session keys of the account.
/// @return The array of session keys of the account.
function getSessionKeys() external view returns (address[] memory);

/// @notice Check if a session key is a session key of the account.
/// @param sessionKey The session key to check.
/// @return The boolean whether the session key is a session key of the account.
function isSessionKey(address sessionKey) external view returns (bool);

// ┏━━━━━━━━━━━━━━━━━━━━━━┓
// ┃ View functions ┃
// ┗━━━━━━━━━━━━━━━━━━━━━━┛
Expand Down
54 changes: 10 additions & 44 deletions src/plugins/session/SessionKeyPlugin.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,10 @@ contract SessionKeyPlugin is ISessionKeyPlugin, SessionKeyPermissions, BasePlugi
_sessionKeys.contains(msg.sender, CastLib.toSetValue(sessionKey)) && sessionKey == recoveredSig
) {
return _checkUserOpPermissions(userOp, calls, sessionKey);
} else {
return _SIG_VALIDATION_FAILED;
}
} else {
revert InvalidSignature(sessionKey);
return _SIG_VALIDATION_FAILED;
}
revert InvalidSignature(sessionKey);
}
revert NotImplemented();
}
Expand Down Expand Up @@ -190,35 +188,19 @@ contract SessionKeyPlugin is ISessionKeyPlugin, SessionKeyPermissions, BasePlugi

// The function `resetSessionKeyGasLimitTimestamp` is implemented in `SessionKeyPermissions`.

// ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
// ┃ Execution view functions ┃
// ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

/// @inheritdoc ISessionKeyPlugin
function getSessionKeys() external view returns (address[] memory) {
SetValue[] memory values = _sessionKeys.getAll(msg.sender);

return CastLib.toAddressArray(values);
}

/// @inheritdoc ISessionKeyPlugin
function isSessionKey(address sessionKey) external view returns (bool) {
return _sessionKeys.contains(msg.sender, CastLib.toSetValue(sessionKey));
}

// ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
// ┃ Plugin view functions ┃
// ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

/// @inheritdoc ISessionKeyPlugin
function sessionKeysOf(address account) external view returns (address[] memory) {
function sessionKeysOf(address account) external view override returns (address[] memory) {
SetValue[] memory values = _sessionKeys.getAll(account);

return CastLib.toAddressArray(values);
}

/// @inheritdoc ISessionKeyPlugin
function isSessionKeyOf(address account, address sessionKey) external view returns (bool) {
function isSessionKeyOf(address account, address sessionKey) external view override returns (bool) {
return _sessionKeys.contains(account, CastLib.toSetValue(sessionKey));
}

Expand All @@ -227,7 +209,7 @@ contract SessionKeyPlugin is ISessionKeyPlugin, SessionKeyPermissions, BasePlugi
// ┗━━━━━━━━━━━━━━━━━━━━━━┛

/// @inheritdoc ISessionKeyPlugin
function findPredecessor(address account, address sessionKey) external view returns (bytes32) {
function findPredecessor(address account, address sessionKey) external view override returns (bytes32) {
address[] memory sessionKeys = CastLib.toAddressArray(_sessionKeys.getAll(account));

uint256 length = sessionKeys.length;
Expand Down Expand Up @@ -257,14 +239,12 @@ contract SessionKeyPlugin is ISessionKeyPlugin, SessionKeyPermissions, BasePlugi
manifest.dependencyInterfaceIds[_MANIFEST_DEPENDENCY_INDEX_OWNER_RUNTIME_VALIDATION] =
type(IPlugin).interfaceId;

manifest.executionFunctions = new bytes4[](7);
manifest.executionFunctions = new bytes4[](5);
manifest.executionFunctions[0] = this.executeWithSessionKey.selector;
manifest.executionFunctions[1] = this.addSessionKey.selector;
manifest.executionFunctions[2] = this.removeSessionKey.selector;
manifest.executionFunctions[3] = this.rotateSessionKey.selector;
manifest.executionFunctions[4] = this.updateKeyPermissions.selector;
manifest.executionFunctions[5] = this.getSessionKeys.selector;
manifest.executionFunctions[6] = this.isSessionKey.selector;

ManifestFunction memory sessionKeyUserOpValidationFunction = ManifestFunction({
functionType: ManifestAssociatedFunctionType.SELF,
Expand Down Expand Up @@ -301,40 +281,26 @@ contract SessionKeyPlugin is ISessionKeyPlugin, SessionKeyPermissions, BasePlugi

// Session keys are only expected to be used for user op validation, so no runtime validation functions are
// set over executeWithSessionKey, and pre runtime hook will always deny.
ManifestFunction memory alwaysAllowValidationFunction = ManifestFunction({
functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW,
functionId: 0, // Unused.
dependencyIndex: 0 // Unused.
});

ManifestFunction memory ownerRuntimeValidationFunction = ManifestFunction({
functionType: ManifestAssociatedFunctionType.DEPENDENCY,
functionId: 0, // unused since it's a dependency
dependencyIndex: _MANIFEST_DEPENDENCY_INDEX_OWNER_RUNTIME_VALIDATION
});

manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](6);
manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](4);
manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({
executionSelector: this.getSessionKeys.selector,
associatedFunction: alwaysAllowValidationFunction
});
manifest.runtimeValidationFunctions[1] = ManifestAssociatedFunction({
executionSelector: this.isSessionKey.selector,
associatedFunction: alwaysAllowValidationFunction
});
manifest.runtimeValidationFunctions[2] = ManifestAssociatedFunction({
executionSelector: this.addSessionKey.selector,
associatedFunction: ownerRuntimeValidationFunction
});
manifest.runtimeValidationFunctions[3] = ManifestAssociatedFunction({
manifest.runtimeValidationFunctions[1] = ManifestAssociatedFunction({
executionSelector: this.removeSessionKey.selector,
associatedFunction: ownerRuntimeValidationFunction
});
manifest.runtimeValidationFunctions[4] = ManifestAssociatedFunction({
manifest.runtimeValidationFunctions[2] = ManifestAssociatedFunction({
executionSelector: this.rotateSessionKey.selector,
associatedFunction: ownerRuntimeValidationFunction
});
manifest.runtimeValidationFunctions[5] = ManifestAssociatedFunction({
manifest.runtimeValidationFunctions[3] = ManifestAssociatedFunction({
executionSelector: this.updateKeyPermissions.selector,
associatedFunction: ownerRuntimeValidationFunction
});
Expand Down
48 changes: 8 additions & 40 deletions test/plugin/session/SessionKeyPluginWithMultiOwner.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,7 @@ contract SessionKeyPluginWithMultiOwnerTest is Test {
SessionKeyPlugin(address(account1)).addSessionKey(sessionKeyToAdd, bytes32(0));

// Check using all view methods
address[] memory sessionKeys = SessionKeyPlugin(address(account1)).getSessionKeys();
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKeyToAdd);
assertTrue(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKeyToAdd));
sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
address[] memory sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKeyToAdd);
assertTrue(sessionKeyPlugin.isSessionKeyOf(address(account1), sessionKeyToAdd));
Expand All @@ -124,11 +120,7 @@ contract SessionKeyPluginWithMultiOwnerTest is Test {
SessionKeyPlugin(address(account1)).addSessionKey(sessionKeyToAdd, bytes32(0));

// Check using all view methods
address[] memory sessionKeys = SessionKeyPlugin(address(account1)).getSessionKeys();
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKeyToAdd);
assertTrue(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKeyToAdd));
sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
address[] memory sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKeyToAdd);
assertTrue(sessionKeyPlugin.isSessionKeyOf(address(account1), sessionKeyToAdd));
Expand All @@ -144,16 +136,12 @@ contract SessionKeyPluginWithMultiOwnerTest is Test {
vm.stopPrank();

// Check using all view methods
address[] memory sessionKeys = SessionKeyPlugin(address(account1)).getSessionKeys();
assertEq(sessionKeys.length, 2);
assertEq(sessionKeys[0], sessionKey2);
assertEq(sessionKeys[1], sessionKey1);
assertTrue(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey1));
assertTrue(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey2));
sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
address[] memory sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
assertEq(sessionKeys.length, 2);
assertEq(sessionKeys[0], sessionKey2);
assertEq(sessionKeys[1], sessionKey1);
assertTrue(sessionKeyPlugin.isSessionKeyOf(address(account1), sessionKey1));
assertTrue(sessionKeyPlugin.isSessionKeyOf(address(account1), sessionKey2));

vm.expectEmit(true, true, true, true);
emit SessionKeyRemoved(address(account1), sessionKey1);
Expand All @@ -164,11 +152,6 @@ contract SessionKeyPluginWithMultiOwnerTest is Test {
vm.stopPrank();

// Check using all view methods
sessionKeys = SessionKeyPlugin(address(account1)).getSessionKeys();
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKey2);
assertFalse(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey1));
assertTrue(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey2));
sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKey2);
Expand All @@ -191,12 +174,7 @@ contract SessionKeyPluginWithMultiOwnerTest is Test {
SessionKeyPlugin(address(account1)).rotateSessionKey(sessionKey1, predecessor, sessionKey2);

// Check using all view methods
address[] memory sessionKeys = SessionKeyPlugin(address(account1)).getSessionKeys();
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKey2);
assertFalse(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey1));
assertTrue(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey2));
sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
address[] memory sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKey2);
assertFalse(sessionKeyPlugin.isSessionKeyOf(address(account1), sessionKey1));
Expand Down Expand Up @@ -433,12 +411,7 @@ contract SessionKeyPluginWithMultiOwnerTest is Test {
vm.stopPrank();

// Check using all view methods
address[] memory sessionKeys = SessionKeyPlugin(address(account1)).getSessionKeys();
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKey1);
assertTrue(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey1));
assertFalse(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey2));
sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
address[] memory sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKey1);
assertTrue(sessionKeyPlugin.isSessionKeyOf(address(account1), sessionKey1));
Expand All @@ -461,12 +434,7 @@ contract SessionKeyPluginWithMultiOwnerTest is Test {
vm.stopPrank();

// Check using all view methods
address[] memory sessionKeys = SessionKeyPlugin(address(account1)).getSessionKeys();
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKey2);
assertTrue(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey2));
assertFalse(ISessionKeyPlugin(address(account1)).isSessionKey(sessionKey1));
sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
address[] memory sessionKeys = sessionKeyPlugin.sessionKeysOf(address(account1));
assertEq(sessionKeys.length, 1);
assertEq(sessionKeys[0], sessionKey2);
assertTrue(sessionKeyPlugin.isSessionKeyOf(address(account1), sessionKey2));
Expand Down
2 changes: 1 addition & 1 deletion test/plugin/session/permissions/SessionKeyERC20SpendLimits.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ contract SessionKeyERC20SpendLimitsTest is Test {

function test_sessionKeyERC20SpendLimits_validateSetUp() public {
// Check that the session key is registered
assertTrue(SessionKeyPlugin(address(account1)).isSessionKey(sessionKey1));
assertTrue(sessionKeyPlugin.isSessionKeyOf(address(account1), sessionKey1));

// Check that the session key is registered with the permissions plugin and has its allowlist set up
// correctly
Expand Down
2 changes: 1 addition & 1 deletion test/plugin/session/permissions/SessionKeyNativeTokenSpendLimits.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ contract SessionKeyNativeTokenSpendLimitsTest is Test {

function test_sessionKeyNativeTokenSpendLimits_validateSetUp() public {
// Check that the session key is registered
assertTrue(SessionKeyPlugin(address(account1)).isSessionKey(sessionKey1));
assertTrue(sessionKeyPlugin.isSessionKeyOf(address(account1), sessionKey1));

// Check that the session key is registered with the permissions plugin and has its allowlist set up
// correctly
Expand Down

0 comments on commit 8eaac00

Please sign in to comment.