feat: add account factory #105
Conversation
src/account/AccountFactory.sol
Outdated
| function createAccount( | ||
| address owner, | ||
| uint256 salt, | ||
| uint32 entityId, |
There was a problem hiding this comment.
We could opt to always use an entityId of 0 on the factory by default, to make it simpler on the SDK
There was a problem hiding this comment.
Hmm this shouldn't be in the SDK right? I think we could always have the SDK default to 0 too, but allow flexibility in the contracts
There was a problem hiding this comment.
What would the flexibility be needed for, though? Even if it were necessary to do something else, that could just be set after deployment.
|
Will also make another change, adding the sig validator addr into the counterfactual. Else I think it might be possible for a malicious party to take over another persons counterfactual |
src/account/AccountFactory.sol
Outdated
| import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol"; | ||
|
|
||
| contract AccountFactory is Ownable { | ||
| UpgradeableModularAccount public accountImplementation; |
There was a problem hiding this comment.
I think we should make this immutable and just deploy new factories for new versions, this way deployment saves an SLOAD, wdyt?
There was a problem hiding this comment.
bump, if we don't support upgrading this, we should make it immutable
There was a problem hiding this comment.
whoops, missed it, just made the change
src/account/AccountFactory.sol
Outdated
| UpgradeableModularAccount public accountImplementation; | ||
| bytes32 private immutable _PROXY_BYTECODE_HASH; | ||
| uint32 public constant UNSTAKE_DELAY = 1 weeks; | ||
| IEntryPoint public entryPoint; |
There was a problem hiding this comment.
Same comment as acountImplementation, might benefit from being an immutable, though ofc this depends if we want this factory to be used in prod or not
howydev
force-pushed
the
howy/add-factory
branch
from
afc5f27 to
4b9b766
Compare
howydev
force-pushed
the
howy/add-factory
branch
from
485e0c5 to
b2e06d0
Compare
test/account/AccountFactory.sol
Outdated
| import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; | ||
| import {AccountTestBase} from "../utils/AccountTestBase.sol"; | ||
|
|
||
| contract AccountFactoryTest is AccountTestBase { |
There was a problem hiding this comment.
Nit: rename file to AccountFactory.t.sol
test/account/AccountFactory.sol
Outdated
|
|
||
| // Assert the return addresses are the same | ||
| assertEq(address(account), address(account2)); | ||
| } |
There was a problem hiding this comment.
Could we add a test that verifies the address returned by getAddress() and the account created & returned by createAccount are the same?
src/account/AccountFactory.sol
Outdated
| import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol"; | ||
|
|
||
| contract AccountFactory is Ownable { | ||
| UpgradeableModularAccount public accountImplementation; |
There was a problem hiding this comment.
bump, if we don't support upgrading this, we should make it immutable
No description provided.