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.

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

Handle Votes outside of voting period #42

Merged

Conversation

alexkeating
Copy link
Collaborator

@alexkeating alexkeating commented Sep 11, 2023

Description

  • Add tests on L2 for messages sent before the proposal is active
  • Add tests on L2 for messages sent after cast vote window
  • Add tests on L1 for messages received before a proposal is active
  • Add tests on L1 for messages received after a proposal is inactive

Related to #32

This was referenced Sep 11, 2023
@alexkeating alexkeating force-pushed the chore/add-tests-for-handling-votes-outside-testing-period branch 3 times, most recently from 7ae93fd to f2ffc4e Compare September 12, 2023 18:51
@alexkeating alexkeating force-pushed the bug/handle-unauthorized-sender branch from c265997 to df0ae18 Compare September 12, 2023 18:51
@alexkeating alexkeating force-pushed the chore/add-tests-for-handling-votes-outside-testing-period branch from f2ffc4e to 3f0cb5d Compare September 12, 2023 18:51
@alexkeating alexkeating force-pushed the bug/handle-unauthorized-sender branch from df0ae18 to 675d890 Compare September 13, 2023 20:20
@alexkeating alexkeating force-pushed the chore/add-tests-for-handling-votes-outside-testing-period branch 2 times, most recently from a7e5bfe to 3700d27 Compare September 13, 2023 20:47
@alexkeating alexkeating marked this pull request as ready for review September 13, 2023 21:24
test/mock/GovernorMetadataMock.sol Outdated Show resolved Hide resolved
test/WormholeL1VotePool.t.sol Show resolved Hide resolved
test/WormholeL1VotePool.t.sol Outdated Show resolved Hide resolved
test/WormholeL1VotePool.t.sol Show resolved Hide resolved
test/WormholeL1VotePool.t.sol Show resolved Hide resolved
test/WormholeL2VoteAggregator.t.sol Outdated Show resolved Hide resolved
test/WormholeL2VoteAggregator.t.sol Outdated Show resolved Hide resolved
test/WormholeL2VoteAggregator.t.sol Outdated Show resolved Hide resolved
test/WormholeL1VotePool.t.sol Show resolved Hide resolved
test/WormholeL1VotePool.t.sol Show resolved Hide resolved
@wildmolasses wildmolasses changed the base branch from bug/handle-unauthorized-sender to main September 18, 2023 14:52
@wildmolasses wildmolasses force-pushed the chore/add-tests-for-handling-votes-outside-testing-period branch from 7a7fe72 to 85be767 Compare September 18, 2023 14:52
l1Erc20.mint(address(this), uint96(_l2Against) + _l2InFavor + _l2Abstain);
l1Erc20.delegate(address(l1VotePool));

uint256 _proposalId = l1VotePool.createProposalVote(address(l1Erc20));
Copy link
Contributor

Choose a reason for hiding this comment

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

i love the test harness, although i sometimes get confused whether we're calling a contract method or harness method. I see _jumpToProposalEnd, a harness method, is prefixed with underscore. Should we prefix all harness methods with underscores?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have another issue to update the Harness to have an exposed prefix to make this clearer hopefully. #45

@@ -163,6 +163,32 @@ contract CastVote is L2VoteAggregatorTest {
l2VoteAggregator.castVote(1, _support);
}

function testFuzz_RevertWhen_AfterCastWindow(
uint96 _amount,
Copy link
Contributor

Choose a reason for hiding this comment

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

why are two params underscore prefix, but the others aren't?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question, I will giv them all a prefix

uint96 _amount,
uint8 _support,
uint256 proposalId,
uint64 timeToProposalEnd
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint64 timeToProposalEnd
uint64 proposalDuration

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alexkeating alexkeating force-pushed the chore/add-tests-for-handling-votes-outside-testing-period branch from 85be767 to 909b52b Compare September 18, 2023 21:03
@github-actions
Copy link

Coverage after merging chore/add-tests-for-handling-votes-outside-testing-period into main will be

94.32%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   FakeERC20.sol100%100%100%100%
   L1Block.sol25%100%25%25%23, 27, 31
   L1VotePool.sol100%100%100%100%
   L2GovernorMetadata.sol100%100%100%100%
   L2VoteAggregator.sol91.67%81.25%100%96.30%102, 105, 114, 81
   WormholeL1ERC20Bridge.sol88%66.67%100%92.86%63, 83, 88
   WormholeL1GovernorMetadataBridge.sol100%100%100%100%
   WormholeL1VotePool.sol100%100%100%100%
   WormholeL2ERC20.sol95.24%75%100%100%85
   WormholeL2GovernorMetadata.sol100%100%100%100%
   WormholeL2VoteAggregator.sol100%100%100%100%
   WormholeReceiver.sol100%100%100%100%
   WormholeSender.sol100%100%100%100%

@alexkeating alexkeating merged commit 66a5841 into main Sep 20, 2023
4 checks passed
This was referenced Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants