Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions .workflow/reports/SIG-746.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# Implementation Report — SIG-746

## Changes

Jira summary: Rename SignalsPositionStorage zombies to OZ standard pattern.

- `contracts/position/SignalsPositionStorage.sol`
- Renamed the four deprecated storage mappings at slots 3 through 6 to `__deprecated_slot_3` through `__deprecated_slot_6`.
- Replaced the legacy `// @deprecated` comments with `/// @custom:oz-renamed-from <oldName>` NatSpec annotations so the storage diff tooling can prove the label-only rename is intentional.
- Kept declaration order, mapping types, `internal` visibility, and `__gap[48]` unchanged.
- `storage-snapshots/SignalsPosition.json`
- Regenerated the committed `SignalsPosition` snapshot after the label rename.
- The meaningful snapshot change is the four slot labels. AST IDs and compiler-generated type identifiers changed as expected from fresh build output; slot, offset, and storage type semantics remain safe, as verified by `STORAGE_STRICT_BASE=1 yarn storage:check`.

## Design Decisions

I used one NatSpec line per storage declaration because the repository's annotation resolver reads Solidity AST documentation and matches `@custom:oz-renamed-from` directly on the renamed variable node. Keeping the annotation immediately above each mapping makes the old-to-new relationship unambiguous and avoids relying on surrounding prose.

I kept the old names only in the annotation and used slot-number placeholders for the new labels. The slot number is the stable invariant that matters for these retained zombie mappings, while the old semantic label is preserved where the storage safety tooling expects to find it.

I did not add or modify test files. The PM plan explicitly excluded test-code changes for this issue, and SIG-743 already contains focused comparator coverage for top-level label renames with and without OZ annotations. I used that existing harness plus strict base storage checking as the regression path.

## Known Tradeoffs

None. The implementation is intentionally limited to the storage labels and the matching snapshot.

## Test Coverage

- Pre-change harness check:
- `yarn storage:test` passed: 23 tests.
- `STORAGE_STRICT_BASE=1 yarn storage:check` passed against the original snapshot.
- Red-state check after contract rename and before snapshot regeneration:
- `STORAGE_STRICT_BASE=1 yarn storage:check` reported `SignalsPosition` as storage-safe relative to `origin/main` and failed only because `storage-snapshots/SignalsPosition.json` was stale.
- Final verification:
- `forge build` passed. It emitted existing repo-wide warnings unrelated to `SignalsPositionStorage.sol`.
- `STORAGE_STRICT_BASE=1 yarn storage:check` passed.
- `yarn storage:test` passed: 23 tests.
- `forge inspect contracts/position/SignalsPosition.sol:SignalsPosition storageLayout --json` confirmed slots 0 through 7 still map to `core`, `_nextId`, `_positions`, the four `__deprecated_slot_*` placeholders, and `__gap[48]`.
- `forge test` passed: 80 suites, 1079 tests.
- `yarn lint` passed.
- `git diff --check` passed.

## Escalation

None.
16 changes: 8 additions & 8 deletions contracts/position/SignalsPositionStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ abstract contract SignalsPositionStorage {
address public core;
uint256 internal _nextId;
mapping(uint256 => ISignalsPosition.Position) internal _positions;
// @deprecated No longer written to. Kept for storage layout compatibility (UUPS proxy).
mapping(uint256 => uint256[]) internal _marketTokenList;
// @deprecated No longer written to. Kept for storage layout compatibility (UUPS proxy).
mapping(uint256 => uint256) internal _positionMarketIndex;
// @deprecated No longer written to. Kept for storage layout compatibility (UUPS proxy).
mapping(address => uint256[]) internal _ownerTokenList;
// @deprecated No longer written to. Kept for storage layout compatibility (UUPS proxy).
mapping(uint256 => uint256) internal _positionOwnerIndex;
/// @custom:oz-renamed-from _marketTokenList
mapping(uint256 => uint256[]) internal __deprecated_slot_3;
/// @custom:oz-renamed-from _positionMarketIndex
mapping(uint256 => uint256) internal __deprecated_slot_4;
/// @custom:oz-renamed-from _ownerTokenList
mapping(address => uint256[]) internal __deprecated_slot_5;
/// @custom:oz-renamed-from _positionOwnerIndex
mapping(uint256 => uint256) internal __deprecated_slot_6;

// Reserve ample slots for future upgrades; do not change after first deployment.
uint256[48] internal __gap;
Expand Down
42 changes: 21 additions & 21 deletions storage-snapshots/SignalsPosition.json
Original file line number Diff line number Diff line change
@@ -1,63 +1,63 @@
{
"storage": [
{
"astId": 20029,
"astId": 19893,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "core",
"offset": 0,
"slot": "0",
"type": "t_address"
},
{
"astId": 20031,
"astId": 19895,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "_nextId",
"offset": 0,
"slot": "1",
"type": "t_uint256"
},
{
"astId": 20036,
"astId": 19900,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "_positions",
"offset": 0,
"slot": "2",
"type": "t_mapping(t_uint256,t_struct(Position)5512_storage)"
"type": "t_mapping(t_uint256,t_struct(Position)5446_storage)"
},
{
"astId": 20041,
"astId": 19906,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "_marketTokenList",
"label": "__deprecated_slot_3",
"offset": 0,
"slot": "3",
"type": "t_mapping(t_uint256,t_array(t_uint256)dyn_storage)"
},
{
"astId": 20045,
"astId": 19911,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "_positionMarketIndex",
"label": "__deprecated_slot_4",
"offset": 0,
"slot": "4",
"type": "t_mapping(t_uint256,t_uint256)"
},
{
"astId": 20050,
"astId": 19917,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "_ownerTokenList",
"label": "__deprecated_slot_5",
"offset": 0,
"slot": "5",
"type": "t_mapping(t_address,t_array(t_uint256)dyn_storage)"
},
{
"astId": 20054,
"astId": 19922,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "_positionOwnerIndex",
"label": "__deprecated_slot_6",
"offset": 0,
"slot": "6",
"type": "t_mapping(t_uint256,t_uint256)"
},
{
"astId": 20058,
"astId": 19926,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "__gap",
"offset": 0,
Expand Down Expand Up @@ -102,12 +102,12 @@
"numberOfBytes": "32",
"value": "t_array(t_uint256)dyn_storage"
},
"t_mapping(t_uint256,t_struct(Position)5512_storage)": {
"t_mapping(t_uint256,t_struct(Position)5446_storage)": {
"encoding": "mapping",
"key": "t_uint256",
"label": "mapping(uint256 => struct ISignalsPosition.Position)",
"numberOfBytes": "32",
"value": "t_struct(Position)5512_storage"
"value": "t_struct(Position)5446_storage"
},
"t_mapping(t_uint256,t_uint256)": {
"encoding": "mapping",
Expand All @@ -116,45 +116,45 @@
"numberOfBytes": "32",
"value": "t_uint256"
},
"t_struct(Position)5512_storage": {
"t_struct(Position)5446_storage": {
"encoding": "inplace",
"label": "struct ISignalsPosition.Position",
"numberOfBytes": "128",
"members": [
{
"astId": 5503,
"astId": 5437,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "marketId",
"offset": 0,
"slot": "0",
"type": "t_uint256"
},
{
"astId": 5505,
"astId": 5439,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "lowerTick",
"offset": 0,
"slot": "1",
"type": "t_int256"
},
{
"astId": 5507,
"astId": 5441,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "upperTick",
"offset": 0,
"slot": "2",
"type": "t_int256"
},
{
"astId": 5509,
"astId": 5443,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "quantity",
"offset": 0,
"slot": "3",
"type": "t_uint128"
},
{
"astId": 5511,
"astId": 5445,
"contract": "contracts/position/SignalsPosition.sol:SignalsPosition",
"label": "createdAt",
"offset": 16,
Expand Down
Loading