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: SMA 3 - Appended Single Signer Address #123

Merged
merged 34 commits into from
Aug 20, 2024

Conversation

Zer0dot
Copy link
Contributor

@Zer0dot Zer0dot commented Aug 1, 2024

Motivation

The third exploration for a semi-modular account, following up from #121 and #119, this implementation combines both by appending only the original owner's address, while hardcoding the fallback signer logic into the account.

Solution

Append the owner's address to the proxy bytecode at account creation time, then, when a validation equivalent to ModuleEntity.wrap(bytes24(type(uint192).max)) is passed, opt to use, the fallback validation.

This validation first checks if it's enabled, then if there's a fallback signer set in storage, or defaults to the bytecode-appended signer.

This allows for reduced deploy costs without much overhead for regular flows.

A lot is still TBD-- such as what to do about the initializer, and where to put the storage for the fallback signer (as well as whether or not to even allow updating it; since users can just install a standard SingleSigner global validation instead).

@Zer0dot Zer0dot force-pushed the zer0dot/sma-fallback-bytecode-signer branch 2 times, most recently from e5f881a to 1ad0bd8 Compare August 12, 2024 17:40
@Zer0dot Zer0dot force-pushed the zer0dot/sma-fallback-bytecode-signer branch from e23a856 to ba63f8d Compare August 15, 2024 19:47
@Zer0dot Zer0dot marked this pull request as ready for review August 15, 2024 19:50
@@ -25,7 +25,7 @@ depth = 10
[profile.optimized-build]
via_ir = true
test = 'src'
optimizer_runs = 15000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may want to revisit codesize in the future. It's unfortunate to reduce optimizer runs, but it's still more than enough.

@Zer0dot Zer0dot force-pushed the zer0dot/sma-fallback-bytecode-signer branch from 5394a22 to 4ecf2ec Compare August 16, 2024 21:24
@Zer0dot Zer0dot requested a review from a team August 16, 2024 21:46
@jaypaik jaypaik changed the title [DRAFT] SMA 3: Appended Single Signer Address feat: SMA 3 - Appended Single Signer Address Aug 19, 2024
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.

Great work! Had some comments.

.github/workflows/test.yml Show resolved Hide resolved
script/Deploy.s.sol Outdated Show resolved Hide resolved
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/script/Deploy.s.t.sol Show resolved Hide resolved
test/utils/AccountTestBase.sol Show resolved Hide resolved
@@ -1,4 +1,4 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.25;

uint32 constant TEST_DEFAULT_VALIDATION_ENTITY_ID = 1;
uint32 constant TEST_DEFAULT_VALIDATION_ENTITY_ID = type(uint32).max;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause the default validation to coincide with the direct call validation entity ID. Could you double-check that this doesn't break assumptions for those tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I dug into this a good bit, the only time we actually care about the entityId for direct-call validations is to build the entire ModuleEntity-- so the last 4 bytes will be 0xff in both cases, but since we only index with the full ModuleEntity, we're reliant on the module address anyway.

test/account/PerHookData.t.sol Outdated Show resolved Hide resolved
test/module/AllowlistModule.t.sol Outdated Show resolved Hide resolved
@Zer0dot Zer0dot force-pushed the zer0dot/sma-fallback-bytecode-signer branch from 385c116 to 7d713ba Compare August 19, 2024 22:16
@Zer0dot Zer0dot force-pushed the zer0dot/sma-fallback-bytecode-signer branch from 7d713ba to 9d43bde Compare August 19, 2024 22:16
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.

Looks good! The simple refactors to UpgradeableModularAccount to make this possible are great.

@Zer0dot Zer0dot merged commit 98507f8 into develop Aug 20, 2024
3 checks passed
@Zer0dot Zer0dot deleted the zer0dot/sma-fallback-bytecode-signer branch August 20, 2024 19:39
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.

2 participants