feat: V2 sidechain gauges and boosting #1
Conversation
UrAvgDeveloper
requested review from
manymoneymanny,
larrythecucumber321 and
MisterMard
There was a problem hiding this comment.
Because the folder structure has changed between feat/gauge-proxy-v2 and this current branch (e.g. src/dill/gauge-proxy-v2.sol => src/dill/gauge-proxies/gauge-proxy-v2.sol, I'm not able to evaluate the changes. Would you be able to match the current structure in the base branch and rebase so that a diff can be made?
Re: tests, if you plan on writing more GaugeProxyV2 tests, we would probably benefit from abstracting the setup process into a common file so there's less boilerplate
|
|
||
| pragma solidity ^0.8.1; | ||
|
|
||
| contract DillSideChain { |
There was a problem hiding this comment.
Might be more descriptive to name it something like DillSideChainBalanceStore
| deployer | ||
| ); | ||
|
|
||
| const SidechainGaugeProxy = await upgrades.deployProxy( |
There was a problem hiding this comment.
Woah, this upgrades plugin is cool
| Deploy Guage on sidechain | ||
| */ | ||
| console.log("-- Deploying Gaugev2 contract --"); | ||
| const gaugeV2 = await ethers.getContractFactory( |
There was a problem hiding this comment.
Why aren't the sidechain gauges deployed using the factory pattern like for the mainnet gauges?
| /* | ||
| Register Gauge on sidechain gauge proxy on sidechain | ||
| */ | ||
| await SidechainGaugeProxy.addGauge(gaugeToken.address, GaugeV2.address); |
There was a problem hiding this comment.
SidechainGaugeProxy.addGauge probably doesn't require token as an argument right? The property is directly obtainable from the Gauge interface.
| IERC20(token).balanceOf(msg.sender) < amount | ||
| ) { | ||
| uint256 old_balance = IERC20(_underlying).balanceOf(token); | ||
| IERC20(_underlying).safeTransferFrom(msg.sender, token, amount); |
There was a problem hiding this comment.
I don't believe that the user has the amount to transfer in this case. It should be IERC20(token).balanceOf(msg.sender) instead
| periods[_periodToDistribute].amount == 0, | ||
| "All period distribution compleated" | ||
| ); | ||
| int256[] memory _weights = periods[periodToDistribute].weights; |
There was a problem hiding this comment.
If the sendRewards and distribute calls might ever be de-coupled, would we want to use the previous week's weights if the current week's sendRewards call was missed (similar to that of mainnet's GaugeProxyV2)?
| // Check if stake already exists and if it is unlocked | ||
| if (thisStake.liquidity > 0) { | ||
| require( | ||
| (!stakesUnlocked || !stakesUnlockedForAccount[msg.sender]) && |
There was a problem hiding this comment.
I believe it should be an OR for the last evaulation. Any of these conditions being satisfied means the user can withdraw their stake and should do so before locking again.
Also, per my feedback on our initial review, I don't see functionality which allows users to escape the boost decay (by extendin an existing lock for example).
| periodId | ||
| ); | ||
|
|
||
| rewardTokenDetails[_rewardToken] = token; |
There was a problem hiding this comment.
Why is rewardTokenDetails being updated here? I don't see any updates to it.
resolves pickle-finance/internal#69