redacted-cartel / contracts-v2 Goto Github PK

• Review current implementation
• Read Code from Olympus Bonds, v1/v2 Bonds, assess parts to use
• Lock interfaces with Mariposa.sol

• Review Sablier Stream code
• Other implementations of Vesting Contracts
• Easiest Way to set this up ~50 addresses and different vesting formats

• is token migration really necessary? (use the same token as before)
• If necicarry :
• Industry standard migartions
• their pitfalls
• The best lessons

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)
  • 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])
    • May help with readability if the logic for conditionally setting some of the struct members
• 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.

• how minting budget/rewards interface with sub-projects
• Standard incentives Setup Scripts
• Associate with contract or call data (to determine if action is incentivized)

• Assess Other implementations (ie Convex, why lock token)
• Should this be an NFT? compare with pirex lock token implementation

taking over branch: #14
Requirements:

• Accounting Contract that allows Contracts/Addresses mint btrfly up to an allowance.
• Allowance can be set and changed by the contract owner.
• Max-supply should be respected
• should track current allowances.
• should track minters
• should track

Already Done:

• Contract with set interfaces

Need to be done:

• Testing (look at ./tests/RLocker.ts)
• Fuzzing if necessary.

Port over Test Helpers

• Review Contract
• Asses Best Way to Allocate minting Privileges
• Asses Best Way for Policy to Move Allocations to contracts
• Asses Best way to Determine Minting Budgets

• Review Code
• Compare with Hidden Hand and Pirex Distributor Contracts
• Compare to industry standard