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.

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

Jump to bottom

Improve FlexVotingClient test structure #71

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Improve FlexVotingClient test structure #71

wants to merge 11 commits into from
+415 −105

Conversation

davidlaprade
Copy link
Contributor

@davidlaprade davidlaprade commented Nov 13, 2024 •
edited
Loading

Fixes #69

This PR restructures the FlexClient tests to accord with scopelint spec. Some tests were renamed. Other tests were added. The resulting spec is:

Contract Specification: FlexVotingClient
├── constructor
│   ├──  Sets Governor
│   └──  Self Delegates
├── _rawBalanceOf
│   ├──  Returns Zero For Non Depositors
│   ├──  Increases On Deposit
│   ├──  Decreases On Withdrawal
│   └──  Unaffected By Borrow
├── _castVoteReasonString
│   └──  Returns Descriptive String
├── _selfDelegate
│   └──  Sets Client As The Delegate
├── expressVote
│   ├──  Increments Internal Accouting
│   ├──  Revert When: Depositing After Proposal
│   ├──  Revert When: No Client Weight But Token Weight
│   ├──  Revert On: Double Votes
│   └──  Revert On: Unknown Vote Type
├── castVote
│   ├──  Submits Votes To Governor
│   ├──  Weight Is Snapshot Dependent
│   ├──  Tracks Multiple Users Votes
│   ├──  Scales Vote Weight Based On Pool Balance
│   ├──  Abandons Unexpressed Voting Weight
│   ├──  Voting Weight Is Unaffected By Deposits After Proposal
│   ├──  Can Call Multiple Times For The Same Proposal
│   ├──  Revert When: No Votes To Cast
│   └──  Revert When: After Voting Period
├── _checkpointRawBalanceOf
│   └──  Stores The Raw Balance With The Block Number
├── getPastRawBalance
│   ├──  Returns Zero For Users Without Deposits
│   ├──  Returns Current Value For Future Blocks
│   └──  Returns User Balance At A Given Block
└── getPastTotalBalance
    ├──  Returns Zero Without Deposits
    ├──  Returns Current Value For Future Blocks
    ├──  Sums All User Deposits
    └──  Returns Total Deposits At A Given Block

davidlaprade
davidlaprade commented Nov 14, 2024
View reviewed changes
test/FlexVotingClient.t.sol
@@ -196,8 +440,12 @@ contract Deposit is FlexVotingClientTest {
}
}

contract Vote is FlexVotingClientTest {
function testFuzz_UserCanCastVotes(address _user, uint208 _voteWeight, uint8 _supportType) public {
contract ExpressVote is FlexVotingClientTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably worth adding a test that tries to express votes on a bogus proposalId

apbendi
apbendi reviewed Nov 15, 2024
View reviewed changes
Copy link
Member

@apbendi apbendi left a comment

Choose a reason for hiding this comment

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

Looking really great @davidlaprade, huge improvement! Left some thoughts, but it's mostly marginal stuff.

test/FlexVotingClient.t.sol Outdated

// Contract name has a leading underscore for scopelint spec support.
contract _RawBalanceOf is FlexVotingClientTest {
function _commonUserAssumptions(address _user) internal view {
Copy link
Member

Choose a reason for hiding this comment

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

nit: in other projects we tend to use a naming convention that states this actively, i.e. like _assumeSafeUser in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that better, thanks. Done in 4fb9abc

test/FlexVotingClient.t.sol
uint208 _amount
) public {
vm.assume(_depositor != address(flexClient));
vm.assume(_nonDepositor != address(flexClient));
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised we're not getting test failures here, because I think we would need an assumption that _depositor != _nonDepositor. That makes me wonder if something is not right with the test, and it's passing for the wrong reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch. You were right. Fixed in 34e4c2c

test/FlexVotingClient.t.sol Outdated
uint48 _blockNum
) public {
vm.assume(_user != address(flexClient));
vm.assume(_blockNum > 2);
Copy link
Member

Choose a reason for hiding this comment

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

For numeric values, we should use bound rather than assume, for performance reasons. Also, it might be better to use block.number here instead of assuming the current block number is less than 2. We could end up changing setUp to roll the block number, for some reason, or the tests could end up running in a different context in the future (such as a fork test from a network).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh 🤦. Done 4d6d8b2

test/FlexVotingClient.t.sol
flexClient.exposed_setDeposits(_user, _amount);
assertEq(flexClient.getPastRawBalance(_user, _blockNum), 0);

vm.roll(_blockNum);
Copy link
Member

Choose a reason for hiding this comment

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

It would be best to do vm.roll(block.number + _blockNum) here, or alternatively, we can bound the _blockNum value to something above block.number in the first place, as suggested above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken care of in 4d6d8b2

test/FlexVotingClient.t.sol Outdated
function testFuzz_SubmitsVotesToGovernor(address _user, uint208 _voteWeight, uint8 _supportType)
public
{
_voteWeight = _commonFuzzerAssumptions(_user, _voteWeight, _supportType);
Copy link
Member

Choose a reason for hiding this comment

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

We should refactor this test helper for a couple reasons, but I will pull that into its own issue since it's not part of what this PR is changing.

@github-actions GitHub Actions
Copy link

Coverage after merging improve-flex-client-test-structure-#69 into master will be

92.54%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   FlexVotingClient.sol100%100%100%100%
   FractionalPool.sol80.65%70.59%73.33%85.25%107, 109, 111, 114, 191, 194–195, 198, 202, 239, 80, 82–83, 85
   GovernorCountingFractional.sol93.75%76.92%100%97.56%184, 186, 188, 191

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apbendi apbendi apbendi left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve FlexVotingClient test structure
2 participants
@davidlaprade @apbendi