feat/gauge-proxy-v2 and related updates #3

UrAvgDeveloper · 2022-09-29T16:44:34Z

No description provided.

larrythecucumber321 · 2022-09-29T18:33:10Z

src/dill/gauge-proxy-v2.sol

+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.1;
+
+library Address {


We should decouple the (larger) libraries and interfaces in dedicated files/folders

larrythecucumber321 · 2022-09-29T18:41:39Z

src/dill/gauge-proxy-v2.sol

+        _;
+    }
+
+    modifier onlyGov() {


This might be more appropriate to include in ProtocolGovernance

larrythecucumber321 · 2022-09-29T18:47:23Z

src/dill/gauge-proxy-v2.sol

+        address _jar,
+        address _governance,
+        address _distribution,
+        string[] memory _rewardSymbols,


We shouldn't be storing arbitrary rewardSymbols in our contracts - contract addresses should be the first class citizens (in events and whatever else). If needed, the cononical symbols can be obtained using IERC20(token).symbol()

Fixed this.

larrythecucumber321 · 2022-09-29T19:32:30Z

src/dill/gauge-proxy-v2.sol

+        _deposit(amount, account, 0, block.timestamp, false);
+    }
+
+    function depositForAndLock(


What are some scenarios where a delegated deposit is useful? Since the user holding the tokens will have to approve the gauge contract for spending the token, they might as well go all the way and ultimately perform the deposit themselves right, rather than waiting for someone to eventually do it?

Needs discussion. Although these functions cause no harm and might support an use case of infinite approval to the platform and the platform auto renewing your rewards as deposits.

larrythecucumber321 · 2022-09-29T21:15:59Z

src/dill/gauge-proxy-v2.sol

+                        thisStake.ending_timestamp;
+
+                    // Get the weighted-average lock_multiplier
+                    uint256 numerator = (lock_multiplier * timeBeforeExpiry) +


As I understand it, claims that are made after the lock expires receive the original undecayed lock. For example, if a user locks for 1 year and doesn't make a claim until 1 year and 1 days later, they will receive a full 2.5x boost for their lock (when it would have normally decayed if they claimed throughout the year).

Yes I will average the decay.

larrythecucumber321 · 2022-09-29T21:21:08Z

src/dill/gauge-proxy-v2.sol

+            uint256 combined_boosted_amount = (liquidity * lock_multiplier) /
+                _MultiplierPrecision;
+            lockBoostedDerivedBal =
+                lockBoostedDerivedBal +


Isn't lockBoostedDerivedBal always 0? What is this variable meant to represent?

We applied a for loop and add the boost for each locked stake in each iteration in case of multiple stakes. This will be removed now that we have a single lock.

larrythecucumber321 · 2022-09-29T21:25:57Z

src/dill/gauge-proxy-v2.sol

+                combined_boosted_amount;
+        }
+
+        return dillBoostedDerivedBal + lockBoostedDerivedBal;


dillBoostedDerivedBal reaches a maximum of the user's _balance to reflect a 2.5x boost whereas lockBoostedDerivedBal follows a different paradigm and reaches a maximum of _balance * 2.5, meaning that the impact of a locked stake outweighs that of a DILL lock. I believe they have to match up otherwise the math becomes incorrect.

Good catch on the math difference between the 2 boosts. Need to rethink if this is possible similar to the DILL boost calculation. Although at the end the derived balance due to different boosts is relative (max being 2.5 and min being 1). I am thinking even if both boosts have different calcs, at the end it might be okay due to the relativity. I will still rethink this to maintain consistency.

larrythecucumber321 · 2022-09-29T21:41:54Z

src/dill/gauge-proxy-v2.sol

+
+    function setMultipliers(uint256 _lock_max_multiplier) external onlyGov {
+        require(
+            _lock_max_multiplier >= uint256(1e18),


Use _MultiplierPrecision for this

larrythecucumber321 · 2022-09-29T21:45:36Z

src/dill/gauge-proxy-v2.sol

+        emit LockedStakeMaxMultiplierUpdated(lockMaxMultiplier);
+    }
+
+    function setMaxRewardsDuration(uint256 _lockTimeForMaxMultiplier)


I think these functions setMaxRewardsDuration and setMultipliers are dangerous because it disturbs existing derivedSupply and derivedBalance calculations by naively mutating critical global variables. I honestly think these are better off removed.

larrythecucumber321

These comments only include a review of the GaugeV2 and VirtualGaugeV2 contracts for now.

Regarding contract structure, I think these two contracts should both inherit from a GaugeBase abstract contract as there is much shared code/functionality between them.

Also, I think there is also fundamental difference in how we understand the virtual gauges to function. In your implementation, you seem to envision jars having the ability to deposit into the gauge on behalf of users. I would like to understand the full flow, but I don't believe that our jars, as currently designed/deployed, are able to implement this proxy deposit/withdrawal function.

My personal understanding of the purpose of virtual guages was that we could migrate the legacy GaugeProxy users to the new system without the friction of requiring them to withdraw and re-stake. This would've involved querying the legacy gauge for users' balances to determine the rewards they should receive. Although this might not be feasible as updateReward cannot be frequently called in this scenario. Would love to hear your thoughts.

larrythecucumber321 · 2022-09-29T23:42:00Z

src/dill/gauge-middleware.sol

+    }
+
+    function changeGaugeProxy(address _newgaugeProxy) external {
+        require(msg.sender == governance, "can only be called by gaugeProxy");


Error message should say governance

larrythecucumber321 · 2022-09-30T06:49:35Z

src/dill/gauge-proxy-v2.sol

+        if (_indefinite == true) {
+            _delegate.indefinite = true;
+        } else if (_delegate.prevDelegate == address(0)) {
+            _delegate.endPeriod = currentPeriodId + _periodsCount - 1;


Why is it that the endPeriod is subtracted by 1 week if there was no prior delegate?

So if there is a previous delegate, the count for the end period of the next period starts from the next period as the prevDelegate will be active for the current period. If no previous delegate, just start the count from the current period.

larrythecucumber321 · 2022-09-30T06:53:19Z

src/dill/gauge-proxy-v2.sol

+                currentId > _delegate.updatePeriodId) ||
+                (_delegate.prevDelegate == msg.sender &&
+                    currentId == _delegate.updatePeriodId) ||
+                (_delegate.prevDelegate == address(0) &&


Doesn't this condition make it so that if the user previously had no delegate and decided to delegate this period, that ANYONE would be able to vote on their behalf?

(_delegate.prevDelegate == address(0) && currentId == _delegate.updatePeriodId

The second check in the && condition ensures that this will work only if a delegate is set. If not set _delegate.updatePeriodId is 0 and currentId is never 0. If previous delegate is set, this OR condition is not checked as it passes for the previous OR conditions.

larrythecucumber321 · 2022-09-30T06:59:01Z

src/dill/gauge-proxy-v2.sol

+        address[] memory _rewardTokens = new address[](1);
+        _rewardSymbols[0] = "PICKLE";
+        _rewardTokens[0] = address(PICKLE);
+        address vgauge = virtualGaugeMiddleware.addVirtualGauge(


For consistency, gauges[_token] can be assigned the return value from addVirtualGauge, like above

larrythecucumber321 · 2022-09-30T07:28:16Z

src/dill/gauge-proxy-v2.sol

+                    _totalWeight;
+                if (_reward > 0) {
+                    uint256 reward_ = uint256(_reward);
+                    uint256[] memory rewardArr = new uint256[](1);


I see that the gauges are meant to accomodate multiple types of rewards, but this distribute function never allows for the other rewards to be realized (as there's an onlyDistribution modifier on the gauge contracts)

Check for onlyDistribution modifier

Add limit of 8 reward tokens

larrythecucumber321 · 2022-10-01T19:30:49Z

test/02_gaugeProxyV2_periodizationTest.js

+      console.log("Rewards to Pickle gauge", pickleRewards.toString());
+
+      const yvecrvRewards = await pickle.balanceOf(yvecrvGaugeAddr);
+      console.log("Rewards to pyveCRV gauge", yvecrvRewards.toString());


I would expect to see an assertion that the average weekly rewards currently in each gauge are equal to those of the first week.

larrythecucumber321 · 2022-10-01T19:35:13Z

test/02_gaugeProxyV2_periodizationTest.js

+  it("Should vote successfully (second voting)", async () => {
+    console.log("-- Voting on LP Gauge with 100% weight --");
+    const gaugeProxyFromUser = GaugeProxyV2.connect(userAddr);
+    populatedTx = await gaugeProxyFromUser.populateTransaction.vote(


Would it perhaps be more useful to have another user vote (with different weights) and test the feature that votes from past periods persist if untouched?

larrythecucumber321 · 2022-10-03T07:37:49Z

test/07_virtualGaugeV2.test.js

+    ).to.be.revertedWith("set jar first");
+
+    // Set Jar
+    await Jar.setVirtualGauge(VirtualGaugeV2.address);


I don't see a new Jar contract with the virtual gauge interfaces

feat/gauge-proxy-v2 and related updates

6973f66

UrAvgDeveloper requested a review from larrythecucumber321 September 29, 2022 16:44

larrythecucumber321 reviewed Sep 29, 2022

View reviewed changes

larrythecucumber321 reviewed Sep 30, 2022

View reviewed changes

larrythecucumber321 reviewed Oct 1, 2022

View reviewed changes

larrythecucumber321 reviewed Oct 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/gauge-proxy-v2 and related updates #3

feat/gauge-proxy-v2 and related updates #3

UrAvgDeveloper commented Sep 29, 2022

larrythecucumber321 Sep 29, 2022 •

edited

Loading

larrythecucumber321 Sep 29, 2022

larrythecucumber321 Sep 29, 2022 •

edited

Loading

UrAvgDeveloper Sep 30, 2022

larrythecucumber321 Sep 29, 2022

UrAvgDeveloper Oct 4, 2022

larrythecucumber321 Sep 29, 2022

UrAvgDeveloper Oct 4, 2022

larrythecucumber321 Sep 29, 2022 •

edited

Loading

UrAvgDeveloper Oct 4, 2022

larrythecucumber321 Sep 29, 2022

UrAvgDeveloper Oct 4, 2022

larrythecucumber321 Sep 29, 2022

larrythecucumber321 Sep 29, 2022 •

edited

Loading

larrythecucumber321 left a comment

larrythecucumber321 Sep 29, 2022

larrythecucumber321 Sep 30, 2022

UrAvgDeveloper Oct 4, 2022

larrythecucumber321 Sep 30, 2022

UrAvgDeveloper Oct 7, 2022

larrythecucumber321 Sep 30, 2022

larrythecucumber321 Sep 30, 2022

UrAvgDeveloper Oct 6, 2022

UrAvgDeveloper Oct 7, 2022

larrythecucumber321 Oct 1, 2022

larrythecucumber321 Oct 1, 2022

larrythecucumber321 Oct 3, 2022

feat/gauge-proxy-v2 and related updates #3

Are you sure you want to change the base?

feat/gauge-proxy-v2 and related updates #3

Conversation

UrAvgDeveloper commented Sep 29, 2022

larrythecucumber321 Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larrythecucumber321 Sep 29, 2022 • edited Loading

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

larrythecucumber321 Sep 29, 2022 • edited Loading

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

larrythecucumber321 Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

larrythecucumber321 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

larrythecucumber321 Sep 29, 2022 •

edited

Loading

larrythecucumber321 Sep 29, 2022 •

edited

Loading

larrythecucumber321 Sep 29, 2022 •

edited

Loading

larrythecucumber321 Sep 29, 2022 •

edited

Loading