redacted-cartel / contracts-v1 Goto Github PK

King @realkinando is back, hyped for V2 👀 . Here's my feedback from looking at the contracts in v2-core

Mariposa.sol

• State variables
  • cap doesn't seem to be changed after it's set in the constructor. You can make it immutable
    • Also, is reaching the cap an issue? Will we need a method that can raise/lower it?
• Constructor
  • epochSeconds_ is never set, which would cause distribute to always revert since it'd never be greater than lastEpoch (which also never seems to be set)
• For integer types, specify the number of bits so we can do variable packing (e.g. uint8/16/32/.../256)
• Should use a formatter so our code is consistent visually
  • VSC Prettier extension
  • Our Prettier config https://github.com/redacted-cartel/pirex/blob/new-arc/.prettierrc.json
• Add checks for all params unless it's obviously redundant/unnecessary (e.g. would be worth it for this constructor's params since it's cheaper to check to do the checks than redeploy because human error - unless there's a setter method we can use)
• Optimization example of this for loop
  • Outer loop header
    • There's no need to initialize the control variable with 0 as it's already the default value
    • The loop condition is evaluated on each iteration so it'd be more efficient to assign the result of currentEpoch - lastEpoch to a variable and use that (vs. doing the calculation on each iteration)
      • Wouldn't currentEpoch - lastEpoch equal 1?
  • Inner loop header
    • Use j <= collectionCount instead of j < collectionCount + 1 (otherwise it'll do the calculation on every iteration, similar to the point above)
  • Inner loop body
    • Assign repeatedly accessed values to a variable (e.g. Collection storage c = getCollection[j])
    • Should break the logic into separate methods which may help with readability
• distribute may run out of gas if there are too many collections in the future

RevenueDistributor

• Mariposa feedback applies here
• State variables
  • lockToken can be immutable - never re-assigned outside of constructor
  • offset can be immutable - never re-assigned outside of constructor
• Need to use SafeTransferLib if dealing with potentially untrustworthy tokens
• The internal methods below (and ones like it - there are many) should be made public or external (if they don't need to be called internally) and the redundant external methods can be removed:

    function _getCurrentRound() internal view returns (uint round){
        round = (block.timestamp + (offset * 1 weeks)) / (4 * 1 weeks);
    }

    function getCurrentRound() external view returns (uint round){
        round = _getCurrentRound();
    }

    function _getTimestampFromRound(uint round) internal view returns (uint timestamp){
        timestamp = ((round * 4) + offset) * 1 weeks;
    }

    function getTimestampFromRound(uint round) external view returns (uint timestamp){
        timestamp = _getTimestampFromRound(round);
    }

    function _getRewardHash(address token, bool conviction) internal view returns(bytes32 rewardHash){
        rewardHash = keccak256(abi.encode(token,conviction));
    }

    function getRewardHash(address token, bool conviction) external view returns(bytes32 rewardHash){
        rewardHash = _getRewardHash(token, conviction);
    }

• Remove this redundant call since rewardRoundHash is the same thing

I'll update this again when I go through the other contracts.

Need to update the original Thecosomata contract with any applicable changes from ThecosomataETH as @realkinando mentioned today.