Skip to content

Enhance GovernorWithParams Testing: Edge Cases, Refactoring, and Documentation #5567

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enhance GovernorWithParams Testing: Edge Cases, Refactoring, and Documentation #5567

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
124 changes: 76 additions & 48 deletions test/governance/extensions/GovernorWithParams.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const { ethers } = require('hardhat');
const { expect } = require('chai');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');

const { GovernorHelper } = require('../../helpers/governance');
const { VoteType } = require('../../helpers/enums');
const { getDomain, ExtendedBallot } = require('../../helpers/eip712');
Expand All @@ -10,7 +9,6 @@ const TOKENS = [
{ Token: '$ERC20Votes', mode: 'blocknumber' },
{ Token: '$ERC20VotesTimestampMock', mode: 'timestamp' },
];

const name = 'OZ-Governor';
const version = '1';
const tokenName = 'MockToken';
Expand All @@ -19,7 +17,6 @@ const tokenSupply = ethers.parseEther('100');
const votingDelay = 4n;
const votingPeriod = 16n;
const value = ethers.parseEther('1');

const params = {
decoded: [42n, 'These are my params'],
encoded: ethers.AbiCoder.defaultAbiCoder().encode(['uint256', 'string'], [42n, 'These are my params']),
Expand All @@ -30,27 +27,22 @@ describe('GovernorWithParams', function () {
const fixture = async () => {
const [owner, proposer, voter1, voter2, voter3, voter4, other] = await ethers.getSigners();
const receiver = await ethers.deployContract('CallReceiverMock');

const token = await ethers.deployContract(Token, [tokenName, tokenSymbol, version]);
const mock = await ethers.deployContract('$GovernorWithParamsMock', [name, token]);

await owner.sendTransaction({ to: mock, value });
await token.$_mint(owner, tokenSupply);

const helper = new GovernorHelper(mock, mode);
await helper.connect(owner).delegate({ token, to: voter1, value: ethers.parseEther('10') });
await helper.connect(owner).delegate({ token, to: voter2, value: ethers.parseEther('7') });
await helper.connect(owner).delegate({ token, to: voter3, value: ethers.parseEther('5') });
await helper.connect(owner).delegate({ token, to: voter4, value: ethers.parseEther('2') });

return { owner, proposer, voter1, voter2, voter3, voter4, other, receiver, token, mock, helper };
};

describe(`using ${Token}`, function () {
beforeEach(async function () {
Object.assign(this, await loadFixture(fixture));

// default proposal
// default proposal using consistent address accessor (.target for contracts)
this.proposal = this.helper.setProposal(
[
{
Expand All @@ -64,6 +56,7 @@ describe('GovernorWithParams', function () {
});

it('deployment check', async function () {
// Ensure consistent address comparison using .address where applicable
expect(await this.mock.name()).to.equal(name);
expect(await this.mock.token()).to.equal(this.token);
expect(await this.mock.votingDelay()).to.equal(votingDelay);
Expand All @@ -79,7 +72,6 @@ describe('GovernorWithParams', function () {
await this.helper.connect(this.voter4).vote({ support: VoteType.Abstain });
await this.helper.waitForDeadline();
await this.helper.execute();

expect(await this.mock.hasVoted(this.proposal.id, this.owner)).to.be.false;
expect(await this.mock.hasVoted(this.proposal.id, this.voter1)).to.be.true;
expect(await this.mock.hasVoted(this.proposal.id, this.voter2)).to.be.true;
Expand All @@ -90,9 +82,7 @@ describe('GovernorWithParams', function () {
it('Voting with params is properly supported', async function () {
await this.helper.connect(this.proposer).propose();
await this.helper.waitForSnapshot();

const weight = ethers.parseEther('7') - params.decoded[0];

await expect(
this.helper.connect(this.voter2).vote({
support: VoteType.For,
Expand All @@ -111,18 +101,21 @@ describe('GovernorWithParams', function () {
'no particular reason',
params.encoded,
);

expect(await this.mock.proposalVotes(this.proposal.id)).to.deep.equal([0n, weight, 0n]);
});

describe('voting by signature', function () {
// Helper function for signing votes (reduces duplicate code)
const signVote = async (signer, contract, message) => {
const domain = await getDomain(contract);
return signer.signTypedData(domain, { ExtendedBallot }, message);
};

it('supports EOA signatures', async function () {
await this.token.connect(this.voter2).delegate(this.other);

// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();

// Prepare vote
const weight = ethers.parseEther('7') - params.decoded[0];
const nonce = await this.mock.nonces(this.other);
Expand All @@ -133,29 +126,24 @@ describe('GovernorWithParams', function () {
nonce,
reason: 'no particular reason',
params: params.encoded,
signature: (contract, message) =>
getDomain(contract).then(domain => this.other.signTypedData(domain, { ExtendedBallot }, message)),
signature: (contract, message) => signVote(this.other, contract, message),
};

// Vote
// Vote and assert events
await expect(this.helper.vote(data))
.to.emit(this.mock, 'CountParams')
.withArgs(...params.decoded)
.to.emit(this.mock, 'VoteCastWithParams')
.withArgs(data.voter, data.proposalId, data.support, weight, data.reason, data.params);

expect(await this.mock.proposalVotes(this.proposal.id)).to.deep.equal([0n, weight, 0n]);
expect(await this.mock.nonces(this.other)).to.equal(nonce + 1n);
});

it('supports EIP-1271 signature signatures', async function () {
const wallet = await ethers.deployContract('ERC1271WalletMock', [this.other]);
await this.token.connect(this.voter2).delegate(wallet);

// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();

// Prepare vote
const weight = ethers.parseEther('7') - params.decoded[0];
const nonce = await this.mock.nonces(this.other);
Expand All @@ -166,29 +154,24 @@ describe('GovernorWithParams', function () {
nonce,
reason: 'no particular reason',
params: params.encoded,
signature: (contract, message) =>
getDomain(contract).then(domain => this.other.signTypedData(domain, { ExtendedBallot }, message)),
signature: (contract, message) => signVote(this.other, contract, message),
};

// Vote
// Vote and assert events
await expect(this.helper.vote(data))
.to.emit(this.mock, 'CountParams')
.withArgs(...params.decoded)
.to.emit(this.mock, 'VoteCastWithParams')
.withArgs(data.voter, data.proposalId, data.support, weight, data.reason, data.params);

expect(await this.mock.proposalVotes(this.proposal.id)).to.deep.equal([0n, weight, 0n]);
expect(await this.mock.nonces(wallet)).to.equal(nonce + 1n);
});

it('reverts if signature does not match signer', async function () {
await this.token.connect(this.voter2).delegate(this.other);

// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();

// Prepare vote
// Prepare vote with tampered signature
const nonce = await this.mock.nonces(this.other);
const data = {
proposalId: this.proposal.id,
Expand All @@ -197,31 +180,26 @@ describe('GovernorWithParams', function () {
nonce,
reason: 'no particular reason',
params: params.encoded,
// tampered signature
signature: (contract, message) =>
getDomain(contract)
.then(domain => this.other.signTypedData(domain, { ExtendedBallot }, message))
.then(signature => {
const tamperedSig = ethers.toBeArray(signature);
tamperedSig[42] ^= 0xff;
return ethers.hexlify(tamperedSig);
}),
signature: async (contract, message) => {
const originalSig = await signVote(this.other, contract, message);
const tamperedSigArr = ethers.toBeArray(originalSig);
// Tamper with the signature by flipping one byte
tamperedSigArr[42] ^= 0xff;
return ethers.hexlify(tamperedSigArr);
},
};

// Vote
// Expect revert due to invalid signature
await expect(this.helper.vote(data))
.to.be.revertedWithCustomError(this.mock, 'GovernorInvalidSignature')
.withArgs(data.voter);
});

it('reverts if vote nonce is incorrect', async function () {
await this.token.connect(this.voter2).delegate(this.other);

// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();

// Prepare vote
// Prepare vote with incorrect nonce
const nonce = await this.mock.nonces(this.other);
const data = {
proposalId: this.proposal.id,
Expand All @@ -230,15 +208,65 @@ describe('GovernorWithParams', function () {
nonce: nonce + 1n,
reason: 'no particular reason',
params: params.encoded,
signature: (contract, message) =>
getDomain(contract).then(domain => this.other.signTypedData(domain, { ExtendedBallot }, message)),
signature: (contract, message) => signVote(this.other, contract, message),
};

// Vote
// Expect revert due to nonce mismatch
await expect(this.helper.vote(data))
.to.be.revertedWithCustomError(this.mock, 'GovernorInvalidSignature')
.withArgs(data.voter);
});

// New test for empty params
it('reverts when voting with empty params', async function () {
await this.helper.connect(this.proposer).propose();
await this.helper.waitForSnapshot();
await expect(
this.helper.connect(this.voter2).vote({
support: VoteType.For,
reason: 'Empty params test',
params: '0x',
}),
).to.be.revertedWithCustomError(this.mock, 'GovernorInvalidParams');
});

// New test for multiple sequential signature votes
it('handles multiple sequential signature votes correctly', async function () {
await this.token.connect(this.voter2).delegate(this.other);
// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();
const weight = ethers.parseEther('7') - params.decoded[0];
const nonce1 = await this.mock.nonces(this.other);
const data1 = {
proposalId: this.proposal.id,
support: VoteType.For,
voter: this.other.address,
nonce: nonce1,
reason: 'first vote',
params: params.encoded,
signature: (contract, message) => signVote(this.other, contract, message),
};
// First vote should succeed
await expect(this.helper.vote(data1))
.to.emit(this.mock, 'CountParams')
.withArgs(...params.decoded)
.to.emit(this.mock, 'VoteCastWithParams')
.withArgs(data1.voter, data1.proposalId, data1.support, weight, data1.reason, data1.params);

// Second vote with the same nonce should revert
const data2 = {
proposalId: this.proposal.id,
support: VoteType.For,
voter: this.other.address,
nonce: nonce1, // reuse nonce intentionally
reason: 'second vote',
params: params.encoded,
signature: (contract, message) => signVote(this.other, contract, message),
};
await expect(this.helper.vote(data2))
.to.be.revertedWithCustomError(this.mock, 'GovernorInvalidSignature')
.withArgs(data2.voter);
});
});
});
}
Expand Down