Position invariants #854

ith-harvey · 2023-05-29T11:06:45Z

Description of change

High level

Added PositionManager invariant scaffolding
Added asserts for PostionManger in handlers
Added PM1 - PM8 invariant check
Added forge test commands for Rewards and Position invariants
cleaned up invariant logging
turned getEpochInfo() into "free" method

* 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]>

…sful

MikeHathaway · 2023-06-05T16:54:11Z

tests/forge/invariants/trace.log

@@ -0,0 +1,17 @@
+sender=0x00000000000000000000000000000000000006da addr=[tests/forge/invariants/PositionsAndRewards/handlers/RewardsHandler.sol:RewardsHandler]0xf535008760a9349da0650d92756d9a730c51c9ad calldata=claimRewards(uint256,uint256,uint256,uint256), args=[115792089237316195423570985008687907853269984665640564039457584007913129639933, 486888075223510502299880936499251496488108390102993365331518154575959314103, 1489390063300625330233647743808860618285793249553177794776030333650229253556, 29413051449830420745080834496160737679746193111333313068326]


Can remove this file with git rm if not needed

good call -> 9dec6df

MikeHathaway · 2023-06-05T17:05:36Z

.gitignore

@@ -16,6 +16,7 @@ report/
 keystore/
 broadcast/
 logFile.txt
+tests/forge/invariants/trace.log


nit: can update with a more general restriction *.log since we shouldn't be committing any log files in this repo

nice, fixed -> 9dec6df

MikeHathaway · 2023-06-05T17:07:05Z

src/RewardsManager.sol

 }
+
+    /**********************/


I like the idea of sectioning, but would be helpful to be more specific - Rewards Calculations or similar. Also, this is probably best handled in a separate PR.

agreed, separate PR

MikeHathaway

Generally looks great! Just a few small comments. Also a general nit: many of the new files are missing a new line at the end of the file

ith-harvey · 2023-06-05T17:36:58Z

Generally looks great! Just a few small comments. Also a general nit: many of the new files are missing a new line at the end of the file

fixed -> 2d93b7d

MikeHathaway

LGTM

grandizzy · 2023-06-06T06:41:11Z

tests/forge/invariants/PositionsAndRewards/PositionsInvariants.t.sol

+        _erc721impl        = _erc721poolFactory.implementation();
+        _erc20pool         = ERC20Pool(_erc20poolFactory.deployPool(address(_collateral), address(_quote), 0.05 * 10**18));
+        _pool              = Pool(address(_erc20pool));
+        _position         = new PositionManager(_erc20poolFactory, _erc721poolFactory);


nit: spacing

grandizzy · 2023-06-06T06:44:06Z

tests/forge/invariants/PositionsAndRewards/RewardsInvariants.t.sol

+
+
+
+    function invariant_rewards_RW1() public useCurrentTimestamp {


that's fine, though easy to group them now in an invariant_rewards_RW1_RW2 as they almost use same values to compare, e.g.

function invariant_rewards_RW1_RW2() public useCurrentTimestamp { // get current epoch (is incremented every kickReserve() call) uint256 curEpoch = _pool.currentBurnEpoch(); // get rewards that have been claimed uint256 claimedRewards = IBaseHandler(_handler).totalRewardPerEpoch(curEpoch); // total ajna burned by the pool over the epoch (, uint256 totalBurnedInPeriod,) = _getEpochInfo(address(_pool), curEpoch); // stake rewards cap is 80% of total burned uint256 stakeRewardsCap = Maths.wmul(totalBurnedInPeriod, 0.8 * 1e18); // check claimed rewards < rewards cap if (stakeRewardsCap != 0) require(claimedRewards < stakeRewardsCap, "Rewards invariant RW1"); // update rewards cap is 10% of total burned uint256 updateRewardsCap = Maths.wmul(totalBurnedInPeriod, 0.1 * 1e18); // check claimed rewards < rewards cap if (updateRewardsCap != 0) require(claimedRewards < updateRewardsCap, "Rewards invariant RW2"); }

grandizzy · 2023-06-06T06:45:04Z

tests/forge/invariants/PositionsAndRewards/handlers/PositionHandlerAbstract.sol

+import { ERC20Pool }                    from 'src/ERC20Pool.sol';
+
+import { UnboundedPositionsHandler } from './unbounded/UnboundedPositionsHandler.sol';
+import { BaseERC20PoolHandler }     from '../../ERC20Pool/handlers/unbounded/BaseERC20PoolHandler.sol';


nit: spacing

grandizzy · 2023-06-06T06:45:55Z

tests/forge/invariants/PositionsAndRewards/handlers/PositionHandlerAbstract.sol

+import { UnboundedPositionsHandler } from './unbounded/UnboundedPositionsHandler.sol';
+import { BaseERC20PoolHandler }     from '../../ERC20Pool/handlers/unbounded/BaseERC20PoolHandler.sol';
+
+abstract contract PositionHandlerAbstract is UnboundedPositionsHandler {


what about Base as we already use in BaseHandler so BasePositionHandler?

tests/forge/invariants/PositionsAndRewards/handlers/PositionHandlerAbstract.sol

grandizzy · 2023-06-06T06:54:14Z

tests/forge/invariants/PositionsAndRewards/handlers/unbounded/UnboundedPositionsHandler.sol

+            // assert that underlying LP balance in PositionManager of fromIndex is less than or equal to preAction and deposit time in PositionManager is 0
+            (uint256 fromLps, uint256 fromDepositTime) = _position.getPositionInfo(tokenId_, fromIndex_);
+            require(fromLps == 0); // difficult to estimate LPS, assert that it is less than
+            require(fromDepositTime == 0);


revert message needed in require

fixed -> c7c84e2

grandizzy · 2023-06-06T06:54:22Z

tests/forge/invariants/PositionsAndRewards/handlers/unbounded/UnboundedPositionsHandler.sol

+
+            // assert that underlying LP balance in PositionManager of toIndex is increased and deposit time in PositionManager is updated
+            (uint256 toLps, uint256 toDepositTime) = _position.getPositionInfo(tokenId_, toIndex_);
+            require(toLps >= preActionToLps); // difficult to estimate LPS, assert that it is greater than


revert message needed in require

fixed -> c7c84e2

grandizzy · 2023-06-06T06:54:27Z

tests/forge/invariants/PositionsAndRewards/handlers/unbounded/UnboundedPositionsHandler.sol

+            (uint256 toLps, uint256 toDepositTime) = _position.getPositionInfo(tokenId_, toIndex_);
+            require(toLps >= preActionToLps); // difficult to estimate LPS, assert that it is greater than
+            (,uint256 postActionDepositTime)= _pool.lenderInfo(toIndex_, address(_position));
+            require(toDepositTime == postActionDepositTime); 


revert message needed in require

fixed -> c7c84e2

grandizzy · 2023-06-06T06:54:34Z

tests/forge/invariants/PositionsAndRewards/handlers/unbounded/UnboundedPositionsHandler.sol

+
+            // assert that poolKey is returns zero address
+            address poolAddress = _position.poolKey(tokenId_);
+            require(poolAddress == address(0));


revert message needed in require

fixed -> c7c84e2

grandizzy · 2023-06-06T06:55:20Z

tests/forge/invariants/interfaces/IBaseHandler.sol

@@ -28,6 +28,13 @@ interface IBaseHandler {
    function increaseInReserves() external view returns(uint256);
    function decreaseInReserves() external view returns(uint256);

+    function totalRewardPerEpoch(uint256) external view returns(uint256);


suggest a different interface for pos / rewards handler

fixed -> c7c84e2

grandizzy · 2023-06-07T06:32:27Z

tests/forge/invariants/PositionsAndRewards/handlers/unbounded/BasePositionsHandler.sol

+ */ 
+abstract contract BasePositionsHandler is BaseERC20PoolHandler {
+
+    PositionManager internal _position;


please name this _positionManager and _rewardsManager for readability

Yep, fixed -> 301a11c

* 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]>

grandizzy

LGTM

Ian Harvey and others added 30 commits April 25, 2023 13:27

initial draft

e3e4803

got initial scaffolding running

3da37f9

Merge branch 'develop' into reward-position-invar

944efd0

included Matts invaraints

5c9c223

added PM1 and PM5

5c0acbb

working through PM3

8f3e2ce

Merge branch 'develop' into reward-position-invar

db2d495

wrapped position calls

16ab35f

made getPoolAccumulators a free function & cleaned up logging

d5e5176

included free function

984c57b

increased logging robustness

7e40fc3

working through adding positions without passing positions to baseclass

92d38c2

cleanup

a6fbf85

Fix position manager invariant testing setup

2a01536

Fix EVM reverts

7c87acb

Fix some assertions in moveliquidity handler

d6df724

Move post action checks in try to run only when transaction is succes…

15b946c

…sful

position man initially working

bef1d9e

merged with develop

4889822

removed redundant invariant check

30f5b9e

got rewards structure running

6c1ddc1

Update Rewards Manager handler structure

e48431f

Remove redundancy in Rewards Handler

66250a8

regression PM1 failing

b04bd7f

Fix invariant PM1

43e92e1

Fix invariant RW1

5038629

Add remaining rewards manager handlers

c3d7a00

Add drawDebt before kick reserve auction to generate some reserves

9e9bb81

Fix drawDebt evm revert in preUnstake

31edf8c

Ian Harvey added 5 commits June 3, 2023 19:19

low haning fruit feedback

f978e2a

merged with develop

8bb3850

fixed takeReserves in rewards invariant

37821b7

added NFT position tracking for rewards manager

8ada822

cleanup

e582da9

ith-harvey requested a review from grandizzy June 4, 2023 21:23

Add comments for failing regression test

93e0f71

MikeHathaway reviewed Jun 5, 2023

View reviewed changes

Ian Harvey added 2 commits June 5, 2023 13:29

responded to Mikes feedback

9dec6df

EOF line addition

2d93b7d

ith-harvey requested a review from MikeHathaway June 5, 2023 17:37

MikeHathaway approved these changes Jun 5, 2023

View reviewed changes

grandizzy suggested changes Jun 6, 2023

View reviewed changes

cleaned up requires, formally added more PositionManager invar

e3963a1

grandizzy reviewed Jun 7, 2023

View reviewed changes

ith-harvey and others added 4 commits June 7, 2023 08:11

Quote to lps inc on move (#877)

c7c84e2

* 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]>

merged

a648037

cleaned up rewards and position naming

301a11c

responded to class naming

8881b0f

ith-harvey force-pushed the positions-only branch from 06d90dc to 8881b0f Compare June 7, 2023 14:11

cleaned up spaced

4bb0e76

ith-harvey requested a review from grandizzy June 7, 2023 14:35

grandizzy approved these changes Jun 7, 2023

View reviewed changes

ith-harvey requested a review from mattcushman June 7, 2023 18:39

ith-harvey merged commit 089cb9d into develop Jun 7, 2023

ith-harvey deleted the positions-only branch June 13, 2023 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Position invariants #854

Position invariants #854

ith-harvey commented May 29, 2023 •

edited

Loading

MikeHathaway Jun 5, 2023

ith-harvey Jun 5, 2023

MikeHathaway Jun 5, 2023

ith-harvey Jun 5, 2023

MikeHathaway Jun 5, 2023

ith-harvey Jun 5, 2023

MikeHathaway left a comment

ith-harvey commented Jun 5, 2023

MikeHathaway left a comment

grandizzy Jun 6, 2023

ith-harvey Jun 7, 2023

grandizzy Jun 6, 2023

grandizzy Jun 6, 2023

ith-harvey Jun 7, 2023

grandizzy Jun 6, 2023

grandizzy Jun 6, 2023

ith-harvey Jun 7, 2023

grandizzy Jun 6, 2023

ith-harvey Jun 7, 2023

grandizzy Jun 6, 2023

ith-harvey Jun 7, 2023

grandizzy Jun 6, 2023

ith-harvey Jun 7, 2023

grandizzy Jun 6, 2023

ith-harvey Jun 7, 2023

grandizzy Jun 7, 2023

ith-harvey Jun 7, 2023

grandizzy left a comment

		@@ -0,0 +1,17 @@
		sender=0x00000000000000000000000000000000000006da addr=[tests/forge/invariants/PositionsAndRewards/handlers/RewardsHandler.sol:RewardsHandler]0xf535008760a9349da0650d92756d9a730c51c9ad calldata=claimRewards(uint256,uint256,uint256,uint256), args=[115792089237316195423570985008687907853269984665640564039457584007913129639933, 486888075223510502299880936499251496488108390102993365331518154575959314103, 1489390063300625330233647743808860618285793249553177794776030333650229253556, 29413051449830420745080834496160737679746193111333313068326]




		function invariant_rewards_RW1() public useCurrentTimestamp {

Position invariants #854

Position invariants #854

Conversation

ith-harvey commented May 29, 2023 • edited Loading

Description of change

High level

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeHathaway left a comment

Choose a reason for hiding this comment

ith-harvey commented Jun 5, 2023

MikeHathaway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy left a comment

Choose a reason for hiding this comment

ith-harvey commented May 29, 2023 •

edited

Loading