Add session key plugins to reference implementation #10
Conversation
|
Thank you @sm-stack for this contribution! We'll kick off a deeper code review next week. |
There was a problem hiding this comment.
Hi @sm-stack - thank you for your contribution and I appreciate your patience in the review! This is well-written (save for a couple required changes), and I think you bring up a great argument for this to be included in the reference implementation as sample plugins that demonstrate the use of dependencies and executeFromPluginExternal. However, I'm less keen on using ERC-6900 to support an opinionated session key implementation. It's a valid problem, but not sure if this is the right place to try to further the standardization efforts in the session key space.
While the reference implementation can definitely benefit from examples like these that demonstrate the use of ERC-6900 interfaces, we're also discussing where a more permanent and appropriate home for plugin contributions might be.
I'd love to see this land, but bear with us while we decide what the best path forward here might be. In the meantime, I hope you find the comments in this review helpful!
| // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ | ||
|
|
||
| /// @inheritdoc BasePlugin | ||
| function onInstall(bytes calldata data) external override {} |
There was a problem hiding this comment.
It may be useful to be able to set up initial session key(s) as part of the installation. Thoughts?
There was a problem hiding this comment.
I agree, that will add a convenient way to add session keys to the accounts.
But I was thinking about adding it on the onInstall at child plugins(in this case TokenSessionKeyPlugin.sol`), since each session keys will be associated with them. I'll think more about it and update soon.
| function onInstall(bytes calldata data) external override {} | ||
|
|
||
| /// @inheritdoc BasePlugin | ||
| function onUninstall(bytes calldata) external override {} |
There was a problem hiding this comment.
Similarly, it may be less surprising to clean up existing session keys when this plugin is uninstalled, such that when the plugin is installed in the future, old session keys don't become valid again (given that they haven't expired).
There was a problem hiding this comment.
Thinking about the same thing with the above, thanks. Will update it soon.
There was a problem hiding this comment.
Added this functionality on TokenSessionKeyPlugin.sol, but currently facing an issue on using executeFromPlugin inside the onUninstall function. More detailed demostration of the issue comes from here.
Thank you for all the valuable feedback and support! I will address the provided suggestions in the coming days. Also I'll close this pull request once they are resolved. |
| /// @inheritdoc BasePlugin | ||
| function onUninstall(bytes calldata data) external override { | ||
| if (data.length != 0) { | ||
| // Call to removeTemporaryOwnerBatch function in BaseSessionKeyPlugin | ||
| (bool success, bytes memory returnData) = address(msg.sender).call( | ||
| abi.encodeWithSelector(IPluginExecutor.executeFromPlugin.selector, data)); | ||
| if (!success) { | ||
| assembly ("memory-safe") { | ||
| revert(add(returnData, 32), mload(returnData)) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@jaypaik I got a question on the use of executeFromPlugin at onUninstall function. I was thinking of a solution that modifies session keys through calls to BaseSessionKeyPlugin by executeFromPlugin, in the (un)installatation process of each child plugin (such as TokenSessionKey). The installation process worked well, but there was a problem at the uninstallation. Since the permittedExecutionSelectors are initialized before the invocation of onUninstall, the above function reverts with ExecFromPluginNotPermitted error.
Could you let me know if there are any additional contexts or security concerns you considered in the design process, about putting onUninstall at the last part of uninstallPlugin?
Motivation
Addressing Fragmentation in Session Key Implementations
The concept of session keys has emerged as an important feature for smart accounts, offering enhanced flexibility. Notably, numerous AA providers have developed their own variants of session key implementations. Despite the inherent utility of these implementations, there is the lack of compatibility among different AA providers. This fragmentation hinders the broader adoption and efficiency of session keys in the ecosystem. We believe that standardizing session key implementations under the ERC-6900 framework can effectively address this compatibility issue.
Demonstrating Dependencies and
executeFromPluginExternalAnother aspect is the inadequate demonstration of the dependency functionality and
executeFromPluginExternalfunction within the ERC-6900 ref implementation. While there are some mock implementations, it lacks practical examples, especially in the 'plugins' directory. Our objective is to provide a practical demonstration of these two features. We hope our implementation can clarify their usage and encourage their wider adoption in the further development based on ERC-6900.So we suggest adding session key plugin to the reference implementation.
Solution
The session key design consists of two parts: Base Session Key Plugin and Token Session Key Plugin. These two codes have a parent-child relationship. Each can be described as follows:
BaseSessionKeyPluginThis is a parent plugin for all session keys, managing all session keys existing within an account. Accordingly, it has functions like
addTemporaryOwnerandremoveTemporaryOwner, and two validation functions for session keys. Each validation function checks whether the signer (session key) has authority for the intended execution function and whether the period of the authority is valid. It also hasSingleOwnerPluginas a dependency, ensuring that only the owner can add or remove session keys. The reason for having such a global parent plugin is to avoid duplication in the logic of adding/removing session keys and to make session key management easier.TokenSessionKeyPluginThis acts as a kind of example child plugin and has
BaseSessionKeyPluginas a dependency. It has an execution function that calls the standard ERC20 token’stransferFromfunction throughexecuteFromPluginExternal. While this example is very simple, there could be more practical use cases in the future, such as session keys for a DEX Router contract or contracts within an Onchain game.