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

changePrank fails with No prank in progress to stop #4835

Closed
2 tasks done
grandizzy opened this issue Apr 27, 2023 · 8 comments
Closed
2 tasks done

changePrank fails with No prank in progress to stop #4835

grandizzy opened this issue Apr 27, 2023 · 8 comments
Labels
T-bug Type: bug

Comments

@grandizzy
Copy link
Collaborator

grandizzy commented Apr 27, 2023

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (f05fdd8 2023-04-27T00:04:10.201981651Z)

What command(s) is the bug in?

No response

Operating System

Linux

Describe the bug

changePrank with no prank started used to work and was properly changing prank before enforcing it in #4827 (8973b2b)
For backwards compatibility reasons, can we have the changePrank exempted from this new check? (test works OK if foundryup -v nightly-dedc31eed0683764ae542b9b575c21f6bd2a5c60)

pragma solidity 0.8.14;

import '@std/Test.sol';
import '@std/Vm.sol';

contract SampleTest is Test {

    address bob;
    address alice;

    function setUp() external {
        bob = makeAddr("bob");
        alice = makeAddr("alice");

        vm.prank(bob);
    }

    function test_sample() external {
        changePrank(alice);
    }

}
Running 1 test for tests/forge/SampleTest.t.sol:SampleTest
[FAIL. Reason: No prank in progress to stop] test_sample() (gas: 5191)
Test result: FAILED. 0 passed; 1 failed; finished in 613.35µs
@grandizzy grandizzy added the T-bug Type: bug label Apr 27, 2023
@gakonst gakonst added this to Foundry Apr 27, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Apr 27, 2023
grandizzy added a commit to ajna-finance/ajna-core that referenced this issue Apr 27, 2023
@PaulRBerg
Copy link
Contributor

changePrank with no prank started used to work

This should never have been the case. See #4779. Are you absolutely sure that you didn't have a startPrank somewhere in your code base?

Cc @mds1.

@grandizzy
Copy link
Collaborator Author

changePrank with no prank started used to work

This should never have been the case. See #4779. Are you absolutely sure that you didn't have a startPrank somewhere in your code base?

Cc @mds1.

Oh, could be a plain vm.prank actually, going to double check

ith-harvey pushed a commit to ajna-finance/ajna-core that referenced this issue Apr 27, 2023
* changes needed for contract verification (#764)

* changes needed for contract verification

* gotcha

* Make NFT tokens claimable after `take`, `bucketTake`, `settle` actions (#763)

* Add test failure

* When ERC721.bucketTake always rebalance token ids from borrower to pool claimable array

* Consistent take with bucketTake, always rebalance tokens

* Check invariants in regression test

* Always rebalance tokens on settle (settle could enter with 0 collateral but still with bad debt)

* Changed epsilon from 1e16 to 1e17 in F1 and F2 invariants (#765)

* Add variable token precision for ERC721Pool invariants (#755)

* Add multiple precision of quote token in erc721Pool invariants

* Add shebang and set -e option

* Invariant framework improvements (#769)

* Update forge library, set default env vars using envOr instead reading from env file

* Remove InvariantsTestHelpers contract and move invariants helper functions to BaseInvariants

* env cleanup for invariants

* Update invariant testing scripts

* Add skipTime in merge collateral handler

* Add fuzzed time skip between kick and take/bucketTake

---------

Co-authored-by: grandizzy <[email protected]>

* Removing brownie TODOs as these tests won't be further expanded (#767)

* ERC20 R1_R2_R3_R4_R5_R6_R7_R8 invariant failure (#771)

* Add failing test

* Change cutoff for deminimus check to 1 LP in a bucket for exchange rate diffs

---------

Co-authored-by: mwc <[email protected]>

* Erc721 CT2 invariant failure (#770)

* Reproduce CT2 invariant failure

* Add collateral that could be compensated in bucket 7388 when auction settled

* Make sure MAX bucket not accounted twice. Fix test-regression target

* Fix bucketTake invariant logic in place instead in CT2 invariant. Added regression test comments.

* Fix CI, lock foundry version until resolution for foundry-rs/foundry#4835

---------

Co-authored-by: Ed Noepel <[email protected]>
Co-authored-by: Prateek Gupta <[email protected]>
Co-authored-by: mwc <[email protected]>
@grandizzy
Copy link
Collaborator Author

changePrank with no prank started used to work

This should never have been the case. See #4779. Are you absolutely sure that you didn't have a startPrank somewhere in your code base?
Cc @mds1.

Oh, could be a plain vm.prank actually, going to double check

@PaulRBerg yeah, you're right, I updated test contract sample in PR description, thanks for flagging

@mds1
Copy link
Collaborator

mds1 commented Apr 27, 2023

I think we should keep the behavior from #4827, as if you have a stopPrank without a startPrank semantically that doesn't make sense and maybe you have a bug. Then we'd implement #4826 (comment), which removes the need for changePrank and would fix the UX

@grandizzy
Copy link
Collaborator Author

I think we should keep the behavior from #4827, as if you have a stopPrank without a startPrank semantically that doesn't make sense and maybe you have a bug. Then we'd implement #4826 (comment), which removes the need for changePrank and would fix the UX

Thank you, I think this makes sense. Do you have any timeline for #4826 , not sure if I should start fixing tests (pretty substantial :) ) at this point and then update them again when #4826 gets in. Or if I should just lock version that works here atm and update test only once when #4826 ready.

@mds1
Copy link
Collaborator

mds1 commented Apr 28, 2023

@4meta5 is working on that PR so deferring to them on timeline :)

ith-harvey added a commit to ajna-finance/ajna-core that referenced this issue Jun 7, 2023
* initial draft

* got initial scaffolding running

* included Matts invaraints

* added PM1 and PM5

* Reward invariants merged (#773)

* changes needed for contract verification (#764)

* changes needed for contract verification

* gotcha

* Make NFT tokens claimable after `take`, `bucketTake`, `settle` actions (#763)

* Add test failure

* When ERC721.bucketTake always rebalance token ids from borrower to pool claimable array

* Consistent take with bucketTake, always rebalance tokens

* Check invariants in regression test

* Always rebalance tokens on settle (settle could enter with 0 collateral but still with bad debt)

* Changed epsilon from 1e16 to 1e17 in F1 and F2 invariants (#765)

* Add variable token precision for ERC721Pool invariants (#755)

* Add multiple precision of quote token in erc721Pool invariants

* Add shebang and set -e option

* Invariant framework improvements (#769)

* Update forge library, set default env vars using envOr instead reading from env file

* Remove InvariantsTestHelpers contract and move invariants helper functions to BaseInvariants

* env cleanup for invariants

* Update invariant testing scripts

* Add skipTime in merge collateral handler

* Add fuzzed time skip between kick and take/bucketTake

---------

Co-authored-by: grandizzy <[email protected]>

* Removing brownie TODOs as these tests won't be further expanded (#767)

* ERC20 R1_R2_R3_R4_R5_R6_R7_R8 invariant failure (#771)

* Add failing test

* Change cutoff for deminimus check to 1 LP in a bucket for exchange rate diffs

---------

Co-authored-by: mwc <[email protected]>

* Erc721 CT2 invariant failure (#770)

* Reproduce CT2 invariant failure

* Add collateral that could be compensated in bucket 7388 when auction settled

* Make sure MAX bucket not accounted twice. Fix test-regression target

* Fix bucketTake invariant logic in place instead in CT2 invariant. Added regression test comments.

* Fix CI, lock foundry version until resolution for foundry-rs/foundry#4835

---------

Co-authored-by: Ed Noepel <[email protected]>
Co-authored-by: Prateek Gupta <[email protected]>
Co-authored-by: mwc <[email protected]>

* working through PM3

* wrapped position calls

* made getPoolAccumulators a free function & cleaned up logging

* included free function

* increased logging robustness

* working through adding positions without passing positions to baseclass

* cleanup

* Fix position manager invariant testing setup

* Fix EVM reverts

* Fix some assertions in moveliquidity handler

* Move post action checks in try to run only when transaction is successful

* position man initially working

* removed redundant invariant check

* got rewards structure running

* Update Rewards Manager handler structure

* Remove redundancy in Rewards Handler

* regression PM1 failing

* Fix invariant PM1

* Fix invariant RW1

* Add remaining rewards manager handlers

* Add drawDebt before kick reserve auction to generate some reserves

* Fix drawDebt evm revert in preUnstake

* handled broken rewardsCap

* Fixed evm revert and PM1 invariant

* Add failing regression test

* Update make commands for Position and Rewards Manager

* added check in moveLiquidity that ensure QT amount for actor doesn't change

* cleaned up

* revised invariants per Prototech labs suggestion

* cleanup invar

* added update interest to fix QT diff on moveLiquidity

* working through test failure

* resolved fee on move liquidity

* working through rewards

* initial commit

* cleanup

* low haning fruit feedback

* fixed takeReserves in rewards invariant

* added NFT position tracking for rewards manager

* cleanup

* Add comments for failing regression test

* responded to Mikes feedback

* EOF line addition

* cleaned up requires, formally added more PositionManager invar

* Quote to lps inc on move (#877)

* cleaned up requires, formally added more PositionManager invar

* cleaned up requires, formally added more PositionManager invar

* added logs

* works with diff

* cleanup

---------

Co-authored-by: Ian Harvey <[email protected]>

* cleaned up rewards and position naming

* responded to class naming

* cleaned up spaced

---------

Co-authored-by: Ian Harvey <[email protected]>
Co-authored-by: grandizzy <[email protected]>
Co-authored-by: Ed Noepel <[email protected]>
Co-authored-by: Prateek Gupta <[email protected]>
Co-authored-by: mwc <[email protected]>
Co-authored-by: prateek105 <[email protected]>
ith-harvey added a commit to ajna-finance/ajna-core that referenced this issue Jun 16, 2023
* initial draft

* got initial scaffolding running

* included Matts invaraints

* added PM1 and PM5

* Reward invariants merged (#773)

* changes needed for contract verification (#764)

* changes needed for contract verification

* gotcha

* Make NFT tokens claimable after `take`, `bucketTake`, `settle` actions (#763)

* Add test failure

* When ERC721.bucketTake always rebalance token ids from borrower to pool claimable array

* Consistent take with bucketTake, always rebalance tokens

* Check invariants in regression test

* Always rebalance tokens on settle (settle could enter with 0 collateral but still with bad debt)

* Changed epsilon from 1e16 to 1e17 in F1 and F2 invariants (#765)

* Add variable token precision for ERC721Pool invariants (#755)

* Add multiple precision of quote token in erc721Pool invariants

* Add shebang and set -e option

* Invariant framework improvements (#769)

* Update forge library, set default env vars using envOr instead reading from env file

* Remove InvariantsTestHelpers contract and move invariants helper functions to BaseInvariants

* env cleanup for invariants

* Update invariant testing scripts

* Add skipTime in merge collateral handler

* Add fuzzed time skip between kick and take/bucketTake

---------

Co-authored-by: grandizzy <[email protected]>

* Removing brownie TODOs as these tests won't be further expanded (#767)

* ERC20 R1_R2_R3_R4_R5_R6_R7_R8 invariant failure (#771)

* Add failing test

* Change cutoff for deminimus check to 1 LP in a bucket for exchange rate diffs

---------

Co-authored-by: mwc <[email protected]>

* Erc721 CT2 invariant failure (#770)

* Reproduce CT2 invariant failure

* Add collateral that could be compensated in bucket 7388 when auction settled

* Make sure MAX bucket not accounted twice. Fix test-regression target

* Fix bucketTake invariant logic in place instead in CT2 invariant. Added regression test comments.

* Fix CI, lock foundry version until resolution for foundry-rs/foundry#4835

---------

Co-authored-by: Ed Noepel <[email protected]>
Co-authored-by: Prateek Gupta <[email protected]>
Co-authored-by: mwc <[email protected]>

* working through PM3

* wrapped position calls

* made getPoolAccumulators a free function & cleaned up logging

* included free function

* increased logging robustness

* working through adding positions without passing positions to baseclass

* cleanup

* Fix position manager invariant testing setup

* Fix EVM reverts

* Fix some assertions in moveliquidity handler

* Move post action checks in try to run only when transaction is successful

* position man initially working

* removed redundant invariant check

* got rewards structure running

* Update Rewards Manager handler structure

* Remove redundancy in Rewards Handler

* regression PM1 failing

* Fix invariant PM1

* Fix invariant RW1

* Add remaining rewards manager handlers

* Add drawDebt before kick reserve auction to generate some reserves

* Fix drawDebt evm revert in preUnstake

* handled broken rewardsCap

* Fixed evm revert and PM1 invariant

* Add failing regression test

* Update make commands for Position and Rewards Manager

* added check in moveLiquidity that ensure QT amount for actor doesn't change

* cleaned up

* revised invariants per Prototech labs suggestion

* cleanup invar

* added update interest to fix QT diff on moveLiquidity

* working through test failure

* resolved fee on move liquidity

* working through rewards

* initial commit

* cleanup

* low haning fruit feedback

* fixed takeReserves in rewards invariant

* added NFT position tracking for rewards manager

* cleanup

* Add comments for failing regression test

* responded to Mikes feedback

* EOF line addition

* added

* cleaned up requires, formally added more PositionManager invar

* Quote to lps inc on move (#877)

* cleaned up requires, formally added more PositionManager invar

* cleaned up requires, formally added more PositionManager invar

* added logs

* works with diff

* cleanup

---------

Co-authored-by: Ian Harvey <[email protected]>

* cleaned up rewards and position naming

* responded to class naming

* cleaned up spaced

* working through rewards, tests passing

* working through invariants

* rewards working cleanly

* clean up before looking into RW5

* finnished adding RW6

* cleanup

* comment cleanup

* updated with emergency unstake

* cleaned up error handling

---------

Co-authored-by: Ian Harvey <[email protected]>
Co-authored-by: grandizzy <[email protected]>
Co-authored-by: Ed Noepel <[email protected]>
Co-authored-by: Prateek Gupta <[email protected]>
Co-authored-by: mwc <[email protected]>
Co-authored-by: prateek105 <[email protected]>
@Evalir
Copy link
Member

Evalir commented Aug 14, 2023

@mds1 is this closeable?

@mds1
Copy link
Collaborator

mds1 commented Aug 14, 2023

I think so, if anyone disagrees just let me know why and we can reopen

@mds1 mds1 closed this as completed Aug 14, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
Archived in project
Development

No branches or pull requests

4 participants