Add `samples` directory and custom session key plugins #22

sm-stack · 2024-01-08T08:09:53Z

Motivation

While there are basic plugins like SingleOwnerPlugin in src/plugins folder, it is not sufficient to show the different practices of plugins and the functionalities of ERC-6900. To enhance the understanding and utilization of ERC-6900 among plugin developers, establishing directories for various plugins would be beneficial.

Solution

This involves creating two new directories: src/samples/plugins and test/samples/plugins. These directories would host a range of plugins, showcasing the different capabilities and practices within the ERC-6900 framework.

Also add a custom session key plugin inside them, which demonstrates the usage of dependency and executeFromPluginExternal.

feat: Add Modular Session Key Plugin to `samples` directory

jaypaik · 2024-01-10T07:32:40Z

Thanks @sm-stack ! I plan on merging in the latest iteration of our address associated storage libraries soon. You'll be able to use that here instead of the PluginStorageLib currently included here. Stay tuned!

Update: #23

sm-stack · 2024-01-11T01:07:26Z

Update: #23

Updated some parts of the implementation with new PluginStorageLib!

jaypaik

Took a pass, thanks for your patience here! Some ERC updates in flight too, so I imagine we'll need to work through some rebases.

src/samples/plugins/ModularSessionKeyPlugin.sol

src/samples/plugins/interfaces/ISessionKeyPlugin.sol

src/samples/plugins/ModularSessionKeyPlugin.sol

src/samples/plugins/TokenSessionKeyPlugin.sol

test/samples/plugins/ModularSessionKeyPlugin.t.sol

src/samples/plugins/interfaces/ISessionKeyPlugin.sol

src/samples/plugins/ModularSessionKeyPlugin.sol

sm-stack · 2024-01-28T06:21:26Z

Took a pass, thanks for your patience here! Some ERC updates in flight too, so I imagine we'll need to work through some rebases.

Addressed all feedbacks you provided, with small fixes to maintain compatibility with recent changes in the spec.

src/samples/plugins/ModularSessionKeyPlugin.sol

test/samples/plugins/ModularSessionKeyPlugin.t.sol

jaypaik · 2024-02-15T01:50:58Z

test/samples/plugins/ModularSessionKeyPlugin.t.sol

+            address(ownerPlugin), (uint8(ISingleOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER))
+        );
+        modularSessionDependency[1] = FunctionReferenceLib.pack(
+            address(ownerPlugin), (uint8(ISingleOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF))


Suggested change

address(ownerPlugin), (uint8(ISingleOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF))

address(ownerPlugin), uint8(ISingleOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF)

jaypaik · 2024-02-15T01:53:29Z

src/samples/plugins/ModularSessionKeyPlugin.sol

+    }
+
+    /// @inheritdoc BasePlugin
+    function onUninstall(bytes calldata data) external override {


Area of improvement here would be to be able to clean up all of an account's session keys just based on msg.sender, without relying on what is passed into data. Ideally, the responsibility of proper cleanup lies with the plugin.

This is probably a larger change though. Feel free to tackle this later.

Though it will add some gas overhead, this sounds necessary to keep the consistency in the storage of plugin. I added an additional mapping that stores the information of session key and allowed selector to implement this.

jaypaik · 2024-02-15T02:17:27Z

src/samples/plugins/ModularSessionKeyPlugin.sol

+            if (validUntil != 0) {
+                return _packValidationData(false, validUntil, validAfter);
+            }
+            return _SIG_VALIDATION_FAILED;


Delaying SIG_VALIDATION_FAILED like this helps with gas estimation from bundlers when using dummy signatures.

Suggested change

if (validUntil != 0) {

return _packValidationData(false, validUntil, validAfter);

}

return _SIG_VALIDATION_FAILED;

return _packValidationData(validUntil == 0, validUntil, validAfter);

That seems valid. Fixed it.

jaypaik · 2024-02-15T02:19:26Z

src/samples/plugins/ModularSessionKeyPlugin.sol

+        returns (uint256)
+    {
+        if (functionId == uint8(FunctionId.USER_OP_VALIDATION_TEMPORARY_OWNER)) {
+            (address signer,) = userOpHash.toEthSignedMessageHash().tryRecover(userOp.signature);


Check the result of tryRecover, and if err != ECDSA.RecoverError.NoError, revert with InvalidSignature().

src/samples/plugins/ModularSessionKeyPlugin.sol

jaypaik · 2024-02-15T02:30:31Z

src/samples/plugins/ModularSessionKeyPlugin.sol

+    {
+        if (functionId == uint8(FunctionId.RUNTIME_VALIDATION_TEMPORARY_OWNER)) {
+            bytes4 selector = bytes4(data[0:4]);
+            bytes memory key = address(msg.sender).allocateAssociatedStorageKey(0, 1);


Nit

Suggested change

bytes memory key = address(msg.sender).allocateAssociatedStorageKey(0, 1);

bytes memory key = msg.sender.allocateAssociatedStorageKey(0, 1);

src/samples/plugins/TokenSessionKeyPlugin.sol

sm-stack · 2024-02-15T13:13:44Z

@jaypaik Addressed all the comments you suggested!

adam-alchemy

Looks good! I left a few optional suggestions, feel free to either adopt them or not.

adam-alchemy · 2024-02-15T16:46:03Z

src/samples/plugins/ModularSessionKeyPlugin.sol

+    function _castToBytes32(address addr, bytes4 b4) internal pure returns (bytes32 res) {
+        assembly {
+            res := or(shl(96, addr), b4)
+        }
+    }
+
+    function _castToAddressAndBytes4(bytes32 b32) internal pure returns (address addr, bytes4 b4) {
+        assembly {
+            addr := shr(96, b32)
+            b4 := and(b32, 0xFFFFFFFF)
+        }
+    }


These work as-is, but you could reduce the shift amount down to 32 (from 96) if you wanted to make it compact.

Yep that's valid! Fixed it.

adam-alchemy · 2024-02-15T16:50:11Z

src/samples/plugins/interfaces/ISessionKeyPlugin.sol

+    /// @param _untils The times until which the owners are valid.
+    event SessionKeysAdded(
+        address indexed account,
+        address[] indexed sessionKeys,


Just a heads up, indexing an array will result in the hash of the entire array, rather than indexing on the elements within it: https://docs.soliditylang.org/en/v0.8.19/abi-spec.html#encoding-of-indexed-event-parameters.

This is OK to leave as-is, just flagging that it may behave differently than expected. An alternative is to not index this field, or to emit a SessionKeyAdded event for each key, though that is expensive in gas.

I also think that this will lead to an unexpected behavior, so I just removed the indexed field at the arrays at the both events.

adam-alchemy · 2024-02-15T16:50:25Z

src/samples/plugins/interfaces/ISessionKeyPlugin.sol

+    /// @param account The account whose session keys are updated.
+    /// @param sessionKeys The addresses of the session keys.
+    /// @param selectors The selectors of the functions that the session keys are allowed to call.
+    event SessionKeysRemoved(address indexed account, address[] indexed sessionKeys, bytes4[] selectors);


Same here as above ^

sm-stack · 2024-02-15T23:33:55Z

@adam-alchemy Thanks for the feedbacks! Addressed to all of them.

jaypaik

Some final nits!

Also noticed you use unchecked { ++i; } in some places but not others. Would prefer those be consistent too!

jaypaik · 2024-02-16T04:24:32Z

src/samples/plugins/ModularSessionKeyPlugin.sol

+        uint48 _validAfter,
+        uint48 _validUntil


Nit:

Suggested change

uint48 _validAfter,

uint48 _validUntil

uint48 validAfter,

uint48 validUntil

jaypaik · 2024-02-16T04:24:37Z

src/samples/plugins/ModularSessionKeyPlugin.sol

+        sessionInfo.validAfter = _validAfter;
+        sessionInfo.validUntil = _validUntil;


Suggested change

sessionInfo.validAfter = _validAfter;

sessionInfo.validUntil = _validUntil;

sessionInfo.validAfter = validAfter;

sessionInfo.validUntil = validUntil;

jaypaik · 2024-02-16T04:26:36Z

src/samples/plugins/TokenSessionKeyPlugin.sol

+        for (uint256 i = 0; i < manifest.dependencyInterfaceIds.length;) {
+            manifest.dependencyInterfaceIds[i] = type(IModularSessionKeyPlugin).interfaceId;
+            unchecked {
+                ++i;
+            }
+        }


Why not just:

manifest.dependencyInterfaceIds[0] = ... manifest.dependencyInterfaceIds[1] = ...

That's more straightforward, fixed it.

jaypaik · 2024-02-16T04:26:57Z

src/samples/plugins/interfaces/ISessionKeyPlugin.sol

+    /// @param _afters The times after which the owners are valid.
+    /// @param _untils The times until which the owners are valid.
+    event SessionKeysAdded(
+        address indexed account,
+        address[] sessionKeys,
+        bytes4[] selectors,
+        uint48[] _afters,
+        uint48[] _untils


Suggested change

/// @param _afters The times after which the owners are valid.

/// @param _untils The times until which the owners are valid.

event SessionKeysAdded(

address indexed account,

address[] sessionKeys,

bytes4[] selectors,

uint48[] _afters,

uint48[] _untils

/// @param afters The times after which the owners are valid.

/// @param untils The times until which the owners are valid.

event SessionKeysAdded(

address indexed account,

address[] sessionKeys,

bytes4[] selectors,

uint48[] afters,

uint48[] untils

Making _after to after leads to the following error

Expected ',' but got reserved keyword 'after'

So I changed all of it to validAfter, and same with all untils.

jaypaik · 2024-02-16T04:27:09Z

src/samples/plugins/interfaces/ISessionKeyPlugin.sol

+    /// @param _after The time after which the owner is valid.
+    /// @param _until The time until which the owner is valid.
+    event SessionKeyAdded(
+        address indexed account, address indexed sessionKey, bytes4 selector, uint48 _after, uint48 _until
+    );


Suggested change

/// @param _after The time after which the owner is valid.

/// @param _until The time until which the owner is valid.

event SessionKeyAdded(

address indexed account, address indexed sessionKey, bytes4 selector, uint48 _after, uint48 _until

);

/// @param after The time after which the owner is valid.

/// @param until The time until which the owner is valid.

event SessionKeyAdded(

address indexed account, address indexed sessionKey, bytes4 selector, uint48 after, uint48 until

);

jaypaik · 2024-02-16T04:27:28Z

src/samples/plugins/interfaces/ISessionKeyPlugin.sol

+    /// @param _after The time after which the owner is valid.
+    /// @param _until The time until which the owner is valid.
+    function addSessionKey(address sessionKey, bytes4 allowedSelector, uint48 _after, uint48 _until) external;


Suggested change

/// @param _after The time after which the owner is valid.

/// @param _until The time until which the owner is valid.

function addSessionKey(address sessionKey, bytes4 allowedSelector, uint48 _after, uint48 _until) external;

/// @param after The time after which the owner is valid.

/// @param until The time until which the owner is valid.

function addSessionKey(address sessionKey, bytes4 allowedSelector, uint48 after, uint48 until) external;

jaypaik · 2024-02-16T04:27:39Z

src/samples/plugins/interfaces/ISessionKeyPlugin.sol

+    /// @param _afters The times after which the owners are valid.
+    /// @param _untils The times until which the owners are valid.
+    function addSessionKeyBatch(
+        address[] calldata sessionKeys,
+        bytes4[] calldata allowedSelectors,
+        uint48[] calldata _afters,
+        uint48[] calldata _untils


Suggested change

/// @param _afters The times after which the owners are valid.

/// @param _untils The times until which the owners are valid.

function addSessionKeyBatch(

address[] calldata sessionKeys,

bytes4[] calldata allowedSelectors,

uint48[] calldata _afters,

uint48[] calldata _untils

/// @param afters The times after which the owners are valid.

/// @param untils The times until which the owners are valid.

function addSessionKeyBatch(

address[] calldata sessionKeys,

bytes4[] calldata allowedSelectors,

uint48[] calldata afters,

uint48[] calldata untils

jaypaik

🥳

jaypaik · 2024-02-16T12:23:42Z

Looks like lint failed, can you run forge fmt?

sm-stack · 2024-02-16T12:54:49Z

Forgot it, just committed :) @jaypaik

sm-stack and others added 24 commits November 9, 2023 12:44

Add session key plugin and corresponding tests

0fb2113

Fix name of execution function

e4b429c

Refactor the logic of updating session key to include selector

f12a0ce

Fix lint

c7fd76f

Remove unused internal functions

f77df4c

Update comments

57cf21d

Fix prank overriding issue

3b63b0d

Fix lint

e097c21

Fix lint

d3ad5a7

Merge branch 'main' into session-key

82fc8fe

Merge branch 'main' into session-key

cfd4289

Fix plugin codes to be compatible with the spec update

f2ad9a6

Fix nits

f61b085

Fix functionId

3e5d7fb

Fix session key test corresponding to code changes

65e6d1a

Add batch owner operations and additional error handling logics

f2a74ad

Revert changes for pnpm-lock.yaml

b1367b5

Remove unused dependencies at test

c12d26d

Fix nits

3cb63a6

Add onInstall and unOninstall function at TokenSessionKey.sol

2252299

Fix format with forge fmt

4b99521

Add smaples directory

d21659e

Fix nits with forge fmt

76aa433

Merge pull request #1 from decipherhub/session-key

efb951f

feat: Add Modular Session Key Plugin to `samples` directory

sm-stack and others added 2 commits January 11, 2024 08:11

Merge branch 'main' into sample-with-session-key

691a9bb

Fix the implementation and test with new PluginStorageLib

610533c

sm-stack added 2 commits January 16, 2024 01:37

Merge branch 'main' into sample-with-session-key

fd0cdeb

Merge branch 'main' into sample-with-session-key

a5133fa

Update test codes to be compatible with v0.7

c38624c

jaypaik reviewed Jan 22, 2024

View reviewed changes

sm-stack and others added 5 commits January 24, 2024 20:11

Merge branch 'erc6900:main' into sample-with-session-key

bc48866

Update expressions and nits

b59391c

Merge branch 'main' into sample-with-session-key

844b47e

Apply forge fmt

6a63c80

Update installPlugin

cb031e4

Apply suggested changes

5a64a38

jaypaik reviewed Feb 15, 2024

View reviewed changes

sm-stack added 2 commits February 15, 2024 22:09

Fix onUninstall to be able to run with msg.sender

6288888

Add getSessionKeysAndSelectors function

a8c6d36

adam-alchemy approved these changes Feb 15, 2024

View reviewed changes

sm-stack added 2 commits February 16, 2024 08:30

Reduce amount to shift to 32

f3436ad

Remove indexed field at the array parameter of events

7c833ac

jaypaik reviewed Feb 16, 2024

View reviewed changes

Fix nits

0f755fb

jaypaik approved these changes Feb 16, 2024

View reviewed changes

Apply formatting

874b578

jaypaik merged commit 8b863f2 into erc6900:main Feb 16, 2024
3 checks passed

sm-stack deleted the sample-with-session-key branch February 16, 2024 14:04

jwindawi mentioned this pull request Feb 23, 2024

Community Call #6 erc6900/resources#28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `samples` directory and custom session key plugins #22

Add `samples` directory and custom session key plugins #22

sm-stack commented Jan 8, 2024

jaypaik commented Jan 10, 2024 •

edited

Loading

sm-stack commented Jan 11, 2024

jaypaik left a comment

sm-stack commented Jan 28, 2024

jaypaik Feb 15, 2024

jaypaik Feb 15, 2024

sm-stack Feb 15, 2024

jaypaik Feb 15, 2024

sm-stack Feb 15, 2024

jaypaik Feb 15, 2024

jaypaik Feb 15, 2024

sm-stack commented Feb 15, 2024

adam-alchemy left a comment

adam-alchemy Feb 15, 2024

sm-stack Feb 15, 2024

adam-alchemy Feb 15, 2024

sm-stack Feb 15, 2024

adam-alchemy Feb 15, 2024

sm-stack commented Feb 15, 2024

jaypaik left a comment

jaypaik Feb 16, 2024

jaypaik Feb 16, 2024

jaypaik Feb 16, 2024

sm-stack Feb 16, 2024

jaypaik Feb 16, 2024

sm-stack Feb 16, 2024

jaypaik Feb 16, 2024

jaypaik Feb 16, 2024

jaypaik Feb 16, 2024

jaypaik left a comment

jaypaik commented Feb 16, 2024

sm-stack commented Feb 16, 2024

	address(ownerPlugin), (uint8(ISingleOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF))
	address(ownerPlugin), uint8(ISingleOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF)

	bytes memory key = address(msg.sender).allocateAssociatedStorageKey(0, 1);
	bytes memory key = msg.sender.allocateAssociatedStorageKey(0, 1);

		sessionInfo.validAfter = _validAfter;
		sessionInfo.validUntil = _validUntil;

Add samples directory and custom session key plugins #22

Add samples directory and custom session key plugins #22

Conversation

sm-stack commented Jan 8, 2024

Motivation

Solution

jaypaik commented Jan 10, 2024 • edited Loading

sm-stack commented Jan 11, 2024

jaypaik left a comment

Choose a reason for hiding this comment

sm-stack commented Jan 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-stack commented Feb 15, 2024

adam-alchemy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-stack commented Feb 15, 2024

jaypaik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaypaik left a comment

Choose a reason for hiding this comment

jaypaik commented Feb 16, 2024

sm-stack commented Feb 16, 2024

Add `samples` directory and custom session key plugins #22

Add `samples` directory and custom session key plugins #22

jaypaik commented Jan 10, 2024 •

edited

Loading