Skip to content
This repository has been archived by the owner on Apr 27, 2022. It is now read-only.

Add test verifying deterministic deployment of proxy #398

Merged
merged 1 commit into from
Feb 3, 2021

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Feb 2, 2021

Add a unit test that verifies that the authenticator proxy is at a deterministically deployed.

Test Plan

It is a test!

@nlordell nlordell requested review from bh2smith and fedgiac February 2, 2021 15:09
@nlordell nlordell mentioned this pull request Feb 2, 2021
Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of type magic!

@nlordell
Copy link
Contributor Author

nlordell commented Feb 2, 2021

A bit of type magic!

A refinement of my previous incantation - it was because Proxy wasn't exactly an Artifact (it was missing some fields we didn't care about) so I needed to change things a bit to make the compiler happy.

Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, fancy stuff! Very nice!

@@ -1,6 +1,7 @@
import { expect } from "chai";
import { Contract, Wallet } from "ethers";
import { artifacts } from "hardhat";
import Proxy from "hardhat-deploy/extendedArtifacts/EIP173Proxy.json";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is Key!

@nlordell
Copy link
Contributor Author

nlordell commented Feb 3, 2021

@bh2smith This merges into your branch for #351, feel free to merge if you want (or I can rebase and merge over main after the aforementioned PR gets merged).

Base automatically changed from 167/upgradable_authenticator to main February 3, 2021 14:30
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good


EnumerableSet.AddressSet private solvers;

// solhint-disable-next-line no-empty-blocks
constructor(address initialOwner) CustomInitiallyOwnable(initialOwner) {}
function initializeManager(address _manager) external {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why separate this from the constructor? I assume because EIP173 only works with empty constructors? Would the Ownable base contract now work again instead of implementing it ourselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This because the base branch was merged right before you

Why separate this from the constructor?

Its now a "proxy upgrade function" since proxied contracts cant have constructors.

@nlordell nlordell force-pushed the verify-deterministic-proxy-address branch from 182e296 to 56e0d71 Compare February 3, 2021 15:50
@nlordell nlordell added the merge when green Merge when green! label Feb 3, 2021
@mergify mergify bot merged commit 0758224 into main Feb 3, 2021
@mergify mergify bot deleted the verify-deterministic-proxy-address branch February 3, 2021 15:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
merge when green Merge when green!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants