fix: enable/disable earning in HubPortal #20

0xIryna · 2024-11-19T21:10:37Z

Proposed changes:

remove isApproveEarner check from enableEarning() and disableEarning() functions, since the same check already exists in MToken startEarning() and stopEarning(address) functions.
implement "virtual index" for HubPortal calculated proportionally to the current M Token index.

Benefits of "virtual index":

allows re-enabling earning after it was disabled.
fixes Kirill M-01 finding since Spoke Token index won't "jump" from EXP_SCALE_ONE to the latest current index on the Hub but will be increased gradually.

Drawbacks of "virtual index":

M Token index on Spoke chains will be different from M Token index on the Hub chain, which might confuse users and integrators.

github-actions · 2024-11-19T21:12:59Z

LCOV of commit `6683a6e` during Forge Coverage #110

Summary coverage rate:
  lines......: 96.7% (203 of 210 lines)
  functions..: 90.5% (67 of 74 functions)
  branches...: 78.7% (37 of 47 branches)

Files changed coverage rate:
                                 |Lines       |Functions  |Branches    
  Filename                       |Rate     Num|Rate    Num|Rate     Num
  =====================================================================
  src/HubPortal.sol              | 100%     37| 100%    11|75.0%      4

github-actions · 2024-11-19T21:27:54Z

Changes to gas cost

Generated at commit: 49a0bbc1a2bdbe1f532cc3506d37e683941a6cd1, compared to commit: b8b570fc01e64445006eccc1b67867f7d1864917

🧾 Summary (20% most significant diffs)

Contract	Method	Avg (+/-)	%
SpokePortal	attestationReceived	-3,319 ✅	-3.00%

Full diff report 👇

Contract	Deployment Cost (+/-)	Method	Min (+/-)	%	Avg (+/-)	%	Median (+/-)	%	Max (+/-)	%	# Calls (+/-)
SpokePortal	5,327,645 (0)	attestationReceived	54,466 (-5,318)	-8.90%	107,260 (-3,319)	-3.00%	135,383 (-4,479)	-3.20%	135,383 (-4,479)	-3.20%	7 (0)

src/HubPortal.sol

toninorair · 2024-11-20T02:55:43Z

src/HubPortal.sol

+        super._initialize();
+
+        // set _disablePortalIndex to the default value on first deployment
+        if (_disablePortalIndex == 0) {


why this check is required? isn't it 0 by default on initialize?

I want to set _disablePortalIndex to EXP_SCALED_ONE only during first initialization, and do not override it after

toninorair · 2024-11-20T02:57:12Z

src/HubPortal.sol

-    /// @dev Array of indices at which earning was enabled or disabled.
-    uint128[] internal _enableDisableEarningIndices;
+    /// @dev The M token's index when earning was most recently enabled
+    uint128 private _enableMTokenIndex;


why names_disablePortalIndex and _enableMTokenIndex are not symmetrical? why private and not internal?

_disablePortalIndex is the last index of the Portal when earning was disabled, it's "virtual" index.
_enableMTokenIndex is the index of M Token when earning was enabled, the "actual" index.
Private because we're not planning to inherit from HubPortal

src/HubPortal.sol

0xIryna requested review from PierrickGT and toninorair November 19, 2024 22:22

PierrickGT requested changes Nov 19, 2024

View reviewed changes