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

feat: add replacePlugin #38

Closed
wants to merge 58 commits into from
Closed

Conversation

sm-stack
Copy link
Contributor

@sm-stack sm-stack commented Feb 13, 2024

Motivation

As mentioned at Issue #13, replacing a plugin relies on a weak assumption of nicely implemented uninstallPlugin - installPlugin pattern.

This approach can potentially disrupt the account's state, especially if the plugin being replaced is entangled with dependencies that are not trivially interchangeable. Therefore, it is necessary that plugin replacement is conducted with caution, ensuring that only minor, non-disruptive modifications are permitted.

Solution

This pull request solves Issue #13, by implementing replacePlugin function. This function enables 'hot swapping' between homogeneous plugins. For this, the following concepts are introduced.

  1. Version Registry
    replacePlugin determines the validity of replacement with the versions of plugins. Version in plugins are in the format of sematic versioning, and only the patch version updates are available. For this, a plugin is required to furnish a version registry in its manifest. The registry is responsible for classifying compatible plugins and determining the validity of the replacement. Also, downgrades to lower versions are prohibited, since downgrades have an edge case that an execution function is used in another plugin as dependency, and the downgrade process removes it.

  2. Plugin Data Migration
    During replacement, the user data (e.g. session key data at Session Key plugin) in the replaced plugin should be migrated to the new plugin, to minimize the confusion of users. However, allowing users to define the migration data in arbitrary way is dangerous, since authorizing user to manipulate the storage of the plugin can lead to unintended behavior. To prevent this, users can migrate data defined at getDataForReplacement inside each plugin. This can ensure that the data removal and addition will be done in the expected way.

sm-stack and others added 30 commits January 17, 2024 09:53
Feat. add data migration logic
sm-stack and others added 23 commits February 10, 2024 22:46
Feat: add data migration in replacePlugin
feat: add test codes for `replacePlugin`
@jaypaik
Copy link
Collaborator

jaypaik commented Mar 14, 2024

Thanks for your research and work on this issue @sm-stack @Sangyeup @brynnPark!

There are a few things we may want to think about more, and I'd like to invite other folks to leave their thoughts too.

  1. While something like a version registry could be useful for guaranteeing correct version upgrades, it increases overhead in plugin development and also increases complexity. Would love to explore more to see if there are simpler alternatives or ways we can mitigate this problem.
  2. This PR addresses the piece of upgrading a specific plugin to another version of the same plugin, but not the ability to replace a plugin with a new one (e.g., single owner plugin to a multi owner plugin). Of course, if the original has dependents, the new plugin would need to share the declared interface IDs in their dependents' manifests. But perhaps we should also revamp that: [Improvement] Correct usage of ERC-165 resources#11. Alternatively, maybe with [Improvement] Multiple validation function support resources#4, the "replace plugin" use case goes away, and we're left with just the "upgrade plugin" use case.
  3. Thought a bit more about the data migration piece. Retrieving data from plugins to migrate may not be feasible if the plugins store data in a way that is not iterable (e.g., mappings). Perhaps it might be worth revisiting one of @yoavw's early ideas in storing plugin data in the account (per-plugin getters/setters). If feasible, this would also fix any issues with "unclean" account upgrades, where plugins were not uninstalled before the upgrade, leaving plugins to think they are installed on the account, when the new account storage doesn't know about them. Alternatively, maybe user-supplied data for replacement plugins is okay.

Would love to continue our discussions either here or in erc6900/resources#13!

@jaypaik
Copy link
Collaborator

jaypaik commented Mar 14, 2024

I just read through your doc replacePlugin Implementation Spec by Decipher (this might be good to share in the issue), and I see you touch on a lot of the points here :)

@sm-stack
Copy link
Contributor Author

sm-stack commented Mar 17, 2024

Thanks for your feedback, @jaypaik! We are now discussing how to implement this, and I'll share the doc you mentioned at Issue#13.

The aspects you mentioned may need significant changes on this PR, so I'll close this and work at another branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants