All your rebase are belong to me (convert to non-rebasing token) #81

0xchaosbi · 2022-07-15T22:00:56Z

Relates #75

Rebasing is dead, long live rebasing

TODO:

tests, incl. testing that the exchange rate only changes after the first rewards are received, and multiple stakes doesn't change the exchange rate
update unstake- ~~and claim-related~~ events to have both AVAX and stAVAX amounts
add getExchangeRate function

src/AvaLido.sol

src/stAVAX.sol

…error

…earebelongtome

jnic

Some comments, nothing blocking.

jnic · 2022-07-19T21:10:17Z

src/test/AvaLido.t.sol

+        lido.claimRewards();
+
+        // assert new exchange rate
+        assertEq(lido.exchangeRateAVAXToStAVAX(), 982318271119842829);


Might be worth pulling these out into constants to make it clear, e.g.

Suggested change

assertEq(lido.exchangeRateAVAXToStAVAX(), 982318271119842829);

uint256 EXCHANGE_RATE = 982318271119842829; // 10e18 / 1.018

assertEq(lido.exchangeRateAVAXToStAVAX(), EXCHANGE_RATE);

Are we testing unstaking or are we testing the exchange rate? I think the exchange rate calculations should be tested separately and here it's enough to assert that it's not 1 (if at all).

src/test/AvaLido.t.sol

Willyham · 2022-07-19T22:10:56Z

src/test/AvaLido.t.sol

+
+    // NB: this test fails because the numbers chosen can never match up no matter the precision.
+    // Known issue of Solidity, floating point numbers & division. This round down behaviour is expected.
+    // function testCannotClaimMoreAVAXThanDepositedBeforeRewards() public {


We could assert that the the amount you claim is <= the the amount deposited?

Or we could assert that it's within the range of our known rounding margin. Something like:

assert( abs (old - new - margin) == 0)?

Willyham · 2022-07-19T22:21:26Z

src/test/AvaLido.t.sol

        ) = lido.unstakeRequests(requestId);

        assertEq(requester, USER1_ADDRESS);
        assertEq(requestAt, uint64(block.timestamp));
        assertEq(amountRequested, 5 ether);
        assertEq(amountFilled, 0 ether);
        assertEq(amountClaimed, 0 ether);
+        assertEq(stAVAXLocked, 5 ether);

        // Second withdrawal.


Not sure what the second withdrawal is really testing here? Could simplify by having 1 test which tests multiple withdrawals and remove these second withdrawals from these tests?

Feels like we're testing too much in each test.

Willyham · 2022-07-19T22:22:41Z

src/test/AvaLido.t.sol

+        lido.claimRewards();
+
+        // assert new exchange rate
+        assertEq(lido.exchangeRateAVAXToStAVAX(), 982318271119842829);


Are we testing unstaking or are we testing the exchange rate? I think the exchange rate calculations should be tested separately and here it's enough to assert that it's not 1 (if at all).

Willyham · 2022-07-19T22:23:23Z

src/test/AvaLido.t.sol

+        cheats.deal(rTreasuryAddress, 0.2 ether);
+        lido.claimRewards();


Could make your life a bit simpler by using an unrealistic amount of rewards such that the value is 1:2. That makes the assertions clearer imo - I'm not sure if 5.09 is the right amount at a glance, but if it's 1:2 I can easily tell 5 -> 10 is correct

Willyham · 2022-07-19T22:24:26Z

src/test/AvaLido.t.sol

+        assertEq(amountClaimed, 0 ether);
+        assertEq(stAVAXLocked, 5 ether);
+
+        // Second withdrawal.


Same here, not sure what this is testing

Willyham · 2022-07-19T22:29:13Z

src/test/AvaLido.t.sol

        assertEq(amountRequested3, 0.1 ether);
        assertEq(amountFilled3, 0.1 ether);
    }

+    function testMultipleFillUnstakeRequestsSingleFillAfterRewards() public {


Not sure what this is really testing

Willyham · 2022-07-19T22:29:44Z

src/test/AvaLido.t.sol

+        assertEq(lido.protocolControlledAVAX(), 10.09 ether);
+        assertEq(lido.exchangeRateAVAXToStAVAX(), 991080277502477700);
+        assertEq(lido.exchangeRateStAVAXToAVAX(), 1.009 ether);


Again don't think we need to assert this

Willyham · 2022-07-19T22:30:02Z

src/test/AvaLido.t.sol

+        assertEq(lido.exchangeRateAVAXToStAVAX(), 991080277502477700);
+        assertEq(lido.exchangeRateStAVAXToAVAX(), 1.009 ether);
+
+        // So we withdraw 1 AVAX and lock 0.99108... stAVAX


Willyham · 2022-07-19T22:31:03Z

src/test/AvaLido.t.sol

+        cheats.deal(rTreasuryAddress, 0.1 ether);
+        lido.claimRewards();
+
+        assertEq(lido.protocolControlledAVAX(), 10.09 ether);
+        assertEq(lido.exchangeRateAVAXToStAVAX(), 991080277502477700);
+        assertEq(lido.exchangeRateStAVAXToAVAX(), 1.009 ether);
+
+        // So we withdraw 1 AVAX and lock 0.99108... stAVAX
+        cheats.prank(USER1_ADDRESS);
+        lido.requestWithdrawal(1 ether);


Willyham · 2022-07-19T22:34:20Z

src/test/AvaLido.t.sol

+        assertEq(lido.protocolControlledAVAX(), 10.09 ether);
+        assertEq(lido.exchangeRateAVAXToStAVAX(), 991080277502477700);
+        assertEq(lido.exchangeRateStAVAXToAVAX(), 1.009 ether);


Rather than concerning the test with the values, you could do oldRate = lido.exchangeRate... and then later assert that the currentRate == oldRate.

Ps that would also make it viable to do a fuzz test where the rewards param is fuzzed

refactor: change stAVAX to a non-rebasing token

297b1f0

0xchaosbi requested a review from Willyham July 15, 2022 22:00

0xchaosbi commented Jul 15, 2022

View reviewed changes

src/AvaLido.sol Outdated Show resolved Hide resolved

0xchaosbi commented Jul 15, 2022

View reviewed changes

src/AvaLido.sol Outdated Show resolved Hide resolved

WIP: change Avalido logic to accommodate non-rebasing token

0da1c6f

0xchaosbi force-pushed the allyourrebasearebelongtome branch from 0ed7d8d to 0da1c6f Compare July 15, 2022 23:06

0xchaosbi added 4 commits July 16, 2022 18:04

WIP: get basic deposit working with and without rewards

e59d0cf

WIP: unstake maybe works but has ERC20 approval issue

aab6a33

WIP requestWithdrawal works but needs testing

685849c

WIP: Claim functionality

1030b6d

jnic reviewed Jul 17, 2022

View reviewed changes

src/AvaLido.sol Outdated Show resolved Hide resolved

jnic reviewed Jul 17, 2022

View reviewed changes

src/stAVAX.sol Outdated Show resolved Hide resolved

jnic and others added 11 commits July 17, 2022 13:56

forge install: openzeppelin-contracts-upgradeable

8e2d68a

Migrate stAVAX to upgradeable contract

a70d359

test: uncomment most existing tests before adding more non-rebase tests

ed1e67a

Merge branch 'main' into allyourrebasearebelongtome

d52f9f8

Fix Foundry config warning

be3fd9b

WIP: non-rebasing mostly finished, writing tests and fixing rounding …

b5eface

…error

Use Math.sol for calcs

6d533a3

Use Math.sol for calcs

4d7d1de

Re-enable and fix relevant stAVAX unit tests

8f7446a

WIP: original tests working, adding new tests

374cc32

test: add non 1:1 exchange rate Claim tests

9256850

0xchaosbi changed the title ~~[WIP] All your rebase are belong to me~~ All your rebase are belong to me (convert to non-rebasing token) Jul 19, 2022

0xchaosbi and others added 5 commits July 19, 2022 16:49

Merge branch 'main' of github.com:AvaLido/contracts into allyourrebas…

faa7e02

…earebelongtome

WIP: unstake request tests

bec3db6

Fix import

ca0114d

WIP: update tests to use new MPC functions

1d0761b

test: clean up and finish tests after MPC changes

a261450

0xchaosbi force-pushed the allyourrebasearebelongtome branch from cae65d2 to a261450 Compare July 19, 2022 21:08

jnic reviewed Jul 19, 2022

View reviewed changes

jnic approved these changes Jul 19, 2022

View reviewed changes

Willyham reviewed Jul 19, 2022

View reviewed changes

0xchaosbi merged commit 8de3d5f into main Jul 19, 2022

0xchaosbi deleted the allyourrebasearebelongtome branch July 19, 2022 22:50

0xchaosbi mentioned this pull request Jul 19, 2022

Address test comments in non-rebasing PR #86

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All your rebase are belong to me (convert to non-rebasing token) #81

All your rebase are belong to me (convert to non-rebasing token) #81

0xchaosbi commented Jul 15, 2022 •

edited

Loading

jnic left a comment

jnic Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

Willyham Jul 19, 2022

	assertEq(lido.exchangeRateAVAXToStAVAX(), 982318271119842829);
	uint256 EXCHANGE_RATE = 982318271119842829; // 10e18 / 1.018
	assertEq(lido.exchangeRateAVAXToStAVAX(), EXCHANGE_RATE);

		cheats.deal(rTreasuryAddress, 0.2 ether);
		lido.claimRewards();

All your rebase are belong to me (convert to non-rebasing token) #81

All your rebase are belong to me (convert to non-rebasing token) #81

Conversation

0xchaosbi commented Jul 15, 2022 • edited Loading

jnic 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

0xchaosbi commented Jul 15, 2022 •

edited

Loading