diff --git a/contracts/wallet/WalletFactory.sol b/contracts/wallet/WalletFactory.sol index ef8d5cd04..dcfc1c6fc 100644 --- a/contracts/wallet/WalletFactory.sol +++ b/contracts/wallet/WalletFactory.sol @@ -69,7 +69,7 @@ contract WalletFactory is Owned, Managed { * The wallet is created using the CREATE opcode. * @param _owner The account address. * @param _modules The list of modules. - * @param _label ENS label of the new wallet, e.g. franck. + * @param _label Optional ENS label of the new wallet, e.g. franck. * @param _guardian The guardian address. */ function createWallet( @@ -82,7 +82,6 @@ contract WalletFactory is Owned, Managed { onlyManager { validateInputs(_owner, _modules, _guardian); - validateENSLabel(_label); Proxy proxy = new Proxy(walletImplementation); address payable wallet = address(proxy); configureWallet(BaseWallet(wallet), _owner, _modules, _label, _guardian); @@ -94,7 +93,7 @@ contract WalletFactory is Owned, Managed { * The wallet is created using the CREATE2 opcode. * @param _owner The account address. * @param _modules The list of modules. - * @param _label ENS label of the new wallet, e.g. franck. + * @param _label Optional ENS label of the new wallet, e.g. franck. * @param _guardian The guardian address. * @param _salt The salt. */ @@ -110,8 +109,6 @@ contract WalletFactory is Owned, Managed { returns (address _wallet) { validateInputs(_owner, _modules, _guardian); - validateENSLabel(_label); - bytes32 newsalt = newSalt(_salt, _owner, _modules, _guardian); Proxy proxy = new Proxy{salt: newsalt}(walletImplementation); address payable wallet = address(proxy); @@ -180,7 +177,7 @@ contract WalletFactory is Owned, Managed { * @param _wallet The target wallet * @param _owner The account address. * @param _modules The list of modules. - * @param _label ENS label of the new wallet, e.g. franck. + * @param _label Optional ENS label of the new wallet, e.g. franck. * @param _guardian The guardian address. */ function configureWallet( @@ -203,7 +200,7 @@ contract WalletFactory is Owned, Managed { // add guardian IGuardianStorage(guardianStorage).addGuardian(address(_wallet), _guardian); - // register ENS + // register ENS if needed registerWalletENS(address(_wallet), _label); // remove the factory from the authorised modules _wallet.authoriseModule(address(this), false); @@ -234,23 +231,20 @@ contract WalletFactory is Owned, Managed { require(_guardian != (address(0)), "WF: guardian cannot be null"); } - function validateENSLabel(string memory _label) internal pure { - bytes memory labelBytes = bytes(_label); - require(labelBytes.length != 0, "WF: ENS label must be defined"); - } - /** * @dev Register an ENS subname to a wallet. * @param _wallet The wallet address. * @param _label ENS label of the new wallet (e.g. franck). */ function registerWalletENS(address payable _wallet, string memory _label) internal { - // claim reverse - address ensResolver = IENSManager(ensManager).ensResolver(); - bytes memory methodData = abi.encodeWithSignature("claimWithResolver(address,address)", ensManager, ensResolver); - address ensReverseRegistrar = IENSManager(ensManager).getENSReverseRegistrar(); - BaseWallet(_wallet).invoke(ensReverseRegistrar, 0, methodData); - // register with ENS manager - IENSManager(ensManager).register(_label, _wallet); + if (bytes(_label).length > 0) { + // claim reverse + address ensResolver = IENSManager(ensManager).ensResolver(); + bytes memory methodData = abi.encodeWithSignature("claimWithResolver(address,address)", ensManager, ensResolver); + address ensReverseRegistrar = IENSManager(ensManager).getENSReverseRegistrar(); + BaseWallet(_wallet).invoke(ensReverseRegistrar, 0, methodData); + // register with ENS manager + IENSManager(ensManager).register(_label, _wallet); + } } } diff --git a/test/factory.js b/test/factory.js index 06ca0dd04..5ff1c5b23 100644 --- a/test/factory.js +++ b/test/factory.js @@ -221,6 +221,15 @@ describe("Wallet Factory", function () { assert.equal(res, ensResolver.contractAddress); }); + it("should create when there is no ENS", async () => { + const modules = [module1.contractAddress, module2.contractAddress]; + // we create the wallet + const tx = await factory.from(infrastructure).createWallet(owner.address, modules, NO_ENS, guardian.address); + const txReceipt = await factory.verboseWaitForTransaction(tx); + const walletAddr = txReceipt.events.filter((event) => event.event === "WalletCreated")[0].args.wallet; + assert.notEqual(walletAddr, ZERO_ADDRESS, "wallet shoudl be created"); + }); + it("should fail to create when the guardian is empty", async () => { // we create the wallet const label = `wallet${index}`; @@ -236,12 +245,6 @@ describe("Wallet Factory", function () { "WF: cannot assign with less than 1 module"); }); - it("should fail to create when there is no ENS", async () => { - const modules = [module1.contractAddress, module2.contractAddress]; - await assert.revertWith(factory.from(infrastructure).createWallet(owner.address, modules, NO_ENS, guardian.address), - "WF: ENS label must be defined"); - }); - it("should fail to create with an existing ENS", async () => { const label = `wallet${index}`; const modules = [module1.contractAddress, module2.contractAddress]; @@ -271,13 +274,6 @@ describe("Wallet Factory", function () { await assert.revertWith(factory.from(infrastructure).createWallet(owner.address, modules, label, guardian.address), "WF: one or more modules are not registered"); }); - - it("should fail to create with empty label", async () => { - const label = ""; - const modules = [module1.contractAddress]; - await assert.revertWith(factory.from(infrastructure).createWallet(owner.address, modules, label, guardian.address), - "WF: ENS label must be defined"); - }); }); describe("Create wallets with CREATE2", () => { @@ -360,6 +356,19 @@ describe("Wallet Factory", function () { assert.equal(res, ensResolver.contractAddress); }); + it("should create when there is no ENS", async () => { + const salt = utilities.generateSaltValue(); + const modules = [module1.contractAddress, module2.contractAddress]; + // we get the future address + const futureAddr = await factory.getAddressForCounterfactualWallet(owner.address, modules, guardian.address, salt); + // we create the wallet + const tx = await factory.from(deployer).createCounterfactualWallet(owner.address, modules, NO_ENS, guardian.address, salt); + const txReceipt = await factory.verboseWaitForTransaction(tx); + const walletAddr = txReceipt.events.filter((event) => event.event === "WalletCreated")[0].args.wallet; + // we test that the wallet is at the correct address + assert.equal(futureAddr, walletAddr, "should have the correct address"); + }); + it("should create with the correct guardian", async () => { const salt = utilities.generateSaltValue(); const label = `wallet${index}`; @@ -402,14 +411,6 @@ describe("Wallet Factory", function () { "WF: cannot assign with less than 1 module"); }); - it("should fail to create when there is no ENS", async () => { - const salt = utilities.generateSaltValue(); - const label = ""; - const modules = [module1.contractAddress, module2.contractAddress]; - await assert.revertWith(factory.from(deployer).createCounterfactualWallet(owner.address, modules, label, guardian.address, salt), - "WF: ENS label must be defined"); - }); - it("should fail to create when the guardian is empty", async () => { const salt = utilities.generateSaltValue(); const label = `wallet${index}`;