-
Notifications
You must be signed in to change notification settings - Fork 25
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
Split up base account to fit under size limit #19
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks.
@@ -27,17 +27,20 @@ import { | |||
PluginManifest | |||
} from "../interfaces/IPlugin.sol"; | |||
|
|||
abstract contract PluginManagerInternals is IPluginManager { | |||
contract PluginManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What're the tradeoffs between this approach vs library external function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a library with external functions is a bit less straightforward to deploy through foundry. This pattern of manually delegatecall
ing a custom contract also lets us inspect calls between the account and the plugin manager implementation better, because we know the abi-encoding format. Libraries would use the solidity-internal custom encoding for libraries: https://docs.soliditylang.org/en/v0.8.19/contracts.html#function-signatures-and-selectors-in-libraries
Also, as a note for the future: this change is intended to make it easier to work with the reference implementation as of the current version: Spec Update 6, aka v0.6.0. We hope to remove this workaround as the standard gets simplified in the future version v0.7.0 and have a single-contract reference implementation, even without optimizations.
This reverts commit e1a7882.
Motivation
The contract size of
UpgradeableModularAccount
exceeds the 24.5kb size limit. While the intention behind the reference implementation is not that it should be used in production, this makes it difficult to test integrations with outside of the context of foundry tests and other forked environments.Solution
Split out
PluginManagerInternals
into a distinctPluginManager
contract.UpgradeableModularAccount
performsdelegatecall
s into this contract, much like a library. However, using a delegate-only contract over a library gives us easier integration into deployment tools like scripts.This brings the size for both contracts under the limit.
Also fixes a compiler warning and removes the ignored error codes.