Initial tests and architecture #5

jferas · 2023-11-08T15:51:12Z

Initial tests and test architecture.

jferas · 2023-11-08T15:51:25Z

Current dependencies on/for this PR:

main
- PR forge install: openzeppelin-contracts #1
  - PR Initial code for Radworks upgraded Governor #2
    - PR Initial tests and architecture #5 👈
      - PR Added post-proposal Alpha tests and Bravo tests #8
        
        PR Updated README.md file #12
        
        PR Implement efficient settable quorum without library #15
        
        PR Pulled alpha governor tests out into a separate suite #14
        
        PR Added transfer of RAD tokens to ScopeLift upon successful upgrade to governor Bravo #19
        
        PR Pauser and renounceAdmin Drips tests #18
        
        PR Added tests for changing admin/upgrading protocol via governance #21
        
        PR Cleanup test names / scopelint fix #22
        
        PR Added GovernorVotesCompQuorumFraction for settable quorum #13

This stack of pull requests is managed by Graphite.

script/DeployInput.sol

script/Propose.s.sol

src/RadworksGovernor.sol

script/DeployInput.sol

test/Constants.sol

alexkeating · 2023-11-09T10:14:55Z

test/RadworksGovernor.t.sol

+  }
+}
+
+abstract contract Propose is ProposalTest {


This can be handled in a separate pr, but these tests seems to be testing the Alpha Governor rather than the RadworksGovernor. Maybe, reorganizing them could make this distinction clearer.

Created an issue for this: #6

test/helpers/ProposalTest.sol

test/helpers/RadworksGovernorTest.sol

alexkeating

Looking good John my only feedback are some solidity version bumps

alexkeating · 2023-11-17T11:59:31Z

script/Deploy.s.sol

-
-pragma solidity 0.8.20;
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity ^0.8.18;


It would be good to keep the minimum solidity version the same for these deploy files. So, ^0.8.20

Done 3b3a686

alexkeating · 2023-11-17T11:59:44Z

script/Propose.s.sol

@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity ^0.8.18;


version bump

alexkeating · 2023-11-17T12:00:00Z

test/Constants.sol

@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity ^0.8.18;


Version bump

Done 3b3a686

alexkeating · 2023-11-17T12:00:08Z

test/RadworksGovernor.t.sol

@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity ^0.8.18;


Version bump

Done 3b3a686

alexkeating · 2023-11-17T12:00:19Z

test/helpers/Proposal.sol

@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity ^0.8.18;


Version bump

Done 3b3a686

alexkeating · 2023-11-17T12:00:31Z

test/helpers/ProposalTest.sol

@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity ^0.8.18;


Version bump

Done 3b3a686

alexkeating · 2023-11-17T12:00:43Z

test/helpers/RadworksGovernorTest.sol

@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity ^0.8.18;


Version bump

Done 3b3a686

alexkeating · 2023-11-17T12:00:51Z

test/helpers/TestableProposeScript.sol

@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity ^0.8.17;


Version bump

Done 3b3a686

wildmolasses

hey @jferas, thank you. Got a few nits for you.

wildmolasses · 2023-11-22T19:29:25Z

script/Propose.s.sol

+import {RadworksGovernor} from "src/RadworksGovernor.sol";
+import {IGovernorAlpha} from "src/interfaces/IGovernorAlpha.sol";
+
+contract Propose is Script {


It would be nice to include some natspec on what purpose this script serves. It seems though that it'll be used to make 1 proposal specifically: the upgrade proposal. is that true? Further nitting, perhaps we should change the name to something like ProposeNewGovernor.s.sol. anyway, this shouldn't block merge, so i'll track in an issue, and from there we could ignore or PR.

Added natspec: ce8bc2a

script/Propose.s.sol

test/helpers/Proposal.sol

test/helpers/RadworksGovernorTest.sol

test/helpers/ProposalTest.sol

wildmolasses · 2023-11-24T02:17:32Z

test/helpers/ProposalTest.sol

+    vm.assume(success);
+  }
+
+  function _assumeReceiver(address _receiver) internal {


love the comments in this section. it may be more explicit to name this method _assumeValidReceiver, but current works too

This was the name used in the PoolTogether code.. keeping for consistency with that.

wildmolasses · 2023-11-24T02:20:23Z

test/helpers/ProposalTest.sol

+    assumeNoPrecompiles(_receiver);
+  }
+
+  function _randomERC20Token(uint256 _seed) internal pure returns (IERC20 _token) {


another name that i think is fine as-is, yet here i am trying to optimize. perhaps _selectERC20Token expresses more the fact we're pulling these from a list.

This was the name used in the PoolTogether code.. keeping for consistency with that.

github-actions · 2023-11-27T16:10:30Z

Coverage after merging initial-tests into main will be

12.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src
RadworksGovernor.sol	13.04%	0%	23.08%	12%	101, 106, 108, 113, 115, 126, 126, 126–127, 129, 131, 131, 131–133, 133, 133–135, 135, 135–136, 138, 152, 174, 192–193, 203, 214, 84

wildmolasses

did a review of just the changes made and they look good, so lgtm

@jferas i think it's worth reconsidering the "consistency with prior codebase" idea if we have a better name for something, but it's not merge blocking! Think this one's ready for merge.

jferas mentioned this pull request Nov 8, 2023

forge install: openzeppelin-contracts #1

Merged

jferas mentioned this pull request Nov 8, 2023

Initial code for Radworks upgraded Governor #2

Merged

jferas requested review from apbendi, wildmolasses and alexkeating November 8, 2023 16:16

alexkeating reviewed Nov 9, 2023

View reviewed changes

jferas added 10 commits November 10, 2023 14:04

Initial tests and architecture

16e875c

Formatting fix on foundry.toml

fe04983

Add MAINNET_RPC_URL env var creation to ci.yml

199ee17

Fixed config parameters and compiler version in DeployInput.sol

a5b2b15

Added ENS name of PROPOSER

58d0e4d

Removed placeholder comment about QUORUM

cba08f6

Removed proposalVotes accessor function from RadworksGovernor

5dca083

Changed storage to memory for proposalVote in _voteSucceeded

27c565c

Removed fixedpointmathlib superfluous comment

d85e504

Cleaned up treasury tokens

60195ce

jferas changed the base branch from feature/add-governor to main November 10, 2023 19:05

jferas force-pushed the initial-tests branch from a0845f0 to 60195ce Compare November 10, 2023 19:05

jferas requested a review from alexkeating November 13, 2023 14:41

This was referenced Nov 13, 2023

Added post-proposal Alpha tests and Bravo tests #8

Merged

Updated README.md file #12

Merged

Added GovernorVotesCompQuorumFraction for settable quorum #13

Closed

alexkeating reviewed Nov 17, 2023

View reviewed changes

Updated pragmas to consistent solidity version

3b3a686

jferas mentioned this pull request Nov 20, 2023

Pulled alpha governor tests out into a separate suite #14

Merged

alexkeating approved these changes Nov 21, 2023

View reviewed changes

jferas mentioned this pull request Nov 21, 2023

Implement efficient settable quorum without library #15

Closed

wildmolasses reviewed Nov 22, 2023

View reviewed changes

wildmolasses reviewed Nov 24, 2023

View reviewed changes

jferas added 4 commits November 27, 2023 10:29

Update proposal script

ce8bc2a

Fix constant name to convention

667ed97

More clearly comment when deploy script used in tests

0d8cb81

Clearly comment proposal test setup

d21c155

jferas requested a review from wildmolasses November 28, 2023 15:39

This was referenced Nov 28, 2023

Pauser and renounceAdmin Drips tests #18

Merged

Added transfer of RAD tokens to ScopeLift upon successful upgrade to governor Bravo #19

Merged

wildmolasses approved these changes Dec 7, 2023

View reviewed changes

jferas merged commit dd6c933 into main Dec 7, 2023
6 checks passed

This was referenced Dec 8, 2023

Added tests for changing admin/upgrading protocol via governance #21

Merged

Cleanup test names / scopelint fix #22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial tests and architecture #5

Initial tests and architecture #5

jferas commented Nov 8, 2023

jferas commented Nov 8, 2023 •

edited

Loading

alexkeating Nov 9, 2023

jferas Nov 9, 2023

alexkeating left a comment

alexkeating Nov 17, 2023

jferas Nov 17, 2023

alexkeating Nov 17, 2023

alexkeating Nov 17, 2023

jferas Nov 17, 2023

alexkeating Nov 17, 2023

jferas Nov 17, 2023

alexkeating Nov 17, 2023

jferas Nov 17, 2023

alexkeating Nov 17, 2023

jferas Nov 17, 2023

alexkeating Nov 17, 2023

jferas Nov 17, 2023

alexkeating Nov 17, 2023

jferas Nov 17, 2023

wildmolasses left a comment

wildmolasses Nov 22, 2023

jferas Nov 27, 2023

wildmolasses Nov 24, 2023

jferas Nov 27, 2023

wildmolasses Nov 24, 2023

jferas Nov 27, 2023

github-actions bot commented Nov 27, 2023

wildmolasses left a comment •

edited

Loading

		@@ -0,0 +1,54 @@
		// SPDX-License-Identifier: AGPL-3.0-only
		pragma solidity ^0.8.18;

		@@ -0,0 +1,22 @@
		// SPDX-License-Identifier: AGPL-3.0-only
		pragma solidity ^0.8.18;

		@@ -0,0 +1,176 @@
		// SPDX-License-Identifier: AGPL-3.0-only
		pragma solidity ^0.8.18;

		@@ -0,0 +1,26 @@
		// SPDX-License-Identifier: AGPL-3.0-only
		pragma solidity ^0.8.18;

		@@ -0,0 +1,336 @@
		// SPDX-License-Identifier: AGPL-3.0-only
		pragma solidity ^0.8.18;

		@@ -0,0 +1,70 @@
		// SPDX-License-Identifier: AGPL-3.0-only
		pragma solidity ^0.8.18;

		@@ -0,0 +1,14 @@
		// SPDX-License-Identifier: AGPL-3.0-only
		pragma solidity ^0.8.17;

Initial tests and architecture #5

Initial tests and architecture #5

Conversation

jferas commented Nov 8, 2023

jferas commented Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkeating left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildmolasses left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 27, 2023

wildmolasses left a comment • edited Loading

Choose a reason for hiding this comment

jferas commented Nov 8, 2023 •

edited

Loading

wildmolasses left a comment •

edited

Loading