test: add integration test for proposals which need ETH#80
Merged
thelostone-mc merged 5 commits intoaudit-fixesfrom Aug 28, 2025
Merged
test: add integration test for proposals which need ETH#80thelostone-mc merged 5 commits intoaudit-fixesfrom
thelostone-mc merged 5 commits intoaudit-fixesfrom
Conversation
thelostone-mc
commented
Aug 19, 2025
thelostone-mc
commented
Aug 19, 2025
thelostone-mc
force-pushed
the
timelock-eth-test
branch
from
7984d36 to
1b38f90
Compare
thelostone-mc
force-pushed
the
timelock-eth-test
branch
from
1b38f90 to
5ed5d0e
Compare
thelostone-mc
force-pushed
the
timelock-eth-test
branch
2 times, most recently
from
9ceca0c to
781a1e4
Compare
apbendi
reviewed
Aug 21, 2025
Member
apbendi
left a comment
apbendi
left a comment
There was a problem hiding this comment.
Looks good, a couple of thoughts on how we can expand these as fuzz tests
|
|
||
| /// @notice Test that a proposal requiring ETH works when we send ETH along with the execute call | ||
| /// @dev This test demonstrates the scenario where ETH is sent WITH the execute call (not FROM the timelock) | ||
| function test_ProposalRequiringEth_WorksWhenEthIsSentAlongWithExecuteCall() external { |
Member
There was a problem hiding this comment.
Can we fuz the amount required here? And fuzz the executor to show it can be any address.
Contributor
Author
There was a problem hiding this comment.
Did this but it's causing the test to run 30min +
Contributor
Author
There was a problem hiding this comment.
Ended breaking the test into 2 tests where 1 we fuzz the executor and the other we fuzz the amount
thelostone-mc
force-pushed
the
timelock-eth-test
branch
3 times, most recently
from
f6e9f03 to
972141c
Compare
thelostone-mc
force-pushed
the
audit-fixes
branch
from
fc94a53 to
911e7b3
Compare
- scenario where timelock has eth - scenario when eth is sent along with execute
thelostone-mc
force-pushed
the
timelock-eth-test
branch
from
972141c to
5be06f2
Compare
thelostone-mc
force-pushed
the
timelock-eth-test
branch
from
0a3f2b8 to
cccd113
Compare
thelostone-mc
force-pushed
the
timelock-eth-test
branch
from
cccd113 to
670e43d
Compare
apbendi
reviewed
Aug 27, 2025
Member
apbendi
left a comment
apbendi
left a comment
There was a problem hiding this comment.
A couple small comments but overall looking good
| /// @notice Internal helper function to test ETH proposal execution | ||
| /// @dev This contains the core test logic that can be reused by different fuzz tests | ||
| function _testProposalRequiringEthAndEthIsSentWithExecuteCall(uint256 _requiredEthAmount, address _executor) internal { | ||
| // 0. Verify initial state - contract should have no ETH initially |
Member
There was a problem hiding this comment.
nit: comment implies we're verifying, but we're just recording/saving the balance, not verifying them in any way.
| govHelper.submitPassAndQueue(proposer, ethProposal); | ||
|
|
||
| // 4. Execute with ETH using the specified executor | ||
| if (_executor != address(this)) { |
Member
There was a problem hiding this comment.
What is the point of this check? Why does it matter if the fuzzer chooses the test contract address?
|
Coverage after merging timelock-eth-test into audit-fixes will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||
thelostone-mc
added a commit
that referenced
this pull request
Sep 9, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To be merged after the medium severity issues are resolved