Saner naming conventions in the contract [small refactor]

[jellegerbrandy]

Some proposals for making the code more readable and the API more friendly:

ABI changes

• pendingApprovalClaim --> submitClaim
• dismissPendingApprovalClaim --> dismissClaim
• approveClaim --> approveClaim (remains the same)

Changes related to ERC4246

Cf. #124

internal changes

• merge HatMaster and HATVaults into a single contract
• user.amount --> user.shares
• pool.totalUsersAmount --> pool.totalShares
• HatMaster.add() --> remove this function, add the logic under addPool

Other changes

• The term Reward is used in the contract for two distinct value transfers: (a) the HATs reward that goes to contributors to a pool (i.e. the "LP Staking Reward") and (b) the reward that is paid out when a valid vulnerability is found (which goes to hackker, committe, etc etc). This is confusing (for example, the function safeTransferReward should only touch rewards of the first type, not the second, but there is no way of knowing from the function name alone). As the term reward is used for (a) also int he Masterchef contracts (on which HatMaster is based, cf. https://github.com/pancakeswap/pancake-farm/blob/master/contracts/MasterChef.sol), we propose here to refer to case (b) in all cases as Bounty. For example:

  • RewardsSplit --> BountySplit
  • hackerReward --> HackerBounty
  • ClaimReward --> Bounty etc etc.

• Rename checkWithdrawRequest --> checkWithdrawAndResetWithdrawRequest as it checks that the withdraw is legal (and not withdraw request) and also sets the withdrawRequest to 0

[lirona]

also maybe rename WithdrawRequests to be a bit more informative? it means the time when WithdrawRequest pending period had ended in case the last action was WithdrawRequests, or 0 if last action was deposit or withdraw. so maybe WithdrawRequestPendingFinished ?

[lirona]

also decide on pool vs. vault

[lirona]

@jellegerbrandy what about approver --> committee ?

[lirona]

_startBlock -> _startRewardingBlock ?

[jellegerbrandy]

Sorry just seeing this now :)

• About WithdrawRequests: I think your suggestion is a bit long, so maybe not such a good one
• pool --> vault seems to me much better, also more in line with https://eips.ethereum.org/EIPS/eip-4626
• approver --> committee seems right to me

[jellegerbrandy]

also lets remove the Log from PendingApprovalLog, PendingRewardsLevelsLog and PendingRewardsLevelsLog

[lirona]

• @jellegerbrandy and _startBlock -> _startRewardingBlock ?
• About WithdrawRequests, I prefer long meaningful variable names then short confusing ones, but that's me.. How about WithdrawEnableStart ?
• UserInfo.amount --> UserInfo.shares
• PoolInfo.totalUsersAmount --> PoolInfo.totalUsersShares
• factoredAmount --> userShares

[jellegerbrandy]

ok, all this seems good :)

[lirona]

great :) and what about PoolInfo.balance --> PoolInfo.totalSupply ?

[jellegerbrandy]

So the erc4626 standard which is my bible now calls the shares "shares" and the underlying token "assets". So I'd vote for totalAssets, perhaps. Supply is not so appropriate, I think - these assets are not really "available", they are actually locked up 2 times a day :)

[lirona]

Cool. I have to dig deep into this as well. also thought rewardsLevels --> bugSeverityLevels because we use the term severity for it. or could be just BountyLevels

[lirona]

_pid --> _vaultID

[lirona]

move committeeCheckIn from poolReward to poolInfo ?

[lirona]

all refactoring tasks in a to-do list: https://www.notion.so/refactoring-96b5eb494bb4423a810fe3abe3486a7b