Added post-proposal Alpha tests and Bravo tests #8
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
test/RadworksGovernor.t.sol
Outdated
| @@ -1,8 +1,11 @@ | |||
| // SPDX-License-Identifier: AGPL-3.0-only | |||
| pragma solidity ^0.8.18; | |||
|
|
|||
| import {ERC20VotesComp} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20VotesComp.sol"; | |||
| import {console2} from "forge-std/console2.sol"; | |||
test/RadworksGovernor.t.sol
Outdated
| assertEq(TIMELOCK.balance, _timelockETHBalance - _amount); | ||
| } | ||
|
|
||
| // TODO: This test commented out as the timelock has no ETH as of 2023-11-08 |
There was a problem hiding this comment.
It would be good to create an issue and link it here.
There was a problem hiding this comment.
New issue created.. #16
There was a problem hiding this comment.
related a bit to this TODO: the question had come up for me earlier, why not deal / vm.deal some ETH / tokens to addresses in relevant tests? not saying we should do it, just curious if there's a reason not to
There was a problem hiding this comment.
I don't think there is a reason not to
There was a problem hiding this comment.
As it turns out we already had a test like this with "vm.deal".. this was a test with "nodeal" in the name, removed it.. 1832b53
foundry.toml
Outdated
| @@ -10,7 +10,7 @@ | |||
| verbosity = 3 | |||
|
|
|||
| [profile.ci] | |||
| fuzz = { runs = 5000 } | |||
| fuzz = { runs = 50 } | |||
There was a problem hiding this comment.
One thing we did in the PoolTogether upgrade to help deal with all of these tests was to also use a fuzz seed.
There was a problem hiding this comment.
It would involve adding to the foundry.toml and updating the c
fuzz = { seed = 0x1 }
There was a problem hiding this comment.
And also update the CI. There is an example in the PoolTogether upgrade
|
|
||
| // Counter-intuitively, the Governor (not the Timelock) must hold the ETH. | ||
| // See the deployed GovernorAlpha, line 281: | ||
| // https://etherscan.io/address/0x31c8EAcBFFdD875c74b94b077895Bd78CF1E64A3#code |
There was a problem hiding this comment.
I think this address might be the Radicle token not the Radicle governor
There was a problem hiding this comment.
Will resolve in the PR where alpha tests are separated out to avoid rebasing multiple files
There was a problem hiding this comment.
Was done in this commit in PR 14 .. 8f5e79e
test/RadworksGovernor.t.sol
Outdated
| assertEq(_token.balanceOf(_receiver), _receiverTokenBalance, "Receiver balance is incorrect"); | ||
| } | ||
|
|
||
| //// |
There was a problem hiding this comment.
nit: these section headings are formatted differently from one another. not sure if that's intentional.
There was a problem hiding this comment.
Not intentional, done: 1eb42ac
test/RadworksGovernor.t.sol
Outdated
| assertEq(TIMELOCK.balance, _timelockETHBalance - _amount); | ||
| } | ||
|
|
||
| // TODO: This test commented out as the timelock has no ETH as of 2023-11-08 |
There was a problem hiding this comment.
related a bit to this TODO: the question had come up for me earlier, why not deal / vm.deal some ETH / tokens to addresses in relevant tests? not saying we should do it, just curious if there's a reason not to
| _jumpToActiveProposal(_newProposalId); | ||
|
|
||
| // Delegates vote with a mix of For/Against/Abstain with For winning. | ||
| // TODO: Need more delegates for this.. pool together had more! |
There was a problem hiding this comment.
just curious, can you elaborate on why we need more delegates?
There was a problem hiding this comment.
We should be able to pull all of the "need more delegates" in a future PR, as we've been able to test with the delegates we have.
There was a problem hiding this comment.
i'm confused why we are adding these comments if we don't need them! i won't block merge on it though.
Added tests for Alpha governor after upgrade proposal and proposal actions on Bravo governor.