Cleanup expired schedules #41

itirabasso · 2022-12-16T18:18:39Z

No description provided.

plaintextpaco

this looks good but it's incomplete.

we should include tests like:

a scenario which causes a locked contract with the previous implementation, but which can be recoverable with this one
assert internal state of the rewardschedule array
perhaps unitarily test the function

also, IMO the cleanup function should be callable directly, with a bound on the amount of iterations, so really bad situations can be recovered from (since adding a rewardSchedule is O(1))

plaintextpaco · 2023-02-06T21:33:39Z

src/contracts/aludel/AludelV3Lib.sol

+
+            // check if the period is already expired
+            if (block.timestamp - schedule.start >= schedule.duration) {
+                // length decreces


https://www.learncpp.com/cpp-tutorial/comments/

these comments state that which is obvious from looking at the code, they're better suited to explain why the code does what it does

plaintextpaco · 2023-02-06T21:37:04Z

src/contracts/aludel/AludelV3Lib.sol

+    function cleanupExpiredRewardSchedules(
+        IAludelV3.AludelData storage aludel
+    ) internal {
+        // check : measure how much gas this actually saves


looks like something to implement in this PR 🙃

This should reduce the computational cost of `calculateLockedShares` because if we remove expired schedules, which are fully unlocked at that stage, we need to iterate less elements during the calculation. remove length optimization

if we avoid accessing the array's length this is how the gas-snapshots changes ``` test_many_users_multiple_stakes() (gas: -211 (-0.013%)) test_funding_shares() (gas: -592 (-0.094%)) test_single_unstake(uint8,uint40,uint256,uint256) (gas: 701603 (6.979%)) Overall gas change: 700800 (2.337%) ```

* test_fund_without_approval * test_fund_remove_expired_schedules

expired reward schedules are removed after the program is funded again.

plaintextpaco

issues blocking merging are:

modularization of tests
lack of tests regarding schedule removal, which left bugs slip through

plaintextpaco · 2023-03-01T19:32:01Z

src/test/AludelV3.t.sol

@@ -28,9 +28,9 @@ import {Utils} from "./Utils.sol";
 import {UserFactory} from "./UserFactory.sol";

 import "forge-std/console2.sol";
+import "forge-std/StdUtils.sol";


import individual names used or put the package into a name to avoid polluting the namespace

I understand this concern for contracts that are deployed but I don't see the point of enforcing this in tests.
I would prefer to have the whole package available instead of having to specify each of the names I want to use.

you can do import "forge-std/StdUtils" as STD and access every exported thing as STD.thing

this'd not pollute the global namespace, and would make explicit where every usage comes from. In that way, if I want to find the docs for a particular function when reviewing the code or taking a look at it some time later, I can easily figure out where to look, whereas if the name is in the global namespace I'd have to do a fair bit more guessing

src/contracts/aludel/AludelV3Lib.sol

src/test/AludelV3.t.sol

plaintextpaco · 2023-03-01T22:22:35Z

src/test/AludelV3.t.sol

+
+        IAludelV3.AludelData memory data = aludel.getAludelData();
+        // before fund there should be no schedules
+        assertEq(data.rewardSchedules.length, 0);


this looks like an assert that you added when playing around with the codebase to see what would work, and not one conclusive to test_fund_remove_expired_schedules

plaintextpaco · 2023-03-01T22:26:34Z

src/test/AludelV3.t.sol

+        vm.warp(block.timestamp + 1);
+        aludel.fund(reward, duration);
+        // so there should be two schedules
+        assertEq(aludel.getAludelData().rewardSchedules.length, 2);


the body of the test doesn't fully relate to its description. it states that fund should remove expired schedules yet none is removed.

plaintextpaco · 2023-03-01T22:35:26Z

src/test/AludelV3.t.sol

+    }
+}
+
+contract Given_A_New_Aludel_Is_Being_Funded is Test {


This contract has a lot of code duplication with AludelV3Test

Please create a 3rd contract with the setup common to both and make them extend the setUp method and add whatever extra state variables are necessary.

I agree with this, but I was hesitant to add it right away.
Would you agree to have a few more iterations of these test contracts before creating the common contract for all tests?

I'm not saying we should have a common contract for all tests, that feels like premature optimization given the current state of affairs, I was talking about just this testfile.

If you look at both contracts side by side, you can see the repetition easily:

I don't like the idea of merging so much repeated code, but I wouldn't mind it in a draft PR

src/test/AludelV3.t.sol

plaintextpaco · 2023-03-01T22:46:01Z

src/test/AludelV3.t.sol

+    {
+        reward = bound(reward, 1, MAX_REWARD);
+        duration = bound(duration, 1, 1000 days);
+


why all the whitespace?

I personally don't mind it.
if neither forge fmt nor lint-sol cries about it, I'll just go with it.

for me the whitespace in source code contributes to how I understand it, I expect empty lines to have a similar function to the paragraph separation in natural languages, and when I see two emtpy lines together I expect some big context switch, such as in sections in a book. (or, say, two different contracts, or public vs internal functions of the same contract)

Having the whitespace mean nothing at all (such as having 1-2 emtpy lines between code that accomplishes the same thing) adds a bit of cognitive noise to processing code for me.

this, for example, is how this function makes most sense to me:

reward = bound(reward, 1, MAX_REWARD); duration = bound(duration, 1, 1000 days); Utils.fundMockToken(admin.addr(), rewardToken, reward); rewardToken.approve(address(aludel), reward); aludel.fund(reward, duration); assertEq(aludel.getAludelData().rewardSharesOutstanding, reward * BASE_SHARES_PER_WEI);

... or it could have no empty lines at all. This whitespace pattern is what I usually prefer for asserting reverts, now that I think about it.

The whitespace of this particular function is not a hill I die on, but I expect it to mean something

itirabasso changed the base branch from main to aludelV3Lib December 16, 2022 18:18

itirabasso force-pushed the aludelV3Lib branch from 40b4234 to 852483e Compare December 19, 2022 05:29

itirabasso force-pushed the aludelV3Lib branch 3 times, most recently from a2caeba to 4cbb0df Compare January 16, 2023 19:24

itirabasso force-pushed the cleanup_expired_schedules branch from 9911b02 to bc48be8 Compare January 16, 2023 20:00

itirabasso force-pushed the aludelV3Lib branch 2 times, most recently from d3bdf4d to bb00a8f Compare January 17, 2023 11:26

itirabasso force-pushed the cleanup_expired_schedules branch from bc48be8 to bfb9db2 Compare January 17, 2023 12:09

itirabasso marked this pull request as ready for review January 17, 2023 15:23

itirabasso force-pushed the aludelV3Lib branch 2 times, most recently from 2d4240c to e8532c0 Compare January 25, 2023 11:51

itirabasso force-pushed the cleanup_expired_schedules branch from bfb9db2 to 6e92dd2 Compare January 31, 2023 11:17

plaintextpaco requested changes Feb 6, 2023

View reviewed changes

itirabasso force-pushed the cleanup_expired_schedules branch from 6e92dd2 to 47c33df Compare February 8, 2023 13:57

itirabasso changed the base branch from aludelV3Lib to main February 8, 2023 14:02

itirabasso force-pushed the cleanup_expired_schedules branch 3 times, most recently from 25fa14f to 4da2d5e Compare February 14, 2023 18:38

itirabasso added 8 commits February 16, 2023 15:28

clean up expired reward schedules in fund

c3ec44d

This should reduce the computational cost of `calculateLockedShares` because if we remove expired schedules, which are fully unlocked at that stage, we need to iterate less elements during the calculation. remove length optimization

add single unstake test

03f80e3

update gas-snapshot

c8acfbe

add some tests

eb6d185

* test_fund_without_approval * test_fund_remove_expired_schedules

update hardhat test

7d2ce70

expired reward schedules are removed after the program is funded again.

add aludel fund tests

771c681

forge fmt

0d0eb3e

itirabasso force-pushed the cleanup_expired_schedules branch from 4da2d5e to 0d0eb3e Compare February 24, 2023 16:59

plaintextpaco requested changes Mar 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup expired schedules #41

Cleanup expired schedules #41

itirabasso commented Dec 16, 2022

plaintextpaco left a comment

plaintextpaco Feb 6, 2023

plaintextpaco Feb 6, 2023

plaintextpaco left a comment

plaintextpaco Mar 1, 2023

itirabasso Mar 2, 2023

plaintextpaco Mar 6, 2023

plaintextpaco Mar 1, 2023

plaintextpaco Mar 1, 2023

plaintextpaco Mar 1, 2023

itirabasso Mar 2, 2023

plaintextpaco Mar 6, 2023

plaintextpaco Mar 1, 2023

itirabasso Mar 2, 2023

plaintextpaco Mar 6, 2023 •

edited

Loading

Cleanup expired schedules #41

Are you sure you want to change the base?

Cleanup expired schedules #41

Conversation

itirabasso commented Dec 16, 2022

plaintextpaco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plaintextpaco 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

plaintextpaco Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

plaintextpaco Mar 6, 2023 •

edited

Loading