feat: move token receiver functions into account #124
Conversation
jaypaik
requested review from
adam-alchemy and
howydev
and removed request for
adam-alchemy
jaypaik
force-pushed
the
01-26-feat_move_token_receiver_functions_into_account
branch
4 times, most recently
from
7b02860 to
e58f45c
Compare
jaypaik
force-pushed
the
01-26-feat_move_token_receiver_functions_into_account
branch
2 times, most recently
from
b79fb5e to
4593415
Compare
| - [Modular Account implementation](src/account) | ||
| - [Modular Account factory](src/factory) | ||
| - 2 ERC-6900 compatible plugins: | ||
| - [MultiOwnerPlugin](src/plugins/owner) is a plugin supporting 1+ ECDSA owners. |
There was a problem hiding this comment.
probably not the PR for it, but we do support contract owners too.
jaypaik
force-pushed
the
01-26-feat_move_token_receiver_functions_into_account
branch
from
2d3c9c8 to
f4ed920
Compare
jaypaik
deleted the
01-26-feat_move_token_receiver_functions_into_account
branch
| if (interfaceId == type(IPlugin).interfaceId) { | ||
| revert IPluginInterfaceNotAllowed(); | ||
| if (interfaceId == type(IPlugin).interfaceId || interfaceId == _INVALID_INTERFACE_ID) { | ||
| revert InterfaceNotAllowed(); |
There was a problem hiding this comment.
The following interface types are also not relevant, although it doesn't really hurt to add them
- type(IERC721Receiver).interfaceId
- type(IERC777Recipient).interfaceId
- type(IERC1155Receiver).interfaceId
There was a problem hiding this comment.
True. I'll probably leave this one as is since it doesn't hurt, and adding this in will increase contract size.
There was a problem hiding this comment.
If you want to reduce the contract size || interfaceId == _INVALID_INTERFACE_ID
is also not really neccesary as it also checked in supportsInterface
There was a problem hiding this comment.
Agreed! But I think this is ever so slightly different, which may justify this one.
- Without this check, a plugin wanting to add
_INVALID_INTERFACE_IDwill successfully install but always fail whensupportsInterface(_INVALID_INTERFACE_ID)is called, which could be unexpected. - Without the token receiver interface checks here, a plugin wanting to add
type(IERC721Receiver).interfaceIdwill successfully install, andsupportsInterface(type(IERC721Receiver).interfaceId)will return true as expected.
| _doPreExecHooks(_getAccountStorage().selectorData[msg.sig], ""); | ||
| selector = IERC721Receiver.onERC721Received.selector; | ||
| _postNativeFunction(postExecHooks, postHookArgs); | ||
| } |
There was a problem hiding this comment.
In order to override onERC721Received(), while still keeping the hooks, it might be useful
that onERC721Received() calls another function (which is initially empty), that can be overriden
There was a problem hiding this comment.
Good idea. Addressed in #127 !
Motivation
Opening this to collect feedback and consensus around merging the token receiver functions into the account as native functions vs. installing them from a plugin. Motivation is to decrease the amount of gas spent on account creation.
Solution
TokenReceiverPlugin.MultiOwnerTokenReceiverMSCAFactory.tokensReceived,onERC721Received,onERC1155Received,onERC1155BatchReceivedtoUpgradeableModularAccount.supportsInterface.tokensReceived,onERC721Received,onERC1155Received,onERC1155BatchReceivedtoKnownSelectors.isNativeFunction.Tests in #126