fix: replace null-pointer ambiguity with init-as-node bootstrap (#79)#124
Merged
thedavidmeister merged 35 commits intomainfrom May 4, 2026
Merged
fix: replace null-pointer ambiguity with init-as-node bootstrap (#79)#124thedavidmeister merged 35 commits intomainfrom
thedavidmeister merged 35 commits intomainfrom
Conversation
Resolves the linked-list null/zero ambiguity flagged in #79 by giving index 0 explicit semantic meaning rather than treating it as both "no node" and "node 0". The bootstrap is no longer a magic invisible slot; it is a real node at index 1 with `ACTION_TYPE_INIT_V1` (1<<7), created lazily by `_ensureBootstrap` on the first `schedule` call. Migration walks include the init bit via `BALANCE_MIGRATION_TYPES_MASK`; init is identity for stock-split balance migrations and the bootstrap node exists so every holder's cursor advances `0 → 1 → ...` uniformly through the list. Drops `totalSupplyBootstrapped`; the `nodes.length != 0` test replaces every callsite. Renames `totalSupplyLatestSplit → totalSupplyLatestCursor` to reflect that the field tracks the latest init-or-split cursor. `countCompleted` masks out init so user-facing counts stay correct. `_ensureBootstrap` snapshots OZ's `_totalSupply` into `unmigrated[0]` at schedule time — OZ's `_totalSupply == Σ _balances` still holds at that moment because schedule does not touch balances and no `_migrateAccount` has fired yet. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A second `schedule` call must not re-snapshot OZ's `_totalSupply` into `unmigrated[0]`. Without the `nodes.length != 0` short-circuit in `_ensureBootstrap`, post-first-schedule mints/burns would be double-counted once a second schedule fired. Pokes OZ's `_totalSupply` slot directly between the two schedule calls so a snapshot drift is unambiguous. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLinked-list traversal and migration tracking move to a cursor-based model anchored by a lazily-created bootstrap init node ( ChangesCursor-Based Migration Tracking and NODE_NONE Sentinels
sequenceDiagram
participant Test as Test/Caller
participant Vault as StoxReceiptVault
participant Total as LibTotalSupply
participant Nodes as LibCorporateActionNode
participant Rebase as LibReceiptRebase
Test->>Vault: trigger _update / request migration
Vault->>Total: call fold()
Total->>Nodes: nextOfType(NODE_NONE / cursor, BALANCE_MIGRATION_TYPES_MASK)
Nodes-->>Total: (nodeIndex, actionType) or NODE_NONE
Total-->>Vault: write totalSupplyLatestCursor (if advanced)
Vault->>Rebase: request migrated balances for holder(s)
Rebase->>Nodes: nextOfType(..., BALANCE_MIGRATION_TYPES_MASK)
Nodes-->>Rebase: (nodeIndex, actionType) until NODE_NONE
Rebase-->>Vault: return migrated balance and final cursor
Vault-->>Test: emit AccountMigrated / CorporateAction events with cursor indices
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/src/concrete/StoxReceipt.t.sol (1)
77-87: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winModel the init node in this mock as well.
This now asserts the production migration mask, but the mock still returns a split-only chain. That keeps the receipt integration tests on the old
0 -> 1cursor path, so a regression in bootstrap handling would still pass here. Please makeMockVault.nextOfTypeexpose the init node before the first split so these assertions run against the real0 -> 1 -> 2 ...sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/concrete/StoxReceipt.t.sol` around lines 77 - 87, MockVault.nextOfType must model the initial migration node before splits: change the logic so when cursor == 0 it returns the init node (candidate 1) with the migration-init action type and size 1, and for cursor >= 1 continue returning splits as before (use the existing splits array and ACTION_TYPE_STOCK_SPLIT_V1 for those). Ensure the mask/filter checks (BALANCE_MIGRATION_TYPES_MASK and CompletionFilter.COMPLETED) remain, and adjust indexing so the first call yields the init node and subsequent calls yield splits[cursor-1].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/concrete/StoxReceiptVault.sol`:
- Around line 124-128: The change to _update() (which now expects the new
init/bootstrap behavior in the corporate-actions flow and relies on
LibCorporateAction.CorporateActionStorage.totalSupplyLatestCursor after
LibTotalSupply.fold()) requires regenerating the LibProdDeployV3 pointer so
fallback() will delegatecall the updated facet bytecode that implements the
bootstrap node; regenerate and publish the new LibProdDeployV3 pointer/address
used by fallback(), update any frozen facet pointer constants or deployment
metadata that reference LibProdDeployV3, and re-run the deploy pointer
regeneration step so production vaults point to the new facet before merging.
In `@test/src/concrete/StoxCorporateActionsFacet.t.sol`:
- Around line 523-550: Replace hard-coded action bit literals in the tests with
the protocol’s named action-bit constants: update uses of numeric literals
passed to corporateActionHarness.nextOfType (e.g., 0, 1, 2, any combined mask
values) in testNextOfTypeCompletedEmpty and testNextOfTypeCompletedFilters to
use the repository’s action-bit constants (the ACTION_TYPE_* or
ACTION_TYPE_MASK_* symbols that represent the init/bootstrap bit and
type-1/type-2 bits) and keep CompletionFilter.COMPLETED and
corporateActionHarness.nextOfType calls unchanged; this ensures the tests
reference the canonical bit definitions rather than magic numbers.
In `@test/src/concrete/StoxCorporateActionsInvariant.t.sol`:
- Around line 385-395: The receipt-side invariant in
_assertReceiptCursorInvariant currently uses raw numeric comparisons
(RECEIPT.holderIdCursor vs VAULT.totalSupplyLatestCursor) which fails for valid
schedules where node ids are non-monotonic; update this check to use the same
list-order monotonicity logic used on the share-side (compare positions in the
id ordering rather than raw numeric value) so holderIdCursor is validated by
list-order precedence against totalSupplyLatestCursor; locate
_assertReceiptCursorInvariant and replace the numeric assert with a list-order
comparison helper (the same helper used for share-side checks) or implement an
orderIndex lookup for RECEIPT.holderIdCursor and VAULT.totalSupplyLatestCursor
and assert the indices follow the required monotonicity.
---
Outside diff comments:
In `@test/src/concrete/StoxReceipt.t.sol`:
- Around line 77-87: MockVault.nextOfType must model the initial migration node
before splits: change the logic so when cursor == 0 it returns the init node
(candidate 1) with the migration-init action type and size 1, and for cursor >=
1 continue returning splits as before (use the existing splits array and
ACTION_TYPE_STOCK_SPLIT_V1 for those). Ensure the mask/filter checks
(BALANCE_MIGRATION_TYPES_MASK and CompletionFilter.COMPLETED) remain, and adjust
indexing so the first call yields the init node and subsequent calls yield
splits[cursor-1].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01e4623a-4f5e-416d-8569-b764d2bc8d3e
📒 Files selected for processing (16)
src/concrete/StoxReceiptVault.solsrc/interface/ICorporateActionsV1.solsrc/lib/LibCorporateAction.solsrc/lib/LibRebase.solsrc/lib/LibReceiptRebase.solsrc/lib/LibTotalSupply.soltest/src/concrete/StoxCorporateActionsFacet.t.soltest/src/concrete/StoxCorporateActionsInvariant.t.soltest/src/concrete/StoxReceipt.t.soltest/src/concrete/StoxReceiptVault.t.soltest/src/concrete/StoxReceiptVaultFallbackRouting.t.soltest/src/lib/LibCorporateActionNode.t.soltest/src/lib/LibRebase.t.soltest/src/lib/LibReceiptRebase.t.soltest/src/lib/LibStockSplit.t.soltest/src/lib/LibTotalSupply.t.sol
Resolves the static check failure on PR #124. CI runs forge fmt --check; the imports in LibRebase.sol / LibTotalSupply.sol and a handful of test files were on a single line where the formatter prefers multi-line. No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Init is the primordial action type — every vault touched by `schedule` has exactly one — so it sits at the lowest bit. Reshuffles user types up by one: ACTION_TYPE_INIT_V1 = 1 << 0 (was 1 << 7) ACTION_TYPE_STOCK_SPLIT_V1 = 1 << 1 (was 1 << 0) ACTION_TYPE_STABLES_DIVIDEND_V1 = 1 << 2 (was 1 << 1) Tests now use the symbolic constants throughout instead of hardcoded bit values, except the bit-position pin test (`testActionTypeInitV1BitPosition`) which is the deliberate canary for renumbering. The pin asserts each constant's exact value plus the non-overlap invariant. Pointer files regenerated for the contracts whose bytecode embeds these constants (`StoxCorporateActionsFacet`, `StoxReceipt`, `StoxReceiptVault`, `StoxOffchainAssetReceiptVaultBeaconSetDeployer`). LibProdDeployV3 imports `DEPLOYED_ADDRESS` from the pointer files via aliased names, so the new addresses propagate automatically — no manual constant edits needed there. Also bumps the "undefined bit" used by mask-validation tests from 1 << 2 / 1 << 8 (now valid types) to 1 << 5 (still reserved). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Catches the renumbering fallout that the previous commit missed: - LibStockSplit testConstantValues: pin updated to 1 << 1 / 1 << 2. - LibCorporateActionNode testMaskWithMixedBitsPasses, testMultiBitMaskMatchesAnyBit, testTraversalRespectsTiedEffectiveTimeOrdering: use ACTION_TYPE_STOCK_SPLIT_V1 / USER_TYPES_TEST_MASK instead of bare 1 / 2 literals. - testMaskWithOnlyUndefinedBitsReverts and the facet's testFacetTraversalGettersRevertOnInvalidMask: use bit 3 (the next natural undefined bit after defined 0/1/2) instead of the arbitrary bit 5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves the renumbering breakage in the LibCorporateActionNode forward/backward sequence tests and the facet traversal tests: - assertForwardSequence / assertBackwardSequence: replace bare 1 / 2 with ACTION_TYPE_STOCK_SPLIT_V1 / ACTION_TYPE_STABLES_DIVIDEND_V1. - testFuzzEarliestCompletenessVsBruteForce: schedule with the symbolic constants, drive masks from [ACTION_TYPE_STOCK_SPLIT_V1, ACTION_TYPE_STABLES_DIVIDEND_V1, USER_TYPES_TEST_MASK]. - facet earliestActionOfType / latestActionOfType: bare 1 → constant. - testForwardBackwardConsistency: replace `schedule(3, ...)` with `schedule(1 << 3, ...)` (single-bit synthetic third type) plus matching userMask update. - forge fmt re-wrap of the two test files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit finding on PR #124: the share-side cursor monotonicity checker was upgraded to walk forward via `next` pointers (so that a later-scheduled earlier-effective split — landing at a numerically smaller node id — is still recognised as a valid forward step). The receipt-side checks still used raw `assertGe`, which would falsely flag the same valid schedule. Both `_recordReceiptCursor` and the public `invariantReceiptCursorMonotonicity` now use `cursorReachableForward`, matching the share-side semantics. The 256-run invariant fuzz is green. Also corrects the `CorporateActionNode` NatSpec: real user-scheduled nodes start at index 2 (index 1 is the lazily-created `ACTION_TYPE_INIT_V1` bootstrap), not at index 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pointer hashes were stale against current bytecode; CodeRabbit flagged LibProdDeployV3 must be regenerated before merge. NatSpec in LibCorporateAction documents the target 0-based indexing scheme that the rest of this commit series implements. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…l-sentinel-uint-max # Conflicts: # src/generated/StoxOffchainAssetReceiptVaultBeaconSetDeployer.pointers.sol # src/generated/StoxReceipt.pointers.sol # src/generated/StoxReceiptVault.pointers.sol # test/src/concrete/StoxReceiptVault.t.sol
…s pending) Replaces the index-0-as-null sentinel with `NODE_NONE` (= type(uint256).max) so every array index — including the bootstrap node now at idx 0 — can be a real walkable cursor without aliasing "no node" semantics. Source code changes: - Bootstrap node lives at idx 0 (previously idx 1 with sentinel at idx 0). - User-scheduled nodes start at idx 1 (previously idx 2). - `prev`/`next`/head-or-tail null sentinel is `NODE_NONE`. - `nextOfType(NODE_NONE, ...)` / `prevOfType(NODE_NONE, ...)` mean "from head/tail inclusive"; passing a real index means "after/before this node" as before. - `totalSupplyLatestCursor` initialised to `NODE_NONE` by `_ensureBootstrap` so the first `fold()` walks head-inclusive. - `effectiveTotalSupply` walks bootstrap-inclusive instead of seeding from `unmigrated[0]` then walking after-bootstrap. - Receipt-side cursor default 0 = "at bootstrap" (identity for splits). - Library tests updated for new index semantics. Pending: many concrete-vault and invariant tests still pin HEAD's bootstrap-as-init indices and need a similar shift (subtract 1 from node-index assertions; replace `(0, ` with `(NODE_NONE, ` for "from head/tail" intent). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…de tests Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/src/concrete/StoxReceipt.t.sol (1)
71-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn
NODE_NONEfrom the mock traversal.
ICorporateActionsV1.nextOfTypenow exhausts withNODE_NONE, but this mock still returns0, which is a real cursor under the bootstrap-node model. That makes the test double diverge from the contract it implements and can hide cursor-walk regressions in the receipt rebase tests.🧪 Suggested fix
-import {CompletionFilter} from "../../../src/lib/LibCorporateActionNode.sol"; +import {CompletionFilter, NODE_NONE} from "../../../src/lib/LibCorporateActionNode.sol"; ... uint256 candidate = cursor + 1; if (candidate > splits.length) { - return (0, 0, 0); + return (NODE_NONE, 0, 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/concrete/StoxReceipt.t.sol` around lines 71 - 87, The mock nextOfType currently returns 0 when exhausted which collides with a valid cursor; update the exhaustion branch in nextOfType to return NODE_NONE instead of 0 (i.e., when candidate > splits.length return (NODE_NONE, 0, 0)); keep the existing checks for BALANCE_MIGRATION_TYPES_MASK and CompletionFilter.COMPLETED and preserve returning (candidate, ACTION_TYPE_STOCK_SPLIT_V1, 1) for valid candidates so behavior matches ICorporateActionsV1's exhaust sentinel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/LibReceiptRebase.sol`:
- Around line 54-64: Update the NatSpec for the function in LibReceiptRebase.sol
to reflect the new bootstrap model: state that `cursor == 0` is the
pre-init/null sentinel (not the bootstrap node), explain that fresh `(holder,
id)` pairs start at the init/bootstrap node after initialization rather than at
cursor 0, and clarify that `newCursor` may advance to the init node even when no
further completed stock-split nodes were applied; keep the existing notes that
`migratedBalance` is the balance after sequential multiplier application and is
always 0 when `storedBalance == 0`.
In `@src/lib/LibTotalSupply.sol`:
- Around line 71-88: Update the NatSpec to reflect that the bootstrap is the
real init node at index 0 (not a separate “pre-init” cursor): reword the
description around unmigrated[0], LibCorporateAction._ensureBootstrap,
ACTION_TYPE_INIT_V1 and effectiveTime to state the bootstrap creates the init
node at index 0 and the first user split is at index 1; remove references to a
“pre-init” cursor and any wording that implies an init node at index 1, and
clarify that fold() and totalSupplyLatestCursor advance starting from index 0 so
_migrateAccount, BALANCE_MIGRATION_TYPES_MASK, and subsequent mint/burn routing
assumptions hold with the init at index 0.
In `@test/src/concrete/StoxReceiptVault.t.sol`:
- Around line 201-203: Update the remaining assertions that expect the first
completed-split cursor to be 1 to expect 2 instead: change any assertEq(..., 1,
...) or hardcoded toCursor == 1 checks in the tests
testAccountMigratedFiresOnZeroBalanceCursorAdvance,
testAccountMigratedFiresOnTruncationCollision, and
testAccountMigratedDoesNotReEmitWhenAlreadyAtLatest to assert 2; specifically
adjust checks against vault.migrationCursor(ALICE) and any toCursor variables so
they match the bootstrap-adjusted indexing (expected value 2).
In `@test/src/lib/LibCorporateActionNode.t.sol`:
- Around line 470-473: Tests and helper walks still treat 0 as the
"no-match"/termination cursor but the codebase now uses the NODE_NONE sentinel;
update all assertions and control checks to use NODE_NONE instead of literal 0.
Specifically, change the assertions around calls to h.earliest and h.latest
(with USER_TYPES_TEST_MASK and CompletionFilter.COMPLETED) to expect NODE_NONE,
and update any fuzz/base-case logic that does checks like cursor == 0 to use
cursor == NODE_NONE; ensure any helper functions that interpret "no match"
return or terminate on 0 are adjusted to check against the NODE_NONE constant so
deterministic and fuzz paths (including those around earliest/latest,
CompletionFilter, and USER_TYPES_TEST_MASK) behave correctly.
In `@test/src/lib/LibRebase.t.sol`:
- Around line 52-54: The cursor assertions and comments are off by one because
_ensureBootstrap inserts a completed init node on the first schedule, so update
all affected expectations (the assertEq(cursor, ...) and related comments in the
tests that reference cursor) to account for that extra init node (i.e., follow
the returned action ids rather than treating 0 as already at bootstrap);
specifically, adjust the assertions to match the action id offsets produced by
schedule/_ensureBootstrap (as testPartialMigration does), and update surrounding
explanatory comments to explain that cursor 0 advances to init at index 1 before
any user split.
In `@test/src/lib/LibReceiptRebase.t.sol`:
- Around line 20-26: Update the test mock to include the bootstrap init node so
receipt rebase sees the real vault ordering: insert an ACTION_TYPE_INIT_V1 node
at cursor 0 in the mock's preloaded nodes, renumber the existing scheduled split
nodes to start at 1, and ensure nextOfType and getActionParameters return the
init node (as completed) before returning subsequent split nodes; keep the
NODE_NONE sentinel behavior unchanged. Also adjust/add the bootstrap-only test
(the path that calls schedule) to assert the cursor advances 0 -> 1, referencing
nextOfType, getActionParameters, NODE_NONE, ACTION_TYPE_INIT_V1 and the schedule
call.
In `@test/src/lib/LibTotalSupply.t.sol`:
- Around line 103-118: The tests assume the old bootstrap indexing and must be
updated to account for the new init node inserted before the user split: after
calling schedule(...) and fold() the fold will advance through the init node
then the split, so increment any expected cursor indices by 1 — e.g., update
assertions using h.totalSupplyLatestCursor() (expected value -> +1) and calls to
h.onAccountMigrated(..., toCursor, ...) (increment the toCursor argument by 1)
in testAccountMigration and the other affected test cases (lines referenced: the
similar assertions at 145-168, 186-187, 269-270, 330-335, 364-378) so they
reflect the init node shift.
---
Outside diff comments:
In `@test/src/concrete/StoxReceipt.t.sol`:
- Around line 71-87: The mock nextOfType currently returns 0 when exhausted
which collides with a valid cursor; update the exhaustion branch in nextOfType
to return NODE_NONE instead of 0 (i.e., when candidate > splits.length return
(NODE_NONE, 0, 0)); keep the existing checks for BALANCE_MIGRATION_TYPES_MASK
and CompletionFilter.COMPLETED and preserve returning (candidate,
ACTION_TYPE_STOCK_SPLIT_V1, 1) for valid candidates so behavior matches
ICorporateActionsV1's exhaust sentinel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2666da36-7ebb-4a6e-b089-897d9fcc1331
⛔ Files ignored due to path filters (1)
src/generated/StoxCorporateActionsFacet.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (19)
src/concrete/StoxCorporateActionsFacet.solsrc/concrete/StoxReceipt.solsrc/concrete/StoxReceiptVault.solsrc/interface/ICorporateActionsV1.solsrc/lib/LibCorporateAction.solsrc/lib/LibCorporateActionNode.solsrc/lib/LibRebase.solsrc/lib/LibReceiptRebase.solsrc/lib/LibTotalSupply.soltest/src/concrete/StoxCorporateActionsFacet.t.soltest/src/concrete/StoxCorporateActionsInvariant.t.soltest/src/concrete/StoxReceipt.t.soltest/src/concrete/StoxReceiptVault.t.soltest/src/concrete/StoxReceiptVaultFallbackRouting.t.soltest/src/lib/LibCorporateActionNode.t.soltest/src/lib/LibRebase.t.soltest/src/lib/LibReceiptRebase.t.soltest/src/lib/LibStockSplit.t.soltest/src/lib/LibTotalSupply.t.sol
…cheme Bootstrap moves from idx 1 to idx 0; user splits start at idx 1 instead of 2. Fold's "no run yet" sentinel becomes NODE_NONE (set by _ensureBootstrap) instead of 0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Storage layout pin, schedule index returns, and tied-effective-time ordering tests updated. Many other facet/invariant test sites still need similar shifts — separate commits for each focused area. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- testFacetTraversalGettersFilterParameter: index shifts (2/3 -> 1/2) - testScheduleViaFacetReturnsActionIndex / testScheduleCorporateActionEmitsEvent: idx 1 not 2 - testFuzzScheduleStockSplitsSequentialIndex: i+1 not i+2 - testCancelBootstrapNodeRevertsAlreadyComplete: cancel(0) (idx 0 = bootstrap) - testNodeStructFieldLayoutPin: derive elementSize from idx 1, not idx 2 - testFacetTraversalGettersEmpty: assert NODE_NONE for null cursor - testBootstrapNodePropertiesAfterFirstSchedule: bootstrap.prev = NODE_NONE Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Source bytecode changes from the 0-based refactor cascade through five contracts: facet → receipt vault → receipt → beacon-set deployer → unified deployer. Multi-pass forge clean + BuildPointers needed because each Zoltu deploy is one-shot per session. Fallback routing test sites also shifted (idx 2→1) and assertEq nulls became NODE_NONE. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/src/concrete/StoxCorporateActionsInvariant.t.sol (1)
117-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
NODE_NONEin thenextOfTypewrapper.
LibCorporateActionNode.nextOfTypenow returnsNODE_NONEon “no match”, so the oldnextCursor != 0guard still takes the branch at end-of-list and dereferencesnodes[NODE_NONE]. That turns a valid “no next action” read into a revert in the invariant harness’s receipt-side traversal path.Suggested fix
{ nextCursor = LibCorporateActionNode.nextOfType(cursor, mask, filter); - if (nextCursor != 0) { + if (nextCursor != NODE_NONE) { CorporateActionNode storage node = LibCorporateAction.getStorage().nodes[nextCursor]; actionType = node.actionType; effectiveTime = node.effectiveTime; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/concrete/StoxCorporateActionsInvariant.t.sol` around lines 117 - 127, The wrapper function nextOfType should check for the sentinel NODE_NONE returned by LibCorporateActionNode.nextOfType instead of comparing to 0; update the guard in nextOfType to skip dereferencing LibCorporateAction.getStorage().nodes[nextCursor] when nextCursor == NODE_NONE so you don't read nodes[NODE_NONE] and cause a revert. Reference the wrapper function nextOfType, the call to LibCorporateActionNode.nextOfType, the NODE_NONE sentinel, and the storage access LibCorporateAction.getStorage().nodes to locate and fix the check.
♻️ Duplicate comments (1)
test/src/lib/LibCorporateActionNode.t.sol (1)
745-746:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
NODE_NONEconsistently in fuzz no-match checks/invariants.Line 745/746, Line 790, Line 831, and Line 887 still treat
0as “not none”, and Line 932 only short-circuits oncursor == 0. After the sentinel migration, this can both mask failures (NODE_NONE != 0) and trigger invalidnodeAt(NODE_NONE)reads in invariant checks.Suggested patch
@@ - assertTrue(earliestCursor != 0, "earliest must find an existing match"); - assertTrue(latestCursor != 0, "latest must find an existing match"); + assertTrue(earliestCursor != NODE_NONE, "earliest must find an existing match"); + assertTrue(latestCursor != NODE_NONE, "latest must find an existing match"); @@ - if (cursor == 0) { - assertEq(latestCursor, NODE_NONE, "earliest 0 implies latest 0"); + if (cursor == NODE_NONE) { + assertEq(latestCursor, NODE_NONE, "earliest NONE implies latest NONE"); continue; } @@ - assertTrue(cursor != 0, "forward walk hit 0 before reaching latest"); + assertTrue(cursor != NODE_NONE, "forward walk hit NONE before reaching latest"); @@ - if (cursor == 0) { - assertEq(earliestCursor, NODE_NONE, "latest 0 implies earliest 0"); + if (cursor == NODE_NONE) { + assertEq(earliestCursor, NODE_NONE, "latest NONE implies earliest NONE"); continue; } @@ - assertTrue(cursor != 0, "backward walk hit 0 before reaching earliest"); + assertTrue(cursor != NODE_NONE, "backward walk hit NONE before reaching earliest"); @@ - assertTrue(earliestCursor != 0, "earliest must find a non-cancelled match"); + assertTrue(earliestCursor != NODE_NONE, "earliest must find a non-cancelled match"); @@ - if (cursor == 0) return; + if (cursor == NODE_NONE) return;Also applies to: 781-783, 790-790, 822-824, 831-831, 887-890, 931-933
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/LibCorporateActionNode.t.sol` around lines 745 - 746, The fuzz/no-match invariant checks currently compare cursor values to literal 0 (e.g., assertions using earliestCursor != 0 and latestCursor != 0 and a short-circuit check of cursor == 0) which is incorrect after the sentinel migration; replace all such 0 comparisons with the sentinel constant NODE_NONE and ensure any short-circuits (where code avoids nodeAt) use cursor == NODE_NONE so you don't call nodeAt(NODE_NONE); update all occurrences listed (around assertions at earliestCursor/latestCursor, the short-circuit at cursor check, and the invariant blocks that read nodeAt) to use NODE_NONE consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/src/concrete/StoxCorporateActionsFacet.t.sol`:
- Around line 1600-1606: The docstring above
testBootstrapNodePropertiesAfterFirstSchedule is off-by-one: change the text to
state that the bootstrap node lands at index 0 (with ACTION_TYPE_INIT_V1,
effectiveTime = block.timestamp, and empty parameters) and that the first
user-scheduled action (created via corporateActionHarness.schedule with
ACTION_TYPE_STOCK_SPLIT_V1) lands at index 1 to match the assertEq(userId, 1)
check; apply the same wording correction to the other comment block around the
test near the 1746-1749 region so all prose matches the assertions and removes
index/cursor ambiguity.
- Around line 1016-1022: The test testCancelAtNodesLengthReverts currently calls
corporateActionHarness.cancel(3) which doesn't exercise the exact boundary index
== nodes.length after one schedule (nodes length is 2: bootstrap at 0 and user
at 1); update the expectation and call to use index 2 instead: change
vm.expectRevert(abi.encodeWithSelector(ActionDoesNotExist.selector, uint256(3)))
to expect uint256(2) and call corporateActionHarness.cancel(2), and update the
test comment/docstring to state it checks index == nodes.length for clarity.
In `@test/src/concrete/StoxReceipt.t.sol`:
- Around line 86-88: The branch that handles cursor == NODE_NONE currently
returns non-zero action metadata (ACTION_TYPE_STOCK_SPLIT_V1, 1) even when
splits.length == 0; change it to return zeroed metadata when there are no more
nodes (return (NODE_NONE, 0, 0) or use the codebase's ACTION_TYPE_NONE and 0 for
effectiveTime) so nextCursor == NODE_NONE implies no action; make the same
change in the other empty-list branch at the same logic point (the places
referencing cursor, splits, NODE_NONE, and ACTION_TYPE_STOCK_SPLIT_V1).
In `@test/src/concrete/StoxReceiptVault.t.sol`:
- Around line 842-843: Update the stale inline comments that describe the
bootstrap/pot indexing to reflect the new cursor semantics: replace any mentions
of "bootstrap == 1", "idx 1" or "pot 4" with "bootstrap == 0", "idx 0" and "pot
3" respectively so the comments match the assertions that pin bootstrap==0 and
final pot 3; apply the same change to the other occurrences called out in the
review (the other comment blocks in the same test file). Ensure the wording
describes that both splits land in one fold call, both events fire, and
bootstrap takes idx 0 so the splits are at idx 2 and 3 (and mirror this
corrected numbering wherever the old model was referenced).
In `@test/src/concrete/StoxReceiptVaultFallbackRouting.t.sol`:
- Around line 178-187: The test only asserts cursor and effectiveTime from the
routed ICorporateActionsV1.nextOfType and .prevOfType calls but omits asserting
the middle return value actionType; update the assertions in the block that
calls nextOfType and prevOfType (variables cursor, actionType, effectiveTime) to
also assert that actionType == ACTION_TYPE_STOCK_SPLIT_V1 so the test validates
the full routed tuple and will catch any ABI-decoding/regression that corrupts
the middle return value.
---
Outside diff comments:
In `@test/src/concrete/StoxCorporateActionsInvariant.t.sol`:
- Around line 117-127: The wrapper function nextOfType should check for the
sentinel NODE_NONE returned by LibCorporateActionNode.nextOfType instead of
comparing to 0; update the guard in nextOfType to skip dereferencing
LibCorporateAction.getStorage().nodes[nextCursor] when nextCursor == NODE_NONE
so you don't read nodes[NODE_NONE] and cause a revert. Reference the wrapper
function nextOfType, the call to LibCorporateActionNode.nextOfType, the
NODE_NONE sentinel, and the storage access LibCorporateAction.getStorage().nodes
to locate and fix the check.
---
Duplicate comments:
In `@test/src/lib/LibCorporateActionNode.t.sol`:
- Around line 745-746: The fuzz/no-match invariant checks currently compare
cursor values to literal 0 (e.g., assertions using earliestCursor != 0 and
latestCursor != 0 and a short-circuit check of cursor == 0) which is incorrect
after the sentinel migration; replace all such 0 comparisons with the sentinel
constant NODE_NONE and ensure any short-circuits (where code avoids nodeAt) use
cursor == NODE_NONE so you don't call nodeAt(NODE_NONE); update all occurrences
listed (around assertions at earliestCursor/latestCursor, the short-circuit at
cursor check, and the invariant blocks that read nodeAt) to use NODE_NONE
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91d94912-11fd-49ba-8959-98f413ac169f
⛔ Files ignored due to path filters (5)
src/generated/StoxCorporateActionsFacet.pointers.solis excluded by!**/generated/**src/generated/StoxOffchainAssetReceiptVaultBeaconSetDeployer.pointers.solis excluded by!**/generated/**src/generated/StoxReceipt.pointers.solis excluded by!**/generated/**src/generated/StoxReceiptVault.pointers.solis excluded by!**/generated/**src/generated/StoxUnifiedDeployer.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (6)
test/src/concrete/StoxCorporateActionsFacet.t.soltest/src/concrete/StoxCorporateActionsInvariant.t.soltest/src/concrete/StoxReceipt.t.soltest/src/concrete/StoxReceiptVault.t.soltest/src/concrete/StoxReceiptVaultFallbackRouting.t.soltest/src/lib/LibCorporateActionNode.t.sol
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The custom nextOfType wrapper in InvariantVault used to gate the metadata read on != 0; under the 0-based scheme that test would return (0, 0, 0) for bootstrap (a valid match) and OOB-read at NODE_NONE (uint256.max) on no-match. Fix gates on != NODE_NONE. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/src/concrete/StoxCorporateActionsInvariant.t.sol (1)
117-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor the new
NODE_NONEsentinel innextOfType.Line 123 still uses the old
0sentinel. WhenLibCorporateActionNode.nextOfTypereturnsNODE_NONE, this view will indexnodes[NODE_NONE]and revert instead of returning the sentinel, which breaks the harness’sICorporateActionsV1read surface on traversal exhaustion.Suggested fix
{ nextCursor = LibCorporateActionNode.nextOfType(cursor, mask, filter); - if (nextCursor != 0) { + if (nextCursor != NODE_NONE) { CorporateActionNode storage node = LibCorporateAction.getStorage().nodes[nextCursor]; actionType = node.actionType; effectiveTime = node.effectiveTime; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/concrete/StoxCorporateActionsInvariant.t.sol` around lines 117 - 127, The view nextOfType currently checks nextCursor != 0 before indexing into LibCorporateAction.getStorage().nodes, but LibCorporateActionNode.nextOfType now returns the sentinel NODE_NONE instead of 0; update the conditional to compare against LibCorporateActionNode.NODE_NONE (or the appropriate exported constant) and only read CorporateActionNode storage and populate actionType/effectiveTime when nextCursor != NODE_NONE to avoid indexing nodes[NODE_NONE] and reverting.
♻️ Duplicate comments (4)
src/concrete/StoxReceiptVault.sol (1)
132-143:⚠️ Potential issue | 🟠 MajorBump the production facet pointer before merging.
This
_updatepath now depends on the new bootstrap/init-node behavior, but the vault still delegates throughLibProdDeployV3. If that address bump/regeneration is skipped, production vaults will keep executing the pre-bootstrap facet and this cursor flow will never go live.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/concrete/StoxReceiptVault.sol` around lines 132 - 143, The _update path in StoxReceiptVault now relies on the new bootstrap/init-node behavior but the vault still delegates through the old production facet pointer in LibProdDeployV3, so if that pointer isn't bumped/regenerated the new cursor logic (s.totalSupplyLatestCursor, LibTotalSupply.fold and _emitNewlyEffectiveSplits) will never run; fix this by updating the production facet pointer used by the vault (regenerate/bump the facet address in LibProdDeployV3 or update the delegate target to the new facet) before merging so that StoxReceiptVault._update delegates to the new bootstrap-aware facet and the cursor flow goes live.test/src/concrete/StoxReceiptVault.t.sol (1)
842-843:⚠️ Potential issue | 🟡 MinorCorrect the remaining bootstrap/pot index comments.
These blocks still say
bootstrap == 1orpot 4, while the code and assertions here pinbootstrap == 0and the final pot at3.Also applies to: 940-940, 1315-1317, 1457-1457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/concrete/StoxReceiptVault.t.sol` around lines 842 - 843, Update the incorrect inline comments that state "bootstrap == 1" and "pot 4" to reflect the actual test state: change them to "bootstrap == 0" and "pot 3". Search the test file for occurrences of the strings "bootstrap == 1" and "pot 4" (including the blocks around the split/fold assertions and the comments at the locations noted in the review) and replace the text so the comments match the code and assertions that use bootstrap==0 and final pot index 3.test/src/concrete/StoxCorporateActionsFacet.t.sol (2)
1023-1029:⚠️ Potential issue | 🟡 MinorThis still misses the exact
index == nodes.lengthboundary.After one schedule, the array is
{bootstrap, user}, sonodes.length == 2.cancel(3)only proves a generic out-of-range revert; it does not pin the boundary this test claims to cover.Suggested fix
- /// Cancel at index == nodes.length reverts. After one schedule the - /// nodes array has length 3 (sentinel + bootstrap + user node), so - /// idx 3 does not exist. + /// Cancel at index == nodes.length reverts. After one schedule the + /// nodes array has length 2 (bootstrap + user node), so idx 2 is the + /// first out-of-bounds index. function testCancelAtNodesLengthReverts() external { corporateActionHarness.schedule(ACTION_TYPE_STOCK_SPLIT_V1, 1500, ""); - vm.expectRevert(abi.encodeWithSelector(ActionDoesNotExist.selector, uint256(3))); - corporateActionHarness.cancel(3); + vm.expectRevert(abi.encodeWithSelector(ActionDoesNotExist.selector, uint256(2))); + corporateActionHarness.cancel(2); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/concrete/StoxCorporateActionsFacet.t.sol` around lines 1023 - 1029, The test claims to assert the boundary "index == nodes.length" but currently calls cancel(3) which is an unrelated out-of-range index; after one schedule the nodes array length is 2, so change the test to call cancel with the actual nodes.length boundary (use uint256(2) or, if available, read a nodesLength accessor) and expect ActionDoesNotExist(uint256(nodes.length)); update the vm.expectRevert call to match that exact index and keep the same corporateActionHarness.schedule(...) and corporateActionHarness.cancel(...) calls.
471-472:⚠️ Potential issue | 🟡 MinorFix the remaining off-by-one bootstrap prose.
These comments still describe the bootstrap as
idx 1, but the assertions in the same tests pin bootstrap atidx 0and the first user action atidx 1.Also applies to: 1177-1179, 1607-1609, 1755-1757
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/concrete/StoxCorporateActionsFacet.t.sol` around lines 471 - 472, The inline comments that say "Bootstrap (idx 1)" are incorrect — the test assertions treat bootstrap as idx 0 and the first user action as idx 1; update those comments (search for the exact phrase "Bootstrap (idx 1)" and nearby prose) to say "Bootstrap (idx 0)" and adjust the surrounding wording to state that cancelling/unlinking removes the user action at idx 1 while bootstrap remains the head at idx 0; make this same change for the other occurrences of the same comment text in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/src/concrete/StoxCorporateActionsInvariant.t.sol`:
- Around line 467-472: When vault.nodesLength() > 0 ensure list roots are real
before traversing: add an assertion that both head and tail returned by
vault.listHead() and vault.listTail() are not NODE_NONE (or the equivalent
sentinel) prior to performing the walks in the invariant check so that invariant
1 fails if the roots are detached; update the block that reads uint256 head =
vault.listHead(); uint256 tail = vault.listTail(); to assert head != NODE_NONE
&& tail != NODE_NONE when nodesLength() > 0.
---
Outside diff comments:
In `@test/src/concrete/StoxCorporateActionsInvariant.t.sol`:
- Around line 117-127: The view nextOfType currently checks nextCursor != 0
before indexing into LibCorporateAction.getStorage().nodes, but
LibCorporateActionNode.nextOfType now returns the sentinel NODE_NONE instead of
0; update the conditional to compare against LibCorporateActionNode.NODE_NONE
(or the appropriate exported constant) and only read CorporateActionNode storage
and populate actionType/effectiveTime when nextCursor != NODE_NONE to avoid
indexing nodes[NODE_NONE] and reverting.
---
Duplicate comments:
In `@src/concrete/StoxReceiptVault.sol`:
- Around line 132-143: The _update path in StoxReceiptVault now relies on the
new bootstrap/init-node behavior but the vault still delegates through the old
production facet pointer in LibProdDeployV3, so if that pointer isn't
bumped/regenerated the new cursor logic (s.totalSupplyLatestCursor,
LibTotalSupply.fold and _emitNewlyEffectiveSplits) will never run; fix this by
updating the production facet pointer used by the vault (regenerate/bump the
facet address in LibProdDeployV3 or update the delegate target to the new facet)
before merging so that StoxReceiptVault._update delegates to the new
bootstrap-aware facet and the cursor flow goes live.
In `@test/src/concrete/StoxCorporateActionsFacet.t.sol`:
- Around line 1023-1029: The test claims to assert the boundary "index ==
nodes.length" but currently calls cancel(3) which is an unrelated out-of-range
index; after one schedule the nodes array length is 2, so change the test to
call cancel with the actual nodes.length boundary (use uint256(2) or, if
available, read a nodesLength accessor) and expect
ActionDoesNotExist(uint256(nodes.length)); update the vm.expectRevert call to
match that exact index and keep the same corporateActionHarness.schedule(...)
and corporateActionHarness.cancel(...) calls.
- Around line 471-472: The inline comments that say "Bootstrap (idx 1)" are
incorrect — the test assertions treat bootstrap as idx 0 and the first user
action as idx 1; update those comments (search for the exact phrase "Bootstrap
(idx 1)" and nearby prose) to say "Bootstrap (idx 0)" and adjust the surrounding
wording to state that cancelling/unlinking removes the user action at idx 1
while bootstrap remains the head at idx 0; make this same change for the other
occurrences of the same comment text in the file.
In `@test/src/concrete/StoxReceiptVault.t.sol`:
- Around line 842-843: Update the incorrect inline comments that state
"bootstrap == 1" and "pot 4" to reflect the actual test state: change them to
"bootstrap == 0" and "pot 3". Search the test file for occurrences of the
strings "bootstrap == 1" and "pot 4" (including the blocks around the split/fold
assertions and the comments at the locations noted in the review) and replace
the text so the comments match the code and assertions that use bootstrap==0 and
final pot index 3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6fbfb06-a282-47de-95d2-f4c90902e877
📒 Files selected for processing (6)
src/concrete/StoxCorporateActionsFacet.solsrc/concrete/StoxReceiptVault.soltest/src/concrete/StoxCorporateActionsFacet.t.soltest/src/concrete/StoxCorporateActionsInvariant.t.soltest/src/concrete/StoxReceiptVault.t.soltest/src/lib/LibCorporateActionNode.t.sol
…l-sentinel-uint-max # Conflicts: # src/concrete/StoxReceiptVault.sol # src/generated/StoxOffchainAssetReceiptVaultBeaconSetDeployer.pointers.sol # src/generated/StoxReceiptVault.pointers.sol # src/generated/StoxUnifiedDeployer.pointers.sol # src/interface/ICorporateActionsV1.sol # test/src/concrete/StoxCorporateActionsInvariant.t.sol # test/src/concrete/StoxReceiptVault.t.sol
The pointer regen ran on stale out and cache artifacts in earlier passes. After rm -rf out cache and fresh build, pointer hashes for StoxReceiptVault and StoxOffchainAssetReceiptVaultBeaconSetDeployer landed at the values CI computes from the merged source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…NONE check Six tests added, mutation-verified where the underlying claim was falsifiable: - testGetActionParametersBootstrapReturnsEmpty: cursor 0 returns empty bytes (mutation: re-add `cursor == 0` rejection -> caught). - testGetActionParametersNodeNoneReverts: NODE_NONE reverts. Mutation removing the explicit `cursor == NODE_NONE ||` arm was NOT caught — bounds check `cursor >= s.nodes.length` already covers any value approaching `type(uint256).max`. Dropped the redundant check from source; test re-pins the bounds-derived behaviour. - testCancelOnlyNodeLeavesEmptyList: bootstrap.prev/next pinned at NODE_NONE post-cancel (mutation: comment out the writes -> caught). - testEnsureBootstrapInitsTotalSupplyLatestCursorToNodeNone: post-bootstrap totalSupplyLatestCursor is NODE_NONE, distinct from default 0 (mutation: comment out the write -> caught). - testNextOfTypeHeadInclusiveReturnsBootstrap: cross-contract head-inclusive walk via fallback delegatecall returns bootstrap idx 0 (mutation: rewrite NODE_NONE -> s.head as NODE_NONE -> s.nodes[s.head].next so the head is skipped -> caught). - testMigrationCursorDefaultsToBootstrapForFreshAccount / testReceiptCursorDefaultsToBootstrapForFreshPair: pin Solidity's mapping default = 0 = bootstrap. Not source-mutable; defends against namespace-base / mapping-shape drift on the storage slot. Closes #130, #131, #132, #133, #134, #135. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- testEnsureBootstrapDoesNotEmitScheduledEvent: vm.recordLogs counts the events from a single scheduleCorporateAction call and asserts exactly one CorporateActionScheduled fires. Mutation: add a phantom emit alongside the real one; caught. - testCountCompletedExcludesBootstrap: comments updated for 0-based layout; mutation flipping countCompleted's mask to include ACTION_TYPE_INIT_V1; caught. - testFuzzHeadStaysAtBootstrapAcrossScheduleCancelMix: schedules N user actions and cancels a mask-driven subset, asserting head==0 after every step. Mutation: schedule writes s.head = actionIndex unconditionally; caught. - testCancelPendingDoesNotRewindFoldedCursor: schedule + complete + fold, then schedule a future split and cancel it; assert totalSupplyLatestCursor unchanged BEFORE re-folding (a post-fold assertion would be invisible to the mutation since fold re-derives the value). Mutation: cancel writes totalSupplyLatestCursor = NODE_NONE; caught. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/interface/ICorporateActionsV1.sol`:
- Around line 19-44: The published V1 action-type bit values must not change;
move ACTION_TYPE_INIT_V1 off the existing V1 bits onto a new unused bit and
leave ACTION_TYPE_STOCK_SPLIT_V1 and ACTION_TYPE_STABLES_DIVIDEND_V1 exactly as
they are. Update BALANCE_MIGRATION_TYPES_MASK to include the new
ACTION_TYPE_INIT_V1 value (and keep ACTION_TYPE_STOCK_SPLIT_V1 included) and
ensure VALID_ACTION_TYPES_MASK still unions the three V1 symbols
(ACTION_TYPE_INIT_V1, ACTION_TYPE_STOCK_SPLIT_V1,
ACTION_TYPE_STABLES_DIVIDEND_V1) so external masks/events remain consistent. Use
the symbol names ACTION_TYPE_INIT_V1, ACTION_TYPE_STOCK_SPLIT_V1,
ACTION_TYPE_STABLES_DIVIDEND_V1, BALANCE_MIGRATION_TYPES_MASK, and
VALID_ACTION_TYPES_MASK to locate and change the definitions.
In `@test/src/concrete/StoxCorporateActionsFacet.t.sol`:
- Around line 1446-1454: Update the stale comment to correctly describe
behavior: replace the incorrect claim that getActionParameters(0) reverts with a
statement that the bootstrap cursor 0 is valid/returns empty (as asserted by
testGetActionParametersBootstrapReturnsEmpty) and that the invalid sentinel
NODE_NONE (type(uint256).max) is the one that causes ActionDoesNotExist to be
hit via the bounds check on cursor >= s.nodes.length; reference
getActionParameters, NODE_NONE, cursor, and s.nodes.length in the comment to
make the distinction explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7455c950-087f-4e7d-b39a-3a03879c4bc2
⛔ Files ignored due to path filters (3)
src/generated/StoxOffchainAssetReceiptVaultBeaconSetDeployer.pointers.solis excluded by!**/generated/**src/generated/StoxReceiptVault.pointers.solis excluded by!**/generated/**src/generated/StoxUnifiedDeployer.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (9)
src/concrete/StoxCorporateActionsFacet.solsrc/concrete/StoxReceiptVault.solsrc/interface/ICorporateActionsV1.soltest/src/concrete/StoxCorporateActionsFacet.t.soltest/src/concrete/StoxCorporateActionsInvariant.t.soltest/src/concrete/StoxReceipt.t.soltest/src/concrete/StoxReceiptVault.t.soltest/src/concrete/StoxReceiptVaultFallbackRouting.t.soltest/src/lib/LibTotalSupply.t.sol
- testBootstrapEffectiveTimeStrictlyLessThanUserActions: pin bootstrap.effectiveTime < every user action's effectiveTime — the precondition that makes _insertOrdered's tail-walk always terminate at bootstrap. Mutation: set bootstrap.effectiveTime = block.timestamp + 1; caught (user action with block.timestamp + 1 ties). - testMigrateAccountNoOpWhenAtLatest: vm.recordLogs + storage diff asserts a fresh-holder touch when no completed splits exist past bootstrap is a true no-op (no AccountMigrated, no SSTORE to cursor or balance). Mutation: drop the if (newCursor == currentCursor) return; early return; caught (event fired). - testInvariantVaultNextOfTypeGuardsOnNodeNone: focused unit test on the InvariantVault helper's != NODE_NONE guard. Pre-bootstrap query must return (NODE_NONE, 0, 0) cleanly. Mutation: revert the guard to != 0 (the pre-fix bug); caught via array OOB panic. Storage layout pin already covered totalSupplyLatestCursor at offset 5; mutation swapping its struct position; caught via offset-4 unmigrated read returning 0 instead of stored value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- testOnMintOnBurnZeroAreNoOps: explicit zero-amount no-op pin on the pot accounting. Mutation: replace the += accumulator with = assignment in onMint; caught (pot zeroed instead of preserved). - testEnsureBootstrapPrecedesUserActionInArrayOrder: the ordering inside schedule (ensureBootstrap before nodes.push) is the precondition for bootstrap landing at idx 0 and user actions at idx 1+. Mutation: swap the order so user push runs first; caught (ensureBootstrap then no-ops because length != 0, INIT-mask walk returns NODE_NONE). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Source last changed under d7f86df / 20c355a (0-based-refactor cleanup). Pointer file in HEAD reflected an older bytecode, breaking the LibProdDeployV3 codehash + creation/runtime equality tests on CI. Hard-rebuild regen restores parity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- testGetActionParametersExactBoundaryReverts: pin the exact cursor == nodes.length boundary (off-by-one in `>=` would silently read past the array; mutation flips `>=` to `>` and the test fires) - invariantListIntegrity: head/tail must be != NODE_NONE when nodes non-empty (catches detached-roots regression) - testAllTraversalGettersRouteThroughFallback: assert actionType field on routed nextOfType return tuple - LibTotalSupply NatSpec: rewrite the bootstrap explanation in terms of "bootstrap at idx 0 / user actions at idx 1+" rather than the intermediate "index-1 init node + pre-init cursor 0" wording Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 1 of the pot-invariant inductive proof in LibTotalSupply.sol claims
fold writes only the cursor — no pot, no balance. That property was
implicitly tested ("unmigrated[0] still 1000 after fold") but never
asserted as a general invariant across multiple pots.
testFoldDoesNotMutateAnyPot:
- builds non-trivial pot state (pots 0/1/2 with distinct values)
- snapshots all four pots
- runs a fold that MUST advance the cursor (asserted)
- asserts every pre-existing pot is unchanged
Mutation-verified: `s.unmigrated[3] += 1` inside fold's terminal
branch fails this test cleanly with "fold must not mutate
unmigrated[3]: 2 != 1". Catches accidental pot writes anywhere
inside fold's body.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Coupling test verifying cancel + fold + effectiveTotalSupply remain consistent when a middle node is cancelled between the first and second folds. Specifically, after cancel: - the cursor must skip the cancelled node and land on the next live one - effectiveTotalSupply must not include the cancelled multiplier Mutation-verified: stripping the unlink block in cancel (so the cancelled node remains pointed-to by its neighbours) fails this test cleanly with "second fold skips cancelled B and lands on C: 2 != 3", because fold follows the still-intact next pointer to B and stops there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both traversal primitives validate the mask BEFORE the empty-list early return, so calling them with an invalid mask on an empty (pre-bootstrap) list reverts InvalidMask rather than silently returning NODE_NONE. Existing testMaskZeroReverts schedules an action first, so it never exercised the empty-list path. The new test covers it directly: empty list + mask=0 must still revert. Mutation-verified: swapping the two guards (length check first) makes this test fail with "next call did not revert as expected" — the mutation makes empty-list+mask=0 silently return NODE_NONE. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit pointed out the mock's NODE_NONE-cursor branch returned (NODE_NONE, ACTION_TYPE_STOCK_SPLIT_V1, 1) when splits.length == 0, contradicting the cursor-walked-past-end branch which returns (NODE_NONE, 0, 0). The inconsistency would let traversal tests pass against states the real vault never produces. Receipt-side rebase never passes NODE_NONE in production, so this branch is unreachable today — but locking the contract now closes the door on a future regression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit c80b568 regen'd only StoxCorporateActionsFacet pointers, but StoxReceiptVault embeds the facet address in its bytecode (via LibProdDeployV3.STOX_CORPORATE_ACTIONS_FACET in the fallback). When the facet address changes, the vault's bytecode changes, which cascades into: - StoxReceiptVault.pointers.sol (vault bytecode itself) - StoxOffchainAssetReceiptVaultBeaconSetDeployer.pointers.sol (deploys vault impls) - StoxUnifiedDeployer.pointers.sol (composes both) Multi-pass regen converges in 2 passes (pass 1 deploy fails because the deploy script reads stale pointers; pass 2+ stable). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ACTION_TYPE_INIT_V1(1<<7) and a real bootstrap node at idx 1, lazily created by_ensureBootstrapon the firstschedulecall. Index 0 stays a pure null sentinel — no more value-vs-position ambiguity (Linked list uses positional disambiguation instead of value-level disambiguation for null pointers #79).totalSupplyBootstrapped; renamestotalSupplyLatestSplit → totalSupplyLatestCursor._ensureBootstrapsnapshots OZ's_totalSupplyintounmigrated[0]at schedule time (OZ's invariant still holds at that point — schedule does not touch balances).LibRebase,LibReceiptRebase,LibTotalSupply.fold/effectiveTotalSupply) useBALANCE_MIGRATION_TYPES_MASK = INIT | SPLIT. Init is identity for splits; future action types decide independently whether to include the bit.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation