Add Account framework docs and guides #5660
Conversation
Skipping MyAccountERC7913 avoids replacing: +import {Account} from "@openzeppelin/contracts/account/Account.sol";
-import {Account} from "../../.../account/Account.sol"; |
What I don't understand is that I would expect skipped contract to just not cause anything, no transpiled code generated, nothing to compile, no possible error. I was under the impression that skipping is a way around the issue, not that skipping would cause issues. |
| @@ -151,7 +151,7 @@ abstract contract MultiSignerERC7913 is AbstractSigner { | |||
| * | |||
| * Requirements: | |||
| * | |||
| * * The {signers}'s length must be `>=` to the {threshold}. Throws {MultiSignerERC7913UnreachableThreshold} if not. | |||
| * * The {getSignerCount}'s length must be `>=` to the {threshold}. Throws {MultiSignerERC7913UnreachableThreshold} if not. | |||
There was a problem hiding this comment.
| * * The {getSignerCount}'s length must be `>=` to the {threshold}. Throws {MultiSignerERC7913UnreachableThreshold} if not. | |
| * * The {getSignerCount} must be greater or equal than to the {threshold}. Throws {MultiSignerERC7913UnreachableThreshold} if not. |
There was a problem hiding this comment.
I think this makes the line too large, and not sure how to keep a single bullet point in 2 rows
| /// @dev Predict the address of the account | ||
| function predictAddress(bytes32 salt, bytes calldata callData) public view returns (address) { | ||
| return (_impl.predictDeterministicAddress(_saltedCallData(salt, callData), address(this))); | ||
| } | ||
|
|
||
| /// @dev Create clone accounts on demand | ||
| function cloneAndInitialize(bytes32 salt, bytes calldata callData) public returns (address) { | ||
| address predicted = predictAddress(salt, callData); | ||
| if (predicted.code.length == 0) { | ||
| _impl.cloneDeterministic(_saltedCallData(salt, callData)); | ||
| predicted.functionCall(callData); | ||
| } | ||
| return predicted; | ||
| } | ||
|
|
||
| function _saltedCallData(bytes32 salt, bytes calldata callData) internal pure returns (bytes32) { | ||
| // Scope salt to the callData to avoid front-running the salt with a different callData | ||
| return keccak256(abi.encodePacked(salt, callData)); | ||
| } |
There was a problem hiding this comment.
There is something interresting to notice when we pass arbitrary "initialize" calldata to the factory:
If we add more stuff after the abi encoded call (like a suffix), it won't be processed by the implementation (during initialization) but if will affect the hash. So technically, we can make the salt optional here and do
| /// @dev Predict the address of the account | |
| function predictAddress(bytes32 salt, bytes calldata callData) public view returns (address) { | |
| return (_impl.predictDeterministicAddress(_saltedCallData(salt, callData), address(this))); | |
| } | |
| /// @dev Create clone accounts on demand | |
| function cloneAndInitialize(bytes32 salt, bytes calldata callData) public returns (address) { | |
| address predicted = predictAddress(salt, callData); | |
| if (predicted.code.length == 0) { | |
| _impl.cloneDeterministic(_saltedCallData(salt, callData)); | |
| predicted.functionCall(callData); | |
| } | |
| return predicted; | |
| } | |
| function _saltedCallData(bytes32 salt, bytes calldata callData) internal pure returns (bytes32) { | |
| // Scope salt to the callData to avoid front-running the salt with a different callData | |
| return keccak256(abi.encodePacked(salt, callData)); | |
| } | |
| /// @dev Predict the address of the account | |
| function predictAddress(bytes calldata callData) public view returns (address) { | |
| return _impl.predictDeterministicAddress(keccak256(callData), address(this)); | |
| } | |
| /// @dev Create clone accounts on demand | |
| function cloneAndInitialize(bytes calldata callData) public returns (address) { | |
| address predicted = predictAddress(callData); | |
| if (predicted.code.length == 0) { | |
| _impl.cloneDeterministic(keccak256(callData)); | |
| predicted.functionCall(callData); | |
| } | |
| return predicted; | |
| } |
If someone what to use a salt, they will "just" have to call cloneAndInitialize with
abi.encodePacked(abi.encodeCall(Account.intialize, (<...args...>)), salt);
There was a problem hiding this comment.
The question is, given that both versions have similar features, do we want:
- a more simpler version, easy to read, but where some features are "hidden" (and optional)
- a more complex version, harder to read, but where the extra feature are "in your face"
There was a problem hiding this comment.
No strong preference, I think the factory is illustrative and the documentation already suggests some key considerations like "ensure the account address is deterministically tied to the initial owners". So all good with the current suggestion.
Overall I agree that an automated factory generation in Wizard must do the job better than this, and we may want to go for that option instead of a canonical factory in OZ contracts.
Check #5659 before
PR Checklist
npx changeset add)