fix(f02-mocks): align mock with f02 as implemented (#19)

- StepWeightRecords is uncancellable; the discretionary path must not revoke a gate-originated write
- weight records require floor <= v_start <= cap
- SetShares rejects zero shares and duplicate recipients
- methods below FRC-0042's floor are forbidden to EVM callers, not merely unhandled
- GetState is no longer dispatched: reads move to a mockState() view plus an explicit mockSettle(), and the view does not settle, as f02 behaves

Author: wjmelements

src/lib/FVMRewardMethod.sol

@@ -1,10 +1,10 @@
 // SPDX-License-Identifier: Apache-2.0 OR MIT
 pragma solidity ^0.8.36;
 
-// FRC-0042 method numbers for the f02 (Reward actor) stream-splitting methods proposed by
-// FIP-1270 (https://github.com/filecoin-project/FIPs/pull/1270) and tracked upstream at
-// filecoin-project/builtin-actors#1764. The actor does not exist yet; these numbers are the
-// FRC-0042 hash of each method name, defined here so FVMRewards and its mock agree on them.
+// FRC-0042 method numbers for the f02 (Reward actor) stream-splitting methods specified by
+// FIP-0118 (https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0118.md) and implemented in
+// filecoin-project/builtin-actors#1764. Each is the FRC-0042 hash of the method's name. Defined
+// here so FVMRewards and its mock cannot disagree.
 uint64 constant REGISTER_STREAM = 386660827;
 uint64 constant REMOVE_STREAM = 1623858416;
 uint64 constant SET_WEIGHT_RECORDS = 3362570548;
@@ -13,7 +13,6 @@ uint64 constant SET_DISTRIBUTION = 3872725033;
 uint64 constant CANCEL_PENDING = 187585191;
 uint64 constant SET_SHARES = 2414422607;
 uint64 constant CLAIM = 4045527845;
-uint64 constant GET_STATE = 1397113977;
 
 /// @dev The activation timelock SWA writes to f02 are queued under: the mainnet default,
 /// 7 days in epochs (30s/epoch); migration-set per network, so the real actor also exposes it

src/lib/FVMRewardTypes.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0 OR MIT
 pragma solidity ^0.8.36;
 
-/// @notice A stream's Distribution kind (FIP-1270 Section 2.4).
+/// @notice A stream's Distribution kind (FIP-0118 Section 2.4).
 /// @dev IMPLICIT streams store no writer (f02 resolves the recipient from protocol state).
 ///      EXPLICIT streams are paid out per a wallet-to-share map written by their designated writer.
 enum DistributionKind {

src/lib/FVMRewards.sol

@@ -18,10 +18,11 @@ import {
 } from "./FVMRewardMethod.sol";
 import {WeightRecord, DistributionKind, Share, PendingOp} from "./FVMRewardTypes.sol";
 
-/// @notice Calls the f02 (Reward actor) methods proposed by FIP-1270, for the Stream Weight
-/// Actor (solstice#3) and Service Rewards Actor (solstice#4). f02 does not exist upstream yet
-/// (filecoin-project/builtin-actors#1764); the wire format here is this repo's best-effort
-/// CBOR encoding of the FIP's draft method signatures, not an upstream-confirmed ABI.
+/// @notice Calls the f02 (Reward actor) methods specified by FIP-0118, for the Stream Weight
+/// Actor (solstice#3) and Service Rewards Actor (solstice#4).
+/// @dev The param encodings are unverified against f02's own serialization vectors
+/// (builtin-actors `actors/reward/tests/types_test.rs`). Until they are checked against those,
+/// agreement with the mock in this repo is not evidence of agreement with the chain.
 /// @dev Every write params blob is a CBOR array of positional fields; addresses are encoded as
 /// CBOR byte strings wrapping an f410 delegated address (0x04, 0x0a, 20 address bytes). No
 /// abi.encode/abi.decode: params are hand-built in assembly and Filecoin BigInt returns are

test/mocks/FVMCallActorByIdWithReward.sol

@@ -11,18 +11,15 @@ import {FVMCallActorById} from "fvm-solidity/mocks/FVMCallActorById.sol";
 
 import {FVMRewardActor} from "./FVMRewardActor.sol";
 
-/// @notice Extends fvm-solidity's CALL_ACTOR_BY_ID mock with a case for REWARD_ACTOR_ID.
-/// @dev fvm-solidity's FVMCallActorById has no branch for the reward actor (f02) and is not
-///      ours to modify -- these methods don't exist in builtin-actors yet (see
-///      FVMRewardActor.sol) and will only ever be called by solstice's own contracts. Rather
-///      than reimplement burn/power/datacap/miner handling here, this contract intercepts
-///      only REWARD_ACTOR_ID and forwards everything else, unmodified, to a freshly deployed
-///      FVMCallActorById via `delegatecall`. Using `delegatecall` (not `call`) is required: it
-///      is what preserves `address(this)`/`msg.sender` as the original caller all the way
-///      through to FVMCallActorById's `_handleBurn`, which debits `address(this).balance`
-///      expecting that to be the real caller's balance, not this contract's.
-/// @dev Etch this at CALL_ACTOR_BY_ID (replacing the vanilla FVMCallActorById) via
-///      MockRewardTest, after MockFVMTest.setUp() has already run.
+/// @notice Extends fvm-solidity's CALL_ACTOR_BY_ID mock with a branch for REWARD_ACTOR_ID (f02).
+/// @dev Serves calls addressed to f02 from FVMRewardActor and forwards every other actor id,
+///      unmodified, to an FVMCallActorById deployed at construction, leaving burn, power, datacap
+///      and miner behaviour with fvm-solidity.
+/// @dev That forward is a `delegatecall` and must stay one. It preserves `address(this)` and
+///      `msg.sender` as the original caller through to FVMCallActorById's `_handleBurn`, which
+///      debits `address(this).balance`; under `call` the debit lands on this contract instead.
+/// @dev Etch at CALL_ACTOR_BY_ID, replacing the vanilla FVMCallActorById, via MockRewardTest and
+///      after MockFVMTest.setUp() has run.
 contract FVMCallActorByIdWithReward {
     address private immutable BASE;
     FVMRewardActor private immutable REWARD;

test/mocks/FVMRewardActor.sol

@@ -11,7 +11,6 @@ import {
     SET_WEIGHT_RECORDS,
     STEP_WEIGHT_RECORDS,
     SET_SHARES,
-    GET_STATE,
     REGISTER_STREAM,
     REMOVE_STREAM,
     SET_DISTRIBUTION,
@@ -24,10 +23,13 @@ import {WeightRecord, DistributionKind, Share, PendingOp} from "../../src/lib/FV
 /// @dev Weights, and per-orchestrator shares, are WAD-scaled: 1e18 == 1.0 == 100%.
 int256 constant WAD = 1e18;
 
-/// @dev Mock-only caps; f02 requires these limits to exist but never fixes their values.
+/// @dev f02's caps, fixed by FIP-0118.
 uint64 constant MAX_STREAMS = 8;
 uint256 constant MAX_RECIPIENTS = 64;
 
+/// @dev FRC-0042's floor. Below it a method is internal API, closed to EVM callers.
+uint64 constant FIRST_EXPORTED_METHOD_NUMBER = 1 << 24;
+
 /// @dev Same value as WAD, typed uint256, so summing shares needs no signed-to-unsigned cast.
 uint256 constant SHARE_TOTAL = 1e18;
 
@@ -36,8 +38,8 @@ struct LedgerRow {
     uint256 amount;
 }
 
-/// @dev Enumerable, prunable address->uint256 balance -- plain mappings-plus-array, not the
-/// builtin-actor's CBOR-behind-a-CID shape (fine: nothing implements that wire format yet).
+/// @dev Enumerable, prunable address->uint256 balance, as plain mappings plus an array. This is
+/// not f02's on-chain shape, which is CBOR behind a CID, and no contract can read either one.
 struct Ledger {
     mapping(address => uint256) amount;
     mapping(address => uint256) indexPlusOne; // 0 == not tracked
@@ -103,11 +105,21 @@ struct PendingView {
     address writer;
 }
 
+/// @notice Everything mockState reports, bundled so call sites don't juggle an 8-way tuple.
+struct MockState {
+    uint256 totalMintedReward;
+    uint256 totalBurnMinted;
+    uint256 totalServiceMinted;
+    uint64 nextTransitionEpoch;
+    uint64 swaTimelockEpochs;
+    StreamView[] streams;
+    TombstoneView[] tombstones;
+    PendingView[] pendingWrites;
+}
+
 /// @notice Mock for the Filecoin Reward actor (f02), covering its stream-splitting methods.
 /// @dev Etch at REWARD_ACTOR_ADDRESS via MockRewardTest, which also re-etches CALL_ACTOR_BY_ID
 ///      to reach handle_filecoin_method below.
-/// @dev GetState persists due writes rather than only projecting them; behaviorally identical
-///      once `effectiveEpoch` has passed.
 contract FVMRewardActor {
     /// @dev Survives vm.etch: immutables are baked into runtime bytecode at deploy time.
     Vm private immutable VM;
@@ -198,7 +210,7 @@ contract FVMRewardActor {
         emit BlockRewardAwarded(br, minerPortion, servicePortion, burnAmount);
     }
 
-    /// @notice Test helper: an EXPLICIT stream's wallet-to-share map, without the GetState round trip.
+    /// @notice Test helper: an EXPLICIT stream's wallet-to-share map.
     function getShares(uint64 streamId) external view returns (Share[] memory) {
         return _streams[streamId].shares;
     }
@@ -214,7 +226,8 @@ contract FVMRewardActor {
     }
 
     /// @notice Test helper: the clamp(v_start + slope*(e-t_start), floor, cap) math, exposed
-    /// directly since it isn't a dispatched method (GetState already projects each weight).
+    /// directly: an SWA has to mirror this schedule itself, and the mock is where a divergence
+    /// between its copy and f02's should surface.
     function clampWeight(WeightRecord memory record, uint64 epoch) external pure returns (int256) {
         return _clampWeight(record, epoch);
     }
@@ -234,11 +247,14 @@ contract FVMRewardActor {
         external
         returns (uint32, uint64, bytes memory)
     {
+        // restrict_internal_api: the internal API (AwardBlockReward, ThisEpochReward,
+        // UpdateNetworkKPI, Constructor) is closed to EVM callers, and everything reaching a mock
+        // through CALL_ACTOR_BY_ID is one. ThisEpochReward is not a back door.
+        if (method < FIRST_EXPORTED_METHOD_NUMBER) return (USR_FORBIDDEN, 0, "");
         _settle();
         if (method == SET_WEIGHT_RECORDS) return _queueWeightWrite(PendingOp.SET_WEIGHT, params);
         if (method == STEP_WEIGHT_RECORDS) return _queueWeightWrite(PendingOp.STEP_WEIGHT, params);
         if (method == SET_SHARES) return _setShares(params);
-        if (method == GET_STATE) return _getState();
         if (method == REGISTER_STREAM) return _registerStream(params);
         if (method == REMOVE_STREAM) return _removeStream(params);
         if (method == SET_DISTRIBUTION) return _setDistribution(params);
@@ -305,6 +321,10 @@ contract FVMRewardActor {
 
         uint256 total;
         for (uint256 i = 0; i < newShares.length; i++) {
+            if (newShares[i].share == 0) return (USR_ILLEGAL_ARGUMENT, 0, "");
+            for (uint256 j = 0; j < i; j++) {
+                if (newShares[j].wallet == newShares[i].wallet) return (USR_ILLEGAL_ARGUMENT, 0, "");
+            }
             total += newShares[i].share;
         }
         if (total != SHARE_TOTAL) return (USR_ILLEGAL_ARGUMENT, 0, "");
@@ -318,7 +338,13 @@ contract FVMRewardActor {
         return (0, 0, "");
     }
 
-    function _getState() internal view returns (uint32, uint64, bytes memory) {
+    /// @notice Test helper: the mock's whole state, read directly rather than through a method.
+    /// @dev f02 exposes no reads at all, so an SWA or SRA must mirror anything it needs in its own
+    /// state. Tests are not so constrained, and reading here keeps that asymmetry visible.
+    /// @dev A true view: it does not settle. Advancing the epoch and reading without an
+    /// intervening mutating call shows nothing applied, exactly as f02 behaves. Use mockSettle to
+    /// apply due writes.
+    function mockState() external view returns (MockState memory) {
         uint64 nowEpoch = uint64(block.number);
 
         StreamView[] memory streams = new StreamView[](_streamIds.length);
@@ -358,20 +384,21 @@ contract FVMRewardActor {
             });
         }
 
-        return (
-            0,
-            0,
-            abi.encode(
-                totalMintedReward,
-                totalBurnMinted,
-                totalServiceMinted,
-                nextTransitionEpoch,
-                swaTimelockEpochs,
-                streams,
-                tombstones,
-                pendingWrites
-            )
-        );
+        return MockState({
+            totalMintedReward: totalMintedReward,
+            totalBurnMinted: totalBurnMinted,
+            totalServiceMinted: totalServiceMinted,
+            nextTransitionEpoch: nextTransitionEpoch,
+            swaTimelockEpochs: swaTimelockEpochs,
+            streams: streams,
+            tombstones: tombstones,
+            pendingWrites: pendingWrites
+        });
+    }
+
+    /// @notice Test helper: applies due writes, as f02 does at the head of every mutating call.
+    function mockSettle() external {
+        _settle();
     }
 
     function _registerStream(bytes calldata params) internal returns (uint32, uint64, bytes memory) {
@@ -465,6 +492,9 @@ contract FVMRewardActor {
         if (msg.sender != swa) return (USR_FORBIDDEN, 0, "");
         // Params CBOR: [id, op]
         (uint64 id, PendingOp op) = _decodeCancelPendingParams(params);
+        // StepWeightRecords is the one uncancellable op: the discretionary path must not be able
+        // to revoke a governance-gated write.
+        if (op == PendingOp.STEP_WEIGHT) return (USR_ILLEGAL_ARGUMENT, 0, "");
         if (_pendingExists[id][op]) {
             delete _pending[id][op];
             _pendingExists[id][op] = false;
@@ -827,8 +857,10 @@ contract FVMRewardActor {
     }
 
     /// @dev Per-record sanity required at write time: 0 <= floor <= cap <= 1.
+    /// @dev validate_weight_record: floor <= v_start <= cap <= DENOM. The lower bound on floor
+    /// is implicit in f02, where these three are u64; here they are signed and it is not.
     function _sane(WeightRecord memory w) internal pure returns (bool) {
-        return w.floor >= 0 && w.floor <= w.cap && w.cap <= WAD;
+        return w.floor >= 0 && w.floor <= w.cap && w.cap <= WAD && w.vStart >= w.floor && w.vStart <= w.cap;
     }
 
     /// @dev Sum of every registered stream's weight at `atEpoch`, excluding `excludeIds`.