[#4] chainPlot()

[realproject7]

Summary

Implements chainPlot(storylineId, contentCID, contentHash) with all four validations:

1. msg.sender == writer (access control)
2. CID length 46-100 bytes (format guard)
3. !sunset (not sunset)
4. block.timestamp <= lastPlotTime + 72 hours (deadline, if enabled)

Updates plotCount and lastPlotTime, emits PlotChained.

Fixes #4

Test plan

• forge build succeeds
• Unit tests (contracts#6)

🤖 Generated with Claude Code

[project7-interns]

Verdict: REQUEST CHANGES

Summary

The function includes the four requested validation gates, but the current plotCount update/emission sequence produces the wrong plotIndex after the genesis plot. That breaks the event stream the indexer relies on.

Findings

• [high] createStoryline() stores plotCount = 1 for the genesis plot and emits PlotChained(..., 0, ...), but chainPlot() then increments plotCount before emitting and uses the incremented total as plotIndex. The first chained plot therefore emits plotIndex = 2 instead of 1, skipping index 1 entirely and desynchronizing contract events from the off-chain plots table/indexer assumptions.
  • File: src/StoryFactory.sol:158
  • Suggestion: emit the next plot index as the pre-increment count (or store a separate total/index convention consistently). For the current data model, with genesis at index 0 and plotCount = 1, the first chainPlot() call should emit plotIndex = 1.

Decision

Request changes because the current event output would create an incorrect plot index sequence on-chain, which is a behavioral bug rather than a cosmetic issue.

[project7-interns]

T2b Review — REQUEST CHANGES

Bug: Off-by-one in emitted plotIndex

s.plotCount++;
s.lastPlotTime = block.timestamp;

emit PlotChained(storylineId, s.plotCount, msg.sender, contentCID, contentHash);

In createStoryline, the genesis plot emits PlotChained(..., 0, ...) and sets plotCount = 1. When chainPlot runs the first time, plotCount increments from 1 to 2, then the event emits plotIndex = 2. Index 1 is skipped. Every subsequent plot index is off by one.

Fix — capture the index before incrementing:

uint256 plotIndex = s.plotCount;
s.plotCount++;
s.lastPlotTime = block.timestamp;

emit PlotChained(storylineId, plotIndex, msg.sender, contentCID, contentHash);

This gives genesis = 0, first chain = 1, second chain = 2, etc.

Minor: No guard for non-existent storylineId

If storylineId doesn't exist (0, or > storylineCount), the storage read returns a zero-initialized struct (writer == address(0)). The require(msg.sender == s.writer) will revert, but with the misleading message "Not writer" instead of "Storyline does not exist". Consider adding an explicit existence check:

require(s.writer != address(0), "Storyline does not exist");

Validation checklist

#	Check	Result
1	Only writer can chain	PASS
2	CID length 46-100	PASS
3	Sunset check	PASS
4	Deadline check	PASS
5	State updates (plotCount++, lastPlotTime)	PASS
6	Event emitted with correct values	FAIL — off-by-one
7	Non-existent storylineId guard	MINOR — works but misleading error

The off-by-one is a real bug that will produce incorrect on-chain event logs and break any indexer or frontend relying on sequential plot indices. Please fix before re-requesting review.

[project7-interns]

T2b Review: APPROVE

Off-by-one fix confirmed — plotIndex = s.plotCount before s.plotCount++. Genesis=0, first chain=1. All four validations correct.

[project7-interns]

Verdict: APPROVE

Summary

The off-by-one bug in chainPlot() is fixed: the emitted plotIndex now uses the pre-increment count, so genesis remains index 0 and the first chained plot emits index 1. Foundry CI is green on the updated head commit.

Findings

• None.

Decision

Approve because the plot-index sequencing blocker is resolved and the PR now satisfies the merge criteria for issue #4.