xdaichain / easy-staking-contracts Goto Github PK

I noticed that the Solidity optimizer is turned off in truffle config. Is any reason for keeping it disabled?

The sigmoid function contains x - b expression which allows moving sigmoid to left and right (depending on the sign of b), but the current b in the contract is uint256, i.e. always positive. Consider making it int256.

Currently, claimTokens doesn't allow claiming STAKE tokens from the balance of EasyStaking contract.

Consider changing that requirement from require(_token != address(token) to _token != address(token) || token.balanceOf(address(this)) > totalStaked and add the corresponding description for claimTokens outlining when such a case can occur (after the owner of STAKE token contract removes the address of EasyStaking contract from the list of minters).

Currently, _setLiquidityProvidersRewardContract requires the input address to be a contract. Let's change the requirement from require(_contractAddress.isContract()) to require(_contractAddress != address(0)) as we are not sure we will use a contract initially.

_withdraw function should always revert when _amount is greater than balances[_sender][_id].

Currently, getCurrentAccruedEmission public getter only returns the user's share of emission. Consider making it return LPs' share as well.

to ensure it consumes an acceptable amount of gas for very big x and c parameters in the sigmoid function.

so that it would look like this:

    function onTokenTransfer(address _sender, uint256 _amount, bytes calldata) external returns(bool) {
        require(msg.sender == address(token), "only token contract is allowed");
        if (!locked) {
            _deposit(_sender, ++lastDepositIds[_sender], _amount);
        }
        return true;
    }

Let's change the message inside require in this line to _a should be <= MAX_EMISSION_RATE/2.

EasyStaking contract assumes it will only work with the existing STAKE token contract which address won't be changed. I propose to remove setToken function from the EasyStaking contract.

Let's add the following scenarios to the tests:

1. A user withdraws their entire deposit by several parts (more than two parts, different amounts for each part).

2. A user withdraws their balance (and emission) with the amount parameter set to zero and with amount set to the total deposit amount. The result in both cases should match (entire deposit withdrawal).

3. Different users make deposits and withdrawals and the totalStaked is changed correctly.

4. Ensure that date is reset to the current when replenishing the existing deposit.

I have a little concern regarding this line - AFAIU, it checks whether the deposit with the specified id exists, or not.

But what if some deposit was created and then fully closed? In this case, the specified id will be in the range _depositId > 0 && _depositId <= lastDepositIds[msg.sender], but balances[_sender][_id] and depositDates[_sender][_id] will be zero, so the user will be able to create a deposit with the same ID again.

Maybe we should also check the balance for zero? To avoid an ability to create a new deposit having an old ID.

Please, make sure we have a unit test for the case described in #6 - i.e. making a new deposit from scratch with the previously used id should work fine.

Does it require 2 minting functions in the staking protocol?

This line in _deposit:

This line in _withdraw:

We forgot to add events for the following functions:

setFee
setWithdrawalLockDuration
setWithdrawalUnlockDuration
setTotalSupplyFactor
setSigmoidParameters
setLiquidityProvidersRewardAddress

Please ensure unit tests cover this condition:

Is it possible to combine staking into one grouping rather than separate groups each time you buy STAKE. For example, I bought STAKE deposited it in easy stake and the next day bought some more. When I went to deposit it, it automatically placed it in a new stake of its own. It would be beneficial if you could add them into 1, even if that meant forfeiting some of the time you initially staked on.

Please fully review and update README according to the latest changes

I propose to add that field returned by the _mint function to those events (after balance field of the event).

Currently, _getEmissionRateBasedOnTotalStakedAmount calculates emission rate based on the totalSupply. The proposal is to make the supply in denominator configurable by the owner to be able to increase an emission APR making it closer to the upper bound of 7.5% if totalStaked is too small.

Such a denominator shouldn't exceed totalSupply.

Let's set explicit versions in package.json, e.g.

"@openzeppelin/contracts-ethereum-package": "2.5.0"

instead of

"@openzeppelin/contracts-ethereum-package": "^2.5.0"

This will help to prevent something in the code used from changing in the future.

Please review and update comment for the _getAccruedEmission internal function (timePassed missed in the description).