Feat/cantina 0.4 bulk informational

[dapp-whisperer]

Breaking Changes

Function Renames

In CdpManager

stEthFeePerUnitIndex -> cdpStEthFeePerUnitIndex

debtRedistributionIndex →cdpDebtRedistributionIndex

Description:

This issue includes a list of informational issues I have found and noted while reviewing the PR that would merge the Release 0.4 PR into the main branch.

The issues are not sorted by type or files.

1. Inside CdpManagerStorage.getICR currentETH are not ETH but SHARES

   agree, simple fix | ✅ fixed

2. Inside CdpManagerStorage.getNominalICR currentETH are not ETH but SHARES

   agree, simple fix | ✅ fixed

3. LiquityMath _computeNominalCR and _computeCR should specify if _coll is stETH or shares

   _computeNominalCR should be collShares

   _computeCR should be stEthBalance

   ✅ fixed

4. In general, there are other places where the term collateral/coll is used in a mixed way. Sometimes it's about stETH shares, sometimes it's about stETH. I think that the best thing to do is to

   1. Not use anymore the term coll/collateral and always use stETH or stETHShares

   2. Decide which is the collateral and use the other as the secondary term. So, for example, you define that stETH is the collateral and so by default when you see the term coll you know you are referring to stETH and everything else that is not coll or stETH should be explicitly defined as stETHShares

      Agree, the previous idea was to rename it collShares and stEthBalance

      These are visually and sonically distinct, clearly separating the concept

      My fix would be to apply these two universally throughout the codebase as there are many missed instances

      ♾️ in progress

5. missing full-natspec for functions like _calcSyncedAccounting, _getSyncedCdpDebtAndRedistribution and so on (I assume there are more where the natspec miss or is incomplete). Try to fully cover the functions with @notice, @dev, @param and @return when needed.

6. syncGlobalAccounting @dev comment is unfinished

7. if a grace period starts, it won't be bound to the current value of recoveryModeGracePeriod. The caller could call setGracePeriod and change the grace period that has already started by making it smaller or bigger. See Governance can to end or extend the grace period even when a grace period is already ongoing #30

8. Consider changing recoveryModeGracePeriod to recoveryModeGracePeriodDuration

recoveryModeGracePeriod → recoveryModeGracePeriodDuration
GracePeriodSet → GracePeriodDurationSet

✅ fixed

9. Refactor CdpManagerStorage to have constants, storage layouts and so on as the first thing and function after (with a proper order). Please follow the Solidity Style guide https://docs.soliditylang.org/en/latest/style-guide.html

   Agree, we will do the style guide pass as a final commit as it’ll make a mess of the diff

   ⏱️ defer to later commit

10. In CdpManagerStorage stEthFeePerUnitIndex was more meaningful before (stFeePerUnitcdp). Now it has lost the information that it's about CDP. Consider changing the name to be a reference to the single CDP index.

    Agree, changing to cdpStEthFeePerUnitIndex

    (as opposed to systemStEthFeePerUnitIndex)

    ✅ fixed

11. Same thing for debtRedistributionIndex in CdpManagerStorage(not that before was more meaningful). Consider renaming to something that is related to the CDP

    Agree, changing to cdpDebtRedistributionIndex

    (as opposed to systemDebtRedistributionIndex)

    ✅ fixed

12. In CdpManagerStorage lastEBTCDebtErrorRedistribution has "lost" the _ in the name, but lastETHError_Redistribution still has it. Use the same naming standard.

    lastETHError_Redistribution is vestigal and will be removed

    ✅ fixed

13. In CdpManagerStorage there are multiple occurrences of the variable CdpIdsArrayLength that is declared with the first letter uppercase. Follow the Solidity style guide and declare it with the first letter lowercase.

    Agree, uncapitalized to cdpIdsArrayLength

    ✅ fixed

14. In CdpManagerStorage use the same order for the input parameters (both stETH index) for _syncStEthIndex and _calcSyncedGlobalAccounting. In those function, the order is inverted and could cause confusion. Try to always stick with the same order.

15. CdpManagerStorage getSyncedICR and getSyncedTCR can be declared external because are not used by any contract

    Agree, fixed

    I would also consider removing the external getTCR() and getICR()

    For internal use, we are assuming the state is already synced

    For external use, we virtually sync

16. CdpManagerStorage getNominalICR and getICR uses the variable currentETH but inside that variable you don't have ETH or stETH but stETHShares returned by getDebtAndCollShares. Change the name of the variable accordingly
    [from @rayeaster : seems invalid, the variable there already remove ETH naming]

17. In general, it seems that the name EBTC has been replaced in many places with the general term "Debt". For example, getPendingEBTCDebtReward has been renamed getPendingRedistributedDebt. There are still many instances in the whole codebase where variables and functions refer to eBTC instead of debt (same for stETH/ETH for coll or collateral). Badger should define a very strict standard for the naming convention and apply broadly to the whole codebase. Having a mixture of both creates confusion and could lead to errors.

    Agree, moving everything to debt

18. In CdpManagerStorage _getPendingRedistributedDebt explicitly return set or return a value for pendingEBTCDebtReward because right now the else case is not handled and could lead to error/create confusion ✅ fixed

19. IEBTCToken EBTCTokenBalanceUpdated event can be removed because it's not used anywhere in the codebase ✅ fixed

20. In HintHelpers _calculateCdpStateAfterPartialRedemption the variable names should be updated to clarify when they store stETH or shares. ✅ fixed

21. In LiquidationLibrary use the same style to declare the revert message. Sometimes they start with "LiquidationLibrary: ", sometimes with "CdpManager:" and sometimes like in _reInsertPartialLiquidation they do not have the reference to the source at all ✅ fixed

22. Avoid using "magic number" 1e18 in LiquidationLibrary._liquidateIndividualCdpSetupCDPInNormalMode. Use BaseMath.DECIMAL_PRECISION. ✅ fixed

23. Avoid using "magic number" 1e18 in LiquidationLibrary._liquidateIndividualCdpSetupCDPInRecoveryMode. Use BaseMath.DECIMAL_PRECISION. ✅ fixed

24. In LiquidationLibrary the function is passing stETH shares and not stETH to the event CdpLiquidated for the parameter _coll. Please verify that it is implemented as intended ✅ fixed by adding document

25. In LiquidationLibrary to the event CdpLiquidated _premiumToLiquidator is defined as _cappedColl > _debtToColl ? (_cappedColl - _debtToColl) : 0 where _cappedColl is calculated as collateral.getPooledEthByShares(_cappedColPortion + _liquidatorReward)

26. The _liquidatorReward (the reward given to the liquidator as the gas stipend) should be outside the calculation of the _cappedColl and should be always added to the final value passed to the _premiumToLiquidator parameter. Here's an example:

  uint _cappedColl = collateral.getPooledEthByShares(_cappedColPortion);
  uint256 premium = _cappedColl > _debtToColl ? (_cappedColl - _debtToColl) : 0
  emit CdpLiquidated(
      _recoveryState.cdpId,
      _borrower,
      _totalDebtToBurn,
      _cappedColPortion,
      CdpOperation.liquidateInRecoveryMode,
      msg.sender,
      premium + collateral.getPooledEthByShares(_liquidatorReward)
  );

[from @rayeaster : IMO no change needed, current implementation is expected and discussed in discord channel]

27 . LiquidationSequencer does not check if the grace period has passed (if we are in RM). See The liquidation logic is not always using the same requirement when the system is in RM #32✅ fixed by another related PR654
28. LiquidationSequencer does not perform the same check done in LiquidationLibrary when we are in RM. Over there _liquidateIndividualCdpSetup require that _ICR <= _TCR instead in the helper _canLiquidateInCurrentMode check that _icr < _TCR. See The liquidation logic is not always using the same requirement when the system is in RM #32 ✅ fixed by another related PR654

29. Update the revert message in LiquidationSequencer because now they say "LiquidationLibrary:" ✅ fixed

30. Improvement: create a common library for the checks done by LiquidationSequencer and LiquidationLibrary to be sure to implement both of them correctly

31. In LiquidationLibrary avoid using the direct value of the Cdp Status, use the Enum instead (example _cdpStatus == 1)
    [from @rayeaster : seems invalid now, there is no usage of direct integer of Cdp Status in LiquidationLibrary]

32. Remove public visibility from the MultiCdpGetter constructor (it's ignored by solidity) ✅ fixed

33. In ICdpManagerData rename the debtToOffset attribute of the struct LiquidationValues to debtToBurn. The current name used is a concept related to Liquity and could create confusion in eBTC ✅ fixed

34. In ICdpManagerData rename the totalDebtToOffset attribute of the struct LiquidationTotals to debtToBurn. The current name used is a concept related to Liquity and could create confusion in eBTC ✅ fixed

35. In CdpManager.redeemCollateral the require(redemptionsPaused == false, ...) check can be moved at the very beginning of the function to save gas if the redemption process has been paused. ✅ fixed

Recommendation from cantina/spearbit:

Badger should consider solving the issues listed in the description section.

The main suggestion would be to:

• consolidate the renaming migration they have started to remove any possible confusion around the term collateral, stETH and shares
• remove references and nomenclatures that were relevant in the Liquity project but are not inside the eBTC context
• cover all the variables, functions and interfaces with full natspec support

In addition, all references to Liquity should be removed.

This includes the LiquityBase and LiquityMath contract names
✅ fixed

[GalloDaSballo]

If your goal was to remove Liquity from everywhere, there are still a few spots

Beside that, I went through the diff and the changes are all matched

[GalloDaSballo]

Way better with the change

[dapp-whisperer]

If your goal was to remove Liquity from everywhere, there are still a few spots

Beside that, I went through the diff and the changes are all matched

good point, for comments and obselete files (of which there are numerous for both, I planned to do that in a separate PR)

[dapp-whisperer]

@GalloDaSballo we've got to clear out this foundty failure before merging. I'm puzzled at the moment.
Feel free to merge yourself if resolved cleanly.

[rayeaster]

@GalloDaSballo we've got to clear out this foundty failure before merging. I'm puzzled at the moment. Feel free to merge yourself if resolved cleanly.

@dapp-whisperer I pushed the fix by adding missing "indexed" keyword to related events