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.

Sign up for GitHub

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

Jump to bottom

Make ENS label optional in the wallet factory #135

Merged
merged 2 commits into from
Jul 16, 2020
Merged

Make ENS label optional in the wallet factory #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
165c70c
make ENS label optional in the wallet factory
juniset Jul 15, 2020
f00ca2a
Fix typo
elenadimitrova Jul 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 13 additions & 19 deletions contracts/wallet/WalletFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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);
Expand All @@ -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.
*/
Expand All @@ -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);
Expand Down Expand Up @@ -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(
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
}
}
43 changes: 22 additions & 21 deletions test/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 should be created");
});

it("should fail to create when the guardian is empty", async () => {
// we create the wallet
const label = `wallet${index}`;
Expand All @@ -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];
Expand Down Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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}`;
Expand Down Expand Up @@ -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}`;
Expand Down