Governance integration tests #330

baroooo · 2023-12-18T13:14:17Z

Description

Integration tests that cover functionality around new governance and token.

This branch is based on the governance factory branch. Once that is merged all the related changes should be contained in /test/governance/IntegrationTests

Other changes

Utils.sol and Proposals.sol that is used by the tests
Removed the old comment on Airgrab.sol about the ownership of the contract.

Tested

Yes

Related issues

Fixes Governance integration tests #275

…rnings or errors

…simpler testing all other related state vars are public so this seems to have been a minor oversight rather than intentional making it private

integration tests will be added in another branch

…ntracts

…ing SARIF report this should not change the fact that CI fails when slither complains. because the SARIF report should contain the same errors which then would make CI fail. But we'd keep the record of issues via SARIF files which currently is not the case.

…imit issue

… local var

replaced handmade copy/pasta interfaces with the real code from gnosis

…it tests

…tract

…ention

openzeppelin-code · 2023-12-18T13:34:31Z

Governance integration tests

Generated at commit: 8fe3e73a0923ceb5b93c7867f80043753e486d54

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 1 0 16 38 56
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

chapati23

didn't manage to get through everything yet, will continue tomorrow but here's a few small comments already

test/governance/IntegrationTests/Integration.t.sol

chapati23

Very solid work!

Didn't find anything wrong, just some additional test ideas I'd like to align on if we want to add them or not

test/governance/IntegrationTests/Integration.t.sol

test/governance/IntegrationTests/Proposals.sol

test/governance/IntegrationTests/Utils.sol

test/governance/IntegrationTests/Integration.t.sol

chapati23 · 2023-12-19T11:00:56Z

test/governance/IntegrationTests/Integration.t.sol

Governance Test Plan has the following additional tests that we don't have integrations tests for yet:

MentoGovernor - Defeat a proposal (majority votes NO)

MentoToken - Send and burn tokens

Emission - Correctness of calculation of emitted amounts (it's tested a little bit in test_watchdog_cancel_shouldCancelQueuedProposal as a drive by but we could add a more explicit test for it)

Airgrab - Reclaiming or claiming higher amount than allocated reverts

Airgrab - Claiming with an invalid proof reverts (you have a invalidClaimer var in Integration.sol but it appears unused

Airgrab - Claiming with a random account reverts

Airgrab - Draining after airgrab end should send remaining tokens to the Celo Community Fund (and maybe also show that draining before end of airgrab is not possible?)

Governance Timelock - Change roles on timelock (proposer / executor / canceller)

Mento Labs Treasury - Transfer ownership and verify new ownership

Emission & MentoToken - Check that MentoToken.emittedAmount lines up with totalEmittedAmount

Airgrab - Claiming with a delegate delegates the voting power

I'm not saying we should write integration tests for all of these scenarios but as a basis for discussion which ones to add and which ones deliberately not to add.

After a quick discusison we agreed most of the scenarios are covered by unit tests already, but the following should be evaluated more closely for additional tests:

Governance Timelock - Change roles on timelock (proposer / executor / canceller)

Mento Labs Treasury - Transfer ownership and verify new ownership

Emission & MentoToken - Check that MentoToken.emittedAmount lines up with totalEmittedAmount

Airgrab - Claiming with a delegate delegates the voting power

chapati23

Approving this already given I'll be off until February. We aligned on a call on open discussion points and I have full faith in @baroooo to bring it over the finish line.

bowd

🚂 cho-chooo! Here come the integration tests! Great work!
I left some minor comments and a proposal for some quick refactoring around Utils and Proposal that I sketched here: https://github.com/mento-protocol/mento-core/pull/332/files - I'm not married to these but if you like it you can merge 'em.

bowd · 2023-12-18T14:17:35Z

test/governance/IntegrationTests/Proposals.sol

+    targets = new address[](1);
+    targets[0] = address(emission);


We have this Arrays library that lets you do Arrays.addresses(emission) here. It has overloaded methods for up to 6 items (IIRC) and for multiple types (uints, bytes, etc).

bowd · 2023-12-18T14:20:14Z

test/governance/IntegrationTests/Proposals.sol

+import { Test } from "forge-std-next/Test.sol";
+
+contract Proposals is Test {


nit: If I didn't miss anything, this contract can be a Library, and it doesn't need to inherit from Test. I think I'd prefer that more.

actually tried to sketch some changes in this proposal: https://github.com/mento-protocol/mento-core/pull/332/files

bowd · 2023-12-19T16:11:34Z

test/governance/IntegrationTests/Utils.sol

+import { ECDSA } from "openzeppelin-contracts-next/contracts/utils/cryptography/ECDSA.sol";
+import { Strings } from "openzeppelin-contracts-next/contracts/utils/Strings.sol";
+
+contract Utils is Test {


I propose we turn this into a VmExtensions library as this seams to be useful in multiple places.
Here's a sketch of what I mean: https://github.com/mento-protocol/mento-core/pull/332/files

looks great, yeah lets merge this

bowd · 2023-12-19T16:19:23Z

test/governance/IntegrationTests/GovernanceIntegration.t.sol

+    //  create a transaction to transfer tokens from the multisig to the governanceTimelockAddress
+    bytes memory transferCallData = abi.encodeWithSelector(
+      mentoToken.transfer.selector,
+      governanceTimelockAddress,
+      10_000_000e18
+    );


This transaction transfers tokens from the MentoLabsTreasury to the GovernanceTimelock (or MentoProtocolTreasury).
I would fix the comment for sure, but maybe change this so that we transfer tokens from MentoLabsTreasury to the MentoLabsMultisig. It's not a big deal, but that's the actual flow we would do, and I think it's less confusing then transferring to the protocol.

bowd · 2023-12-19T16:20:25Z

test/governance/IntegrationTests/GovernanceIntegration.t.sol

+      0,
+      proposerRoleCallData,
+      0,
+      keccak256(bytes("Transfer tokens to governanceTimelockAddress")),


nit: "Grant proposal role to alice"

PR Changes proposal

bowd and others added 30 commits November 8, 2023 09:55

feat: refactor Factory

21ccd3b

feat: governance factory fork test working checkpoint

bb31aef

feat: fix lint

b13a3ed

feat: fix solhint

7bbba36

chore: make prettier output less spammy by only logging files with wa…

3cfe8f3

…rnings or errors

refactor: extracted gnosis safe interfaces into their own files

c4f82d9

chore: minor formatting and typo fixes

28af90b

feat(GovernanceFactory): made mentolabsTreasury state var public for …

00e6094

…simpler testing all other related state vars are public so this seems to have been a minor oversight rather than intentional making it private

chore(LockingVotes): fix linter warnings on unimplemented overrides

86bc46b

test: added mock for gnosis safe proxy factory

33c567d

chore: added .dcignore to .gitignore

60b044f

test(GovernanceFactory): fixed and expanded tests

0600206

chore: prettier formatting fixes

86d1212

chore: solhint fix

0058c5b

docs(GovernanceFactory): added more natspec comments

b9c7863

chore(GovernanceFactory): fixed linter warning in fork test

07a312e

chore(GovernanceFactory): focus on unit tests of util functions only

5124d46

integration tests will be added in another branch

chore(GovernanceFactory): add upgradeability tests

352fa5b

chore: fix solhint warning

d933dd7

chore: fix typo in comment

834b836

chore: updated slither.db.json to ignore warnings for openzeppelin co…

e5e7dc8

…ntracts

chore(CI): reverted continue-on-error change due to GitHub API Rate L…

2075c83

…imit issue

refactor(GovernanceFactory): extracted inline adressForNonce(10) into…

5a53cc9

… local var

refactor(GovernanceFactory): remove redundant casting

ed0c9b1

refactor(GovernanceFactory): added safe-contracts dependency

c3d8324

replaced handmade copy/pasta interfaces with the real code from gnosis

refactor(GovernanceFactory): renamed fork test to be excluded from un…

8b7032d

…it tests

chore(GovernanceFactory): add more constructor tests for Emission con…

b2c7df5

…tract

chore(GovernanceFactory): renamed unit tests according to naming conv…

e78ee27

…ention

chore(GovernanceFactory): added additional asserts to unit tests

2220f06

baroooo added 9 commits December 15, 2023 12:14

feat: add remappings and tests

f8d1b0f

fix: naming issues

87a14ce

chore: ignore line-length

50b9075

test: veto multisig operation

8a1dded

refactor: integration tests

254ab27

test: comments for integration tests

ac2f28a

chore: slither and solhint

52f57cd

chore: remove loggers

e7a9ccd

Merge branch 'feat/tokenWork' into feat/factory-integration-tests

b964126

fix: rebase errors

712b4fd

baroooo changed the title ~~Feat/factory integration tests~~ Governance integration tests Dec 18, 2023

fix: naming issue

373691d

baroooo marked this pull request as ready for review December 18, 2023 13:41

baroooo requested review from bowd and chapati23 December 18, 2023 13:42

fix: airgrab comment

bed0116

chapati23 reviewed Dec 18, 2023

View reviewed changes

chapati23 reviewed Dec 19, 2023

View reviewed changes

chapati23 approved these changes Dec 19, 2023

View reviewed changes

chapati23 and others added 4 commits December 19, 2023 13:14

chore(GovernanceFactory): fix comment typo

8699b42

fix: review comments

7741f32

fix: remove notice tag from Utils

9d9e3de

test: roles, totalEmittedAmount, claim with a delegate

a39c8f8

bowd approved these changes Dec 19, 2023

View reviewed changes

bowd and others added 3 commits December 19, 2023 18:06

refactor: make Proposal a library, and turn Utils in VmExtension (#332)

79b5fcf

PR Changes proposal

fix: transfer direction on multisig test

9e9b8b6

feat: use and extend arrays lib

8fe3e73

baroooo merged commit 61eb278 into feat/tokenWork Dec 19, 2023
3 of 15 checks passed

baroooo deleted the feat/factory-integration-tests branch December 19, 2023 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Governance integration tests #330

Governance integration tests #330

baroooo commented Dec 18, 2023 •

edited

Loading

openzeppelin-code bot commented Dec 18, 2023 •

edited

Loading

chapati23 left a comment

chapati23 left a comment

chapati23 Dec 19, 2023 •

edited

Loading

chapati23 Dec 19, 2023 •

edited by baroooo

Loading

chapati23 left a comment

bowd left a comment

bowd Dec 18, 2023

bowd Dec 18, 2023

bowd Dec 19, 2023

bowd Dec 19, 2023

baroooo Dec 19, 2023

bowd Dec 19, 2023

bowd Dec 19, 2023

		import { Test } from "forge-std-next/Test.sol";

		contract Proposals is Test {

Governance integration tests #330

Governance integration tests #330

Conversation

baroooo commented Dec 18, 2023 • edited Loading

Description

Other changes

Tested

Related issues

openzeppelin-code bot commented Dec 18, 2023 • edited Loading

Governance integration tests

🚨 Report Summary

chapati23 left a comment

Choose a reason for hiding this comment

chapati23 left a comment

Choose a reason for hiding this comment

chapati23 Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

chapati23 Dec 19, 2023 • edited by baroooo Loading

Choose a reason for hiding this comment

chapati23 left a comment

Choose a reason for hiding this comment

bowd left a comment

Choose a reason for hiding this comment

bowd Dec 18, 2023

Choose a reason for hiding this comment

bowd Dec 18, 2023

Choose a reason for hiding this comment

bowd Dec 19, 2023

Choose a reason for hiding this comment

bowd Dec 19, 2023

Choose a reason for hiding this comment

baroooo Dec 19, 2023

Choose a reason for hiding this comment

bowd Dec 19, 2023

Choose a reason for hiding this comment

bowd Dec 19, 2023

Choose a reason for hiding this comment

baroooo commented Dec 18, 2023 •

edited

Loading

openzeppelin-code bot commented Dec 18, 2023 •

edited

Loading

chapati23 Dec 19, 2023 •

edited

Loading

chapati23 Dec 19, 2023 •

edited by baroooo

Loading