feat(v2): multi owner light account #32

jaypaik · 2024-03-10T19:53:39Z

Used LinkedListSetLib from modular-account to optimize storing multiple owners. We'll probably want to pull this out to a separate repo eventually.
Added tests.

Next up:

Potentially pull common things out into LightAccountBase. refactor(v2): inherit from BaseLightAccount #33
Update LightAccount with style updates (like using /// instead of /** */) that was applied here. style(v2): consistent natspec style #34
Proxy work. feat(v2): use solady's minimal ERC-1967 proxy and UUPSUpgradeable #35

jaypaik · 2024-03-11T15:17:15Z

src/MultiOwnerLightAccount.sol

+    bytes32 private constant _LA_MSG_TYPEHASH = keccak256("MultiOwnerLightAccountMessage(bytes message)");
+    bytes32 private constant _NAME_HASH = keccak256("MultiOwnerLightAccount");
+    bytes32 private constant _VERSION_HASH = keccak256("1");


Note: this is going to get refactored later in the ERC-1271 PR (upcoming). I chose to make these different from the vanilla LightAccount here, but would love thoughts on the safety of just keeping it consistent across the different flavors.

hm... I think this might make a difference if the LA is upgraded between the single owner and the multi owner versions. If they're the same, it would allow a "replay" of previous signatures using only one of the owners, but that might be OK? Since 1271 signatures expect the caller to perform the replay protections.

I think the bigger concerns are around SDK usage, where the signing requests might need to be aware of the version, which could cause debugging issues if signatures aren't working due to using the wrong LA version 712 wrapper.

I'm leaning towards making it the same for ease of SDK integration, but could be convinced otherwise.

So #36 now removes the dependency on the contract to define the typehash, and pushes that responsibility to the client. Each flavor of account still defines its own name and version to prevent replays against different account versions or implementations.

Solady's EIP712 that we use implements EIP-5267 so clients can call eip712Domain to get the name and version, which should make it easier on clients.

adam-alchemy

Looks good, had some comments.

src/MultiOwnerLightAccount.sol

adam-alchemy · 2024-03-11T23:14:04Z

src/MultiOwnerLightAccount.sol

+    bytes32 private constant _LA_MSG_TYPEHASH = keccak256("MultiOwnerLightAccountMessage(bytes message)");
+    bytes32 private constant _NAME_HASH = keccak256("MultiOwnerLightAccount");
+    bytes32 private constant _VERSION_HASH = keccak256("1");


hm... I think this might make a difference if the LA is upgraded between the single owner and the multi owner versions. If they're the same, it would allow a "replay" of previous signatures using only one of the owners, but that might be OK? Since 1271 signatures expect the caller to perform the replay protections.

I think the bigger concerns are around SDK usage, where the signing requests might need to be aware of the version, which could cause debugging issues if signatures aren't working due to using the wrong LA version 712 wrapper.

I'm leaning towards making it the same for ease of SDK integration, but could be convinced otherwise.

adam-alchemy · 2024-03-11T23:18:59Z

src/MultiOwnerLightAccount.sol

+
+    function _initialize(address[] calldata owners_) internal virtual {
+        emit LightAccountInitialized(_ENTRY_POINT, owners_);
+        _updateOwners(owners_, new address[](0));


Can we replace this with _addOwnersOrRevert? It would remove the OwnersUpdated event, but this data is present already in the LightAccountInitialized event.

We emit OwnershipTransferred for LightAccount on initialize, so this was an attempt to make things consistent there. Shall we remove it there as well?

ah... from an integration standpoint, it's probably easier to just index the one event OwnershipTransferred. Arguably the LightAccountInitialized event isn't really needed, but I guess it's fine to keep as-is and have some small duplcation.

adam-alchemy · 2024-03-11T23:22:41Z

src/MultiOwnerLightAccount.sol

+    /// @dev Revert if the caller is not one of the owners or the account itself (when redirected through `execute`).
+    function _onlyOwners() internal view {
+        if (msg.sender != address(this) && !_getStorage().owners.contains(msg.sender.toSetValue())) {
+            revert NotAuthorized(msg.sender);
+        }
+    }
+
+    /// @dev Require that the call is from the entry point or an owner.
+    function _requireFromEntryPointOrOwner() internal view {
+        if (msg.sender != address(entryPoint()) && !_getStorage().owners.contains(msg.sender.toSetValue())) {
+            revert NotAuthorized(msg.sender);
+        }
+    }


Would it make sense to merge these two modifiers? There are some workflows where needing the call to be wrapped in execute is just an added step, like updateOwners.

Yeah. Might be good to just authorize all of (self, owner, entrypoint). Will think about this and address up the stack.

adam-alchemy · 2024-03-11T23:23:20Z

src/MultiOwnerLightAccount.sol

+        for (uint256 i = 0; i < length; ++i) {
+            if (SignatureChecker.isValidERC1271SignatureNow(owners_[i], digest, signature)) {
+                return true;
+            }


In a later PR, we should split the signature checking based on a provided address, rather than iterating.

adam-alchemy · 2024-03-11T23:25:18Z

src/MultiOwnerLightAccountFactory.sol

+        );
+    }
+
+    /// @dev `owners` must be in strictly ascending order and not include the 0 address. Its length must not be empty


Maybe add a comment about how the sorting ensures a canonical counterfactual for a given set of starting owners.

v1.0.x

jaypaik force-pushed the 03-06-feat_4337_v0.7_updates branch 2 times, most recently from 4b1bb9d to dea597b Compare March 10, 2024 20:53

jaypaik force-pushed the 03-03-feat_multi_owner_light_account branch from 3e79e59 to d60557c Compare March 10, 2024 20:56

jaypaik changed the title ~~feat: multi owner light account~~ feat(v2): multi owner light account Mar 10, 2024

jaypaik force-pushed the 03-03-feat_multi_owner_light_account branch from d60557c to 6749293 Compare March 11, 2024 02:12

jaypaik commented Mar 11, 2024

View reviewed changes

adam-alchemy approved these changes Mar 11, 2024

View reviewed changes

Base automatically changed from 03-06-feat_4337_v0.7_updates to develop March 12, 2024 14:41

jaypaik added 3 commits March 12, 2024 10:41

feat: multi owner light account

e89b091

forge install: modular-account

18fa3bd

v1.0.x

feat: use LinkedListSetLib and tests

cb7b2f6

jaypaik force-pushed the 03-03-feat_multi_owner_light_account branch from 6749293 to cb7b2f6 Compare March 12, 2024 14:43

docs: update comment on _validateOwnersArray

d01ddc6

jaypaik merged commit 84ced68 into develop Mar 12, 2024
2 checks passed

jaypaik deleted the 03-03-feat_multi_owner_light_account branch March 12, 2024 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(v2): multi owner light account #32

feat(v2): multi owner light account #32

jaypaik commented Mar 10, 2024 •

edited

Loading

jaypaik Mar 11, 2024

adam-alchemy Mar 11, 2024

jaypaik Mar 12, 2024

adam-alchemy left a comment

adam-alchemy Mar 11, 2024

adam-alchemy Mar 11, 2024

jaypaik Mar 12, 2024

adam-alchemy Mar 12, 2024

adam-alchemy Mar 11, 2024

jaypaik Mar 12, 2024

adam-alchemy Mar 11, 2024

adam-alchemy Mar 11, 2024

feat(v2): multi owner light account #32

feat(v2): multi owner light account #32

Conversation

jaypaik commented Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-alchemy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaypaik commented Mar 10, 2024 •

edited

Loading