SIG-746: rename SignalsPositionStorage zombies to OZ pattern#76
Merged
Merged
Conversation
There was a problem hiding this comment.
Summary
Renames four zombie storage mappings in SignalsPositionStorage.sol (slots 3-6) from their legacy semantic names to __deprecated_slot_<N>, replacing freeform // @deprecated comments with /// @custom:oz-renamed-from <oldName> NatSpec annotations. Regenerates the committed SignalsPosition.json snapshot to reflect the new labels.
Cross-PR Context
Recent Review Patterns
- PR#75 (SIG-743, storage layout CI tooling): the reviewed tooling is the infrastructure that validates this rename. That PR's comparator is what consumes the
@custom:oz-renamed-fromannotations added here. No issues from PR#75 carry over.
Sibling PRs (SIG-746)
- Only this PR. No cross-repo impact: the change is limited to storage variable labels with no ABI, event, or function signature modifications. Impact map confirms no downstream updates needed in
v1-subgraph,v1-sdk,v1-server, orsignals-app.
Issues
No issues found.
Verification performed:
- Slot integrity: Confirmed slots 0-7 preserve identical slot index, offset, and storage type between base and PR snapshots. Only the
labelfield changed on slots 3-6. - Mapping types preserved:
uint256 => uint256[],uint256 => uint256,address => uint256[],uint256 => uint256match exactly. - Annotation correctness: Each
@custom:oz-renamed-fromreferences the exact prior identifier (_marketTokenList,_positionMarketIndex,_ownerTokenList,_positionOwnerIndex). The annotations are placed as///NatSpec directly above each declaration, which compiles into the ASTdocumentationfield thatBuildInfoAnnotationResolver.hasRenameAnnotation()reads. - No stale references: Grepped the full codebase for the old variable names; they appear only in the base branch declarations (not called or accessed anywhere in contract logic).
__gap[48]unchanged: Gap array at slot 7 is untouched in both size and position.- Snapshot diff semantics: AST IDs and
t_struct(Position)NNNN_storagetype identifiers changed (expected from fresh build output). No entries added or removed. Slot, offset, and type semantics are preserved across all 8 storage entries and all 5 struct members. - No other zombies:
SignalsCoreStorage.solandSignalsLPShare.solhave no deprecated mappings requiring similar treatment. - Test coverage: The existing storage layout comparator tests (23 tests in
storage-layout.test.ts) cover rename annotation validation, including positive and negative cases for@custom:oz-renamed-from.
Suggestions
None.
Verdict
APPROVE
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
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.
Context
SIG-746.
SignalsPositionStorage.solkeeps four legacy mappings as zombies on slots 3-6 to preserve UUPS proxy storage layout. Their previous form used freeform// @deprecatedline comments and retained semantic names (_marketTokenList,_positionMarketIndex,_ownerTokenList,_positionOwnerIndex), which is not the OpenZeppelin upgrade-safe pattern that auditors and the OZ upgrades plugin recognize for an intentional rename.This change converts those four declarations to the OZ-standard rename pattern and regenerates the committed storage snapshot.
Decisions
__deprecated_slot_<N>matching its current slot index, and added a/// @custom:oz-renamed-from <oldName>NatSpec line directly above each declaration. Slot index, mapping type,internalvisibility, declaration order, and the trailinguint256[48] __gapare unchanged.@custom:oz-renamed-fromagainst the renamed variable's AST documentation node, so co-locating the annotation with the variable is what the tool actually consumes.STORAGE_STRICT_BASE=1 yarn storage:checkagainstorigin/main) plusforge inspect ... storageLayout.storage-snapshots/SignalsPosition.jsonbecause the snapshot is committed and the fourlabelfields move with the rename. AST IDs and the compiler-generatedt_struct(Position)<id>_storagetype identifier also shift, since the snapshot is produced from a fresh build of the full contract tree.Risk Areas
contracts/position/SignalsPositionStorage.solis the storage parent of the liveSignalsPositionUUPS proxy. Any change to slot index, type, offset, or order on slots 0-6 or to__gap[48]would corrupt every existing position. Reviewers should confirm slots 3-6 still hold the same mapping types and that slot 0 (core), slot 1 (_nextId), slot 2 (_positions), and slot 7 onward (__gap) are untouched.storage-snapshots/SignalsPosition.jsonis the artifact compared byyarn storage:check. The diff includes label changes on slots 3-6 plus AST ID andt_struct(Position)<id>_storageidentifier renumbering. Reviewers should confirm slot, offset, and storage-type semantics on every entry are preserved and that no entry was added or removed.@custom:oz-renamed-fromannotation is what the OZ upgrades plugin and the in-repo storage diff tool use to authorize a label-only rename. Each annotation must point to the exact prior identifier, since a typo there would cause future upgrade-safety checks to flag the slot as a removed variable.