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.

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

Provide Governance Token Bases to TokenVotingSetup Constructor as Arguments to Avoid initCode size limit on Goerli #425

Merged
merged 4 commits into from
Jun 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions packages/contracts/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- Revert with errors (`ConditionNotAContract`, `ConditionInterfacNotSupported`) if the `grantWithCondition` function in `PermissionManager` is called with a condition address that is not a `IPermissionCondition` implementation.
- `_grantWithCondition()` doesn't accept `ALLOW_FLAG` anymore as valid condition input.
- Changed `TokenVotingSetup` to receive the `GovernanceERC20` and `GovernanceWrappedERC20` base contracts as constructor arguments to reduce the `initCode` size because of limitations on the Goerli testnet.
- Revert with errors (`ConditionNotAContract`, `ConditionInterfacNotSupported`) if the `grantWithCondition` function in `PermissionManager` is called with a condition address that is not a `IPermissionCondition` implementation.
- `_grantWithCondition()` doesn't accept `ALLOW_FLAG` anymore as valid condition input.
- Revert with an error (`GrantWithConditionNotSupported`) if the `applySingleTargetPermissions` function in `PermissionManager` is called with `PermissionLib.Operation.GrantWithCondition`.
- Fixed logic bug in the `TokenVoting` and `AddresslistVoting` implementations that caused the `createProposal` function to emit the unvalidated `_startDate` and `_endDate` input arguments (that both can be zero) in the `ProposalCreated` event instead of the validated ones.
- Changed the `createProposal` functions in `Multisig` to allow creating proposals when the `_msgSender()` is listed in the current block.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,51 @@
import {HardhatRuntimeEnvironment} from 'hardhat/types';
import {DeployFunction} from 'hardhat-deploy/types';

import governanceERC20Artifact from '../../../../artifacts/src/token/ERC20/governance/GovernanceERC20.sol/GovernanceERC20.json';
import governanceWrappedERC20Artifact from '../../../../artifacts/src/token/ERC20/governance/GovernanceWrappedERC20.sol/GovernanceWrappedERC20.json';
import tokenVotingSetupArtifact from '../../../../artifacts/src/plugins/governance/majority-voting/token/TokenVotingSetup.sol/TokenVotingSetup.json';
import {MintSettings} from '../../../../test/token/erc20/governance-erc20';

const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
const {deployments, ethers} = hre;
const {deploy} = deployments;
const [deployer] = await ethers.getSigners();

const zeroDaoAddress = ethers.constants.AddressZero;
const zeroTokenAddress = ethers.constants.AddressZero;
const emptyName = '';
const emptySymbol = '';
const emptyMintSettings: MintSettings = {
receivers: [],
amounts: [],
};

// Deploy the bases for the TokenVotingSetup
const governanceERC20DeployResult = await deploy('GovernanceERC20', {
contract: governanceERC20Artifact,
from: deployer.address,
args: [zeroDaoAddress, emptyName, emptySymbol, emptyMintSettings],
log: true,
});

const governanceWrappedERC20DeployResult = await deploy(
'GovernanceWrappedERC20',
{
contract: governanceWrappedERC20Artifact,
from: deployer.address,
args: [zeroTokenAddress, emptyName, emptySymbol],
log: true,
}
);

// Deploy the TokenVotingSetup and provide the bases in the constructor
await deploy('TokenVotingSetup', {
contract: tokenVotingSetupArtifact,
from: deployer.address,
args: [],
args: [
governanceERC20DeployResult.address,
governanceWrappedERC20DeployResult.address,
],
log: true,
});
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import {ethers} from 'ethers';
import {DeployFunction} from 'hardhat-deploy/types';
import {TokenVotingSetup__factory} from '../../../../typechain';
import {setTimeout} from 'timers/promises';
Expand All @@ -22,19 +21,19 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
await setTimeout(30000);
}

hre.aragonToVerifyContracts.push(
await hre.deployments.get('GovernanceERC20')
);

hre.aragonToVerifyContracts.push(
await hre.deployments.get('GovernanceWrappedERC20')
);

hre.aragonToVerifyContracts.push(TokenVotingSetupDeployment);
hre.aragonToVerifyContracts.push({
address: await tokenVotingSetup.implementation(),
args: [],
});
hre.aragonToVerifyContracts.push({
address: await tokenVotingSetup.governanceERC20Base(),
args: [ethers.constants.AddressZero, '', '', [[], []]],
});
hre.aragonToVerifyContracts.push({
address: await tokenVotingSetup.governanceWrappedERC20Base(),
args: [ethers.constants.AddressZero, '', ''],
});
};

export default func;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@ import {HardhatRuntimeEnvironment} from 'hardhat/types';
import {PluginRepo__factory} from '../../../typechain';
import {getContractAddress, uploadToIPFS} from '../../helpers';

import governanceERC20Artifact from '../../../artifacts/src/token/ERC20/governance/GovernanceERC20.sol/GovernanceERC20.json';
import governanceWrappedERC20Artifact from '../../../artifacts/src/token/ERC20/governance/GovernanceWrappedERC20.sol/GovernanceWrappedERC20.json';
import tokenVotingSetupArtifact from '../../../artifacts/src/plugins/governance/majority-voting/token/TokenVotingSetup.sol/TokenVotingSetup.json';
import tokenVotingReleaseMetadata from '../../../src/plugins/governance/majority-voting/token/release-metadata.json';
import tokenVotingBuildMetadata from '../../../src/plugins/governance/majority-voting/token/build-metadata.json';
import {UPDATE_INFOS} from '../../../utils/updates';
import {MintSettings} from '../../../test/token/erc20/governance-erc20';

const TARGET_RELEASE = 1;

Expand All @@ -16,10 +19,41 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
const {deploy} = deployments;
const [deployer] = await ethers.getSigners();

const zeroDaoAddress = ethers.constants.AddressZero;
const zeroTokenAddress = ethers.constants.AddressZero;
const emptyName = '';
const emptySymbol = '';
const emptyMintSettings: MintSettings = {
receivers: [],
amounts: [],
};

// Deploy the bases for the TokenVotingSetup
const governanceERC20DeployResult = await deploy('GovernanceERC20', {
contract: governanceERC20Artifact,
from: deployer.address,
args: [zeroDaoAddress, emptyName, emptySymbol, emptyMintSettings],
log: true,
});

const governanceWrappedERC20DeployResult = await deploy(
'GovernanceWrappedERC20',
{
contract: governanceWrappedERC20Artifact,
from: deployer.address,
args: [zeroTokenAddress, emptyName, emptySymbol],
log: true,
}
);
Comment on lines +32 to +47
Copy link
Contributor Author

@heueristik heueristik Jun 22, 2023

Choose a reason for hiding this comment

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

We could re-use previously deployed GovernanceERC20 or GovernanceWrappedERC20 bases, as they didn't change (except the OZ version bump).

Should we do it? I would say it is better to pay a bit more for a fresh deployment (with the latest OZ updates) instead of having a potential address mix-up just to save a few bucks.

Other question: Will the new active_contract.json after the protocol upgrade contain the two governance token contract addresses automatically, or do we have to remember to add them there?
It could generally make sense to add all implementation/base addresses there.

@mathewmeconry @Rekard0

Copy link
Contributor

Choose a reason for hiding this comment

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

I would update them because they have changed with the OZ update.
Adding stuff to the active_contracts.json is a manual task and we can do that for sure


// Deploy the TokenVotingSetup and provide the bases in the constructor
const deployResult = await deploy('TokenVotingSetup', {
contract: tokenVotingSetupArtifact,
from: deployer.address,
args: [],
args: [
governanceERC20DeployResult.address,
governanceWrappedERC20DeployResult.address,
],
log: true,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ import {UPDATE_INFOS} from '../../../utils/updates';
const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
console.log('\nConcluding TokenVoting Plugin Update');

hre.aragonToVerifyContracts.push(
await hre.deployments.get('GovernanceERC20')
);

hre.aragonToVerifyContracts.push(
await hre.deployments.get('GovernanceWrappedERC20')
);

hre.aragonToVerifyContracts.push(
await hre.deployments.get('TokenVotingSetup')
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,20 +57,16 @@ contract TokenVotingSetup is PluginSetup {
/// @param length The array length of passed helpers.
error WrongHelpersArrayLength(uint256 length);

/// @notice The contract constructor, that deploys the bases.
constructor() {
governanceERC20Base = address(
new GovernanceERC20(
IDAO(address(0)),
"",
"",
GovernanceERC20.MintSettings(new address[](0), new uint256[](0))
)
);
governanceWrappedERC20Base = address(
new GovernanceWrappedERC20(IERC20Upgradeable(address(0)), "", "")
);
/// @notice The contract constructor deploying the plugin implementation contract and receiving the governance token base contracts to clone from.
/// @param _governanceERC20Base The base `GovernanceERC20` contract to create clones from.
/// @param _governanceWrappedERC20Base The base `GovernanceWrappedERC20` contract to create clones from.
constructor(
GovernanceERC20 _governanceERC20Base,
GovernanceWrappedERC20 _governanceWrappedERC20Base
) {
tokenVotingBase = new TokenVoting();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I intentionally left new TokenVoting() here.
This is to prevent that plugins setup and implementation accidentally get out of sync.
It makes also sense because we are more likely to re-use an already deployed governance token base.

governanceERC20Base = address(_governanceERC20Base);
governanceWrappedERC20Base = address(_governanceWrappedERC20Base);
}

/// @inheritdoc IPluginSetup
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers';
import {
ERC20,
ERC20__factory,
GovernanceERC20,
GovernanceERC20__factory,
GovernanceWrappedERC20,
GovernanceWrappedERC20__factory,
TokenVotingSetup,
TokenVotingSetup__factory,
Expand Down Expand Up @@ -54,6 +56,8 @@ const MINT_PERMISSION_ID = ethers.utils.id('MINT_PERMISSION');
describe('TokenVotingSetup', function () {
let signers: SignerWithAddress[];
let tokenVotingSetup: TokenVotingSetup;
let governanceERC20Base: GovernanceERC20;
let governanceWrappedERC20Base: GovernanceWrappedERC20;
let implementationAddress: string;
let targetDao: any;
let erc20Token: ERC20;
Expand All @@ -69,11 +73,39 @@ describe('TokenVotingSetup', function () {
minDuration: ONE_HOUR,
minProposerVotingPower: 0,
};
defaultTokenSettings = {addr: AddressZero, name: '', symbol: ''};

const emptyName = '';
const emptySymbol = '';

defaultTokenSettings = {
addr: AddressZero,
name: emptyName,
symbol: emptySymbol,
};
defaultMintSettings = {receivers: [], amounts: []};

const GovernanceERC20Factory = new GovernanceERC20__factory(signers[0]);
governanceERC20Base = await GovernanceERC20Factory.deploy(
AddressZero,
emptyName,
emptySymbol,
defaultMintSettings
);

const GovernanceWrappedERC20Factory = new GovernanceWrappedERC20__factory(
signers[0]
);
governanceWrappedERC20Base = await GovernanceWrappedERC20Factory.deploy(
AddressZero,
emptyName,
emptySymbol
);

const TokenVotingSetup = new TokenVotingSetup__factory(signers[0]);
tokenVotingSetup = await TokenVotingSetup.deploy();
tokenVotingSetup = await TokenVotingSetup.deploy(
governanceERC20Base.address,
governanceWrappedERC20Base.address
);

implementationAddress = await tokenVotingSetup.implementation();

Expand All @@ -91,6 +123,15 @@ describe('TokenVotingSetup', function () {
expect(await tokenVotingSetup.supportsInterface('0xffffffff')).to.be.false;
});

it('stores the bases provided through the constructor', async () => {
expect(await tokenVotingSetup.governanceERC20Base()).to.be.eq(
governanceERC20Base.address
);
expect(await tokenVotingSetup.governanceWrappedERC20Base()).to.be.eq(
governanceWrappedERC20Base.address
);
});

it('creates token voting base with the correct interface', async () => {
const factory = new TokenVoting__factory(signers[0]);
const tokenVoting = factory.attach(implementationAddress);
Expand Down