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(create): add performCreate[2] method. #168

Closed
wants to merge 2 commits into from

Conversation

andysim3d
Copy link

@andysim3d andysim3d commented Jul 30, 2024

Motivation

Enable create / create2 from ModularAccount.

Solution

@andysim3d andysim3d requested a review from jaypaik July 30, 2024 19:43
@jaypaik jaypaik requested a review from a team July 30, 2024 20:04
Copy link
Collaborator

@jaypaik jaypaik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this in! Some nits.

src/account/UpgradeableModularAccount.sol Outdated Show resolved Hide resolved
src/account/UpgradeableModularAccount.sol Outdated Show resolved Hide resolved
src/account/UpgradeableModularAccount.sol Outdated Show resolved Hide resolved
test/account/UpgradeableModularAccount.t.sol Show resolved Hide resolved
test/account/UpgradeableModularAccount.t.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging that we're missing access control on these methods. In the past we could have used a modifier like wrapNativeFunction, but we've removed that in this repo for code size optimization. We should wrap the two functions with the following:

(FunctionReference[][] memory postExecHooks, bytes[] memory postHookArgs) = _preNativeFunction();
// function body
_postNativeFunction(postExecHooks, postHookArgs);

@andysim3d
Copy link
Author

Flagging that we're missing access control on these methods. In the past we could have used a modifier like wrapNativeFunction, but we've removed that in this repo for code size optimization. We should wrap the two functions with the following:

(FunctionReference[][] memory postExecHooks, bytes[] memory postHookArgs) = _preNativeFunction();
// function body
_postNativeFunction(postExecHooks, postHookArgs);

I'll address in following pr. right now I am not sure the correct modifier to use here.

@andysim3d andysim3d requested a review from jaypaik August 14, 2024 16:13
@adamegyed adamegyed requested review from a team and removed request for a team September 5, 2024 17:27
@Zer0dot
Copy link
Contributor

Zer0dot commented Sep 26, 2024

Superseded for MAv2 by #192, closing this as it's targeting MAv1. Great work though Andy!

@Zer0dot Zer0dot closed this Sep 26, 2024
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