Add missing imports for ERC-7579 modular accounts without a signer #661
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@openzeppelin/wizard': patch | ||
| --- | ||
|
|
||
| Add missing imports for modular accounts without a signer |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -172,6 +172,16 @@ function addERC7579Modules(c: ContractBuilder, opts: AccountOptions): void { | |
|
|
||
| // Accounts that use ERC7579 without a signer must be constructed with at least one module (executor of validation) | ||
| if (!opts.signer) { | ||
| c.addImportOnly({ | ||
| name: 'MODULE_TYPE_VALIDATOR', | ||
| path: '@openzeppelin/contracts/interfaces/draft-IERC7579.sol', | ||
| transpiled: false, // name doesn't start with "I" so its not recognized as an "interface object" | ||
| }); | ||
| c.addImportOnly({ | ||
| name: 'MODULE_TYPE_EXECUTOR', | ||
| path: '@openzeppelin/contracts/interfaces/draft-IERC7579.sol', | ||
| transpiled: false, // name doesn't start with "I" so its not recognized as an "interface object" | ||
| }); | ||
|
There was a problem hiding this comment. afaik we have a test that checks whether the contracts compile in packages/core/solidity/src/test.ts, how come it missed it? There was a problem hiding this comment.
This zip tests do check compilation ...and a bit more (deployment) So I tried to add zip tests that include a account with 7579 and no module. It was a pain and in the end was blocked by the tests script failling because they need a valid ERC7579 module to run the construction/initialization. AFAIK, we don't have tests that check compilation without also deploying the contract (and proxy). There was a problem hiding this comment.
Added it (by using exported options defined elsewhere, so any new values in the future won't be missed) in 88cf4c8. However, this now reveals compile warnings due to the below, which fails the tests. For example, this occurs when no options are selected: function _rawSignatureValidation(bytes32 hash, bytes calldata signature)
internal
view
override
returns (bool)
{
// Custom validation logic
return false;
}Should we ignore these warnings in the tests? There was a problem hiding this comment. My bad, I got confused between test.ts and account.test.ts There was a problem hiding this comment. there are multiple options to make that warning go away, I'm not sure which one is best:
There was a problem hiding this comment. or selectively ignore the warnings our tests by filtering them in There was a problem hiding this comment.
Is there any use case for a contract that doesn't support module AND doesn't have a signer? If not, then I think we should go with this option. There was a problem hiding this comment.
I think the usecase is "wizard giving something incomplete that users will have to finish". Otherwize, contract is unusable. There was a problem hiding this comment.
That is what I meant in 5. There was a problem hiding this comment.
For comparison, ERC20 can also be "incomplete" if users do not have either Premint or Mintable, but we don't give an error. In the case of Account, since Wizard is a dev tool rather than a deployment tool, I think the "incomplete" case is acceptable. However, we should add a clearly actionable TODO comment for
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| c.addConstructorArgument({ type: 'uint256', name: 'moduleTypeId' }); | ||
| c.addConstructorArgument({ type: 'address', name: 'module' }); | ||
| c.addConstructorArgument({ type: 'bytes calldata', name: 'initData' }); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.