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

Add initial happy path integration tests #56

Merged
merged 16 commits into from
Feb 22, 2024
Merged

Conversation

alexkeating
Copy link
Collaborator

@alexkeating alexkeating commented Feb 9, 2024

Description

I added the below integration tests. These are meant to be the initial integration tests and not a comprehensive suite. More tests will be added in follow up prs.

  1. Correctly Enable Fee Amount After Proposal Is Executed
  2. Revert If Enable Fee Amount Fee Is Too High After Proposal Is Executed
  3. Revert If Enable Fee Amount Tick Spacing Is Too High After Proposal Is Executed
  4. Revert If Enable Fee Amount Tick Spacing Is Too Low After Proposal Is Executed
  5. Correctly Swap Weth And Notify Reward After Proposal Is Executed
  6. Correctly Swap Dai And Notify Reward After Proposal Is Executed
  7. Correctly Swap Dai And WETH Then Notify Reward After Proposal Is Executed
  8. Correctly Stake and Earn Rewards After Full Duration
  9. Correctly Stake And Claim Rewards After Full Duration
  10. Correctly Stake And Earn Rewards After Partial Duration
  11. Correctly Stake More And Earn Rewards After Full Duration

vm.warp(block.timestamp + _seconds);
}

function _jumpAheadByPercentOfRewardDuration(uint256 _percent) public {
Copy link
Collaborator Author

@alexkeating alexkeating Feb 12, 2024

Choose a reason for hiding this comment

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

I added a couple of methods that exist in the UniStakerRewardsTest contract. I figured we can duplicate for now and abstract them away in a followup pr.


import {Test} from "forge-std/Test.sol";

contract PercentAssertions is Test {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copied from the UniStaker.t.sol

@alexkeating alexkeating changed the title Add more integration tests Add initial happy path integration tests Feb 12, 2024
@alexkeating alexkeating force-pushed the feature/proposal-script branch from 258c2e4 to 39f7f0f Compare February 14, 2024 17:24
@alexkeating alexkeating force-pushed the chore/more-integration-tests branch 3 times, most recently from 64b5dd9 to 3f5518b Compare February 15, 2024 16:24
@alexkeating alexkeating changed the base branch from feature/proposal-script to main February 15, 2024 16:59
@alexkeating alexkeating force-pushed the chore/more-integration-tests branch 3 times, most recently from 7b0692b to fbb0a82 Compare February 15, 2024 17:09
@alexkeating alexkeating marked this pull request as ready for review February 15, 2024 18:10
@alexkeating alexkeating force-pushed the chore/more-integration-tests branch from 562e4eb to aee6bae Compare February 16, 2024 18:15
Copy link
Collaborator

@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.

Did a big fuzz run locally and got two test failures. Stack was so long it exceeded my terminal context window, but here's the two failure cases and a bit of one stack:

Failing tests:
Encountered 2 failing tests in test/UniStaker.integration.t.sol:Stake
[FAIL. Reason: revert: Uni::_moveVotes: vote amount underflows; counterexample: calldata=0x5260afce00000000000000000000000076f54eeb0d33a2a2c5ccb72fe12542a56f35d67c08c379a0000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000c3c00000000000000000000000000000000ffffffffffffffffffffffffffffffff args=[0x76f54Eeb0D33a2A2c5CCb72FE12542A56f35d67C, 3963877391197344453575983046348115674221700746820753546331534351508065746945 [3.963e75], 0x0000000000000000000000000000000000000c3c, 340282366920938463463374607431768211455 [3.402e38]]] testForkFuzz_CorrectlyStakeAndClaimRewardsAfterFullDuration(address,uint256,address,uint128) (runs: 1548, μ: 4977153, ~: 5003766)
[FAIL. Reason: revert: Uni::_moveVotes: vote amount underflows; counterexample: calldata=0xae6a158600000000000000000000000076f54eeb0d33a2a2c5ccb72fe12542a56f35d67c000000000000000000000044656c61793a2044656c6179206d757374206578630000000000000000000000000000000000000000000000000000000000005fcb0000000000000000000000000000000000000000000000000000000000000594000000000000000000000000000000000000000000000000000000000000022000000000000000000000000000000000000000000000000000000000000033a0 args=[0x76f54Eeb0D33a2A2c5CCb72FE12542A56f35d67C, 99961136377979212079769318731761422811699695941731 [9.996e49], 24523 [2.452e4], 0x0000000000000000000000000000000000000594, 544, 13216 [1.321e4]]] testForkFuzz_CorrectlyStakeMoreAndEarnRewardsAfterFullDuration(address,uint256,uint256,address,uint128,uint256) (runs: 4355, μ: 4987272, ~: 5016334)
    ├─ [25230] 0x1f9840a85d5aF5bf1D1762F925BDADdC4201F984::approve(UniStaker: [0x1B0Fd9Df9c444A4CeEC9863B88e1D7Cb3db621c0], 9318731761422811699695941731 [9.318e27])
    │   ├─ emit Approval(owner: 0x76f54Eeb0D33a2A2c5CCb72FE12542A56f35d67C, spender: UniStaker: [0x1B0Fd9Df9c444A4CeEC9863B88e1D7Cb3db621c0], value: 9318731761422811699695941731 [9.318e27])
    │   └─ ← true
    ├─ [0] console::log("Bound Result", 86) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::prank(0x76f54Eeb0D33a2A2c5CCb72FE12542A56f35d67C)
    │   └─ ← ()
    ├─ [336583] UniStaker::stake(9318731761422811699695941731 [9.318e27], 0x0000000000000000000000000000000000000594)
    │   ├─ [65878] → new DelegationSurrogate@0xa1fCA0e92c8460DCB1e13670194d89CeD91C639B
    │   │   ├─ [26990] 0x1f9840a85d5aF5bf1D1762F925BDADdC4201F984::delegate(0x0000000000000000000000000000000000000594)
    │   │   │   ├─ emit DelegateChanged(delegator: DelegationSurrogate: [0xa1fCA0e92c8460DCB1e13670194d89CeD91C639B], fromDelegate: 0x0000000000000000000000000000000000000000, toDelegate: 0x0000000000000000000000000000000000000594)
    │   │   │   └─ ← ()
    │   │   ├─ [25103] 0x1f9840a85d5aF5bf1D1762F925BDADdC4201F984::approve(UniStaker: [0x1B0Fd9Df9c444A4CeEC9863B88e1D7Cb3db621c0], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
    │   │   │   ├─ emit Approval(owner: DelegationSurrogate: [0xa1fCA0e92c8460DCB1e13670194d89CeD91C639B], spender: UniStaker: [0x1B0Fd9Df9c444A4CeEC9863B88e1D7Cb3db621c0], value: 79228162514264337593543950335 [7.922e28])
    │   │   │   └─ ← true
    │   │   └─ ← 524 bytes of code
    │   ├─ emit SurrogateDeployed(delegatee: 0x0000000000000000000000000000000000000594, surrogate: DelegationSurrogate: [0xa1fCA0e92c8460DCB1e13670194d89CeD91C639B])
    │   ├─ [30823] 0x1f9840a85d5aF5bf1D1762F925BDADdC4201F984::transferFrom(0x76f54Eeb0D33a2A2c5CCb72FE12542A56f35d67C, DelegationSurrogate: [0xa1fCA0e92c8460DCB1e13670194d89CeD91C639B], 9318731761422811699695941731 [9.318e27])
    │   │   ├─ emit Approval(owner: 0x76f54Eeb0D33a2A2c5CCb72FE12542A56f35d67C, spender: UniStaker: [0x1B0Fd9Df9c444A4CeEC9863B88e1D7Cb3db621c0], value: 0)
    │   │   ├─ emit Transfer(from: 0x76f54Eeb0D33a2A2c5CCb72FE12542A56f35d67C, to: DelegationSurrogate: [0xa1fCA0e92c8460DCB1e13670194d89CeD91C639B], value: 9318731761422811699695941731 [9.318e27])
    │   │   └─ ← revert: Uni::_moveVotes: vote amount underflows
    │   └─ ← revert: Uni::_moveVotes: vote amount underflows
    └─ ← revert: Uni::_moveVotes: vote amount underflows

test/helpers/IntegrationTest.sol Outdated Show resolved Hide resolved
test/helpers/IntegrationTest.sol Outdated Show resolved Hide resolved
test/helpers/IntegrationTest.sol Outdated Show resolved Hide resolved
test/helpers/IntegrationTest.sol Outdated Show resolved Hide resolved
test/UniStaker.integration.t.sol Show resolved Hide resolved
test/UniStaker.integration.t.sol Outdated Show resolved Hide resolved
@alexkeating alexkeating requested a review from apbendi February 20, 2024 20:23
Copy link
Collaborator

@wildmolasses wildmolasses left a comment

Choose a reason for hiding this comment

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

lgtm, 2 comments

.github/workflows/ci.yml Outdated Show resolved Hide resolved
foundry.toml Outdated
@@ -1,8 +1,13 @@
[profile.default]
evm_version = "paris"
fuzz = { seed = "1" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

this implies a static fuzz seed will be used in local dev, right? I don't think that's what we want, or do i have the wrong idea?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes exactly, typically we add this locally as well when adding it to the CI. It all depends on what we want the default to be when running tests locally. I am happy to remove if we would rather not have it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer no default seed

- name: Generate fuzz seed with 1 week TTL
run: >
echo "FOUNDRY_FUZZ_SEED=$(
echo $(($EPOCHSECONDS - $EPOCHSECONDS % 604800))
Copy link
Collaborator

Choose a reason for hiding this comment

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

i lean towards a 1 day TTL instead, so that different fuzz runs can get us feedback quicker. but here, it's a big nit, since we're soon to be code complete

@wildmolasses wildmolasses self-requested a review February 22, 2024 16:31
Copy link

Coverage after merging chore/more-integration-tests into main will be

100.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   UniStaker.sol99.49%95.83%100%100%611
   V3FactoryOwner.sol100%100%100%100%

@apbendi apbendi merged commit 1d13ba3 into main Feb 22, 2024
4 checks passed
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