[#3] createStoryline()#11
Conversation
Creates storyline token on MCV2_Bond with Mintpad J-curve params
(set at deployment), transfers creator role to writer for direct
royalty claiming, stores Storyline struct, and emits StorylineCreated
+ PlotChained (genesis plot_index=0) events. CID length validated
(46-100 bytes). Token symbol auto-generated as PLOT-{id}.
Fixes #3
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The PR matches issue #3: createStoryline() creates the Mint Club token with constructor-configured curve params, transfers the creator role to the writer, stores the storyline metadata, and emits both StorylineCreated and genesis PlotChained. The configurable step arrays also fit the issue's placeholder/TBD guidance, and Foundry CI is passing.
Findings
- None.
Decision
Approve because the implementation follows the specified flow, keeps the curve configuration pragmatic for this stage, and satisfies the current merge criteria for createStoryline().
project7-interns
left a comment
There was a problem hiding this comment.
T2b Review — [#3] createStoryline()
Verdict: REQUEST CHANGES — 2 issues (1 must-fix, 1 should-fix), plus minor observations.
MUST FIX
1. No title validation — empty title creates a nameless token
createStoryline validates openingCID length but never checks that title is non-empty. An empty title would pass through to TokenParams.name, creating a bonding-curve token with an empty name. This is a permanent, immutable artefact on MCV2_Bond — there is no rename function.
// Missing:
require(bytes(title).length > 0, "Empty title");Add a require(bytes(title).length > 0) at the top of the function.
SHOULD FIX
2. stepRanges and stepPrices are public mutable storage arrays — no setter guard needed, but storage cost is high and they are copied every call
stepRanges and stepPrices are non-immutable storage arrays (correctly — immutable is not supported for dynamic arrays). However, every createStoryline call copies the full arrays from storage into memory for the BondParams struct. With the 500-step J-curve from the proposal (§3.5), that is 500 × 2 SLOTs = 1000 cold SLOADs on the first call per block (~2.1M gas just for the reads).
This is acceptable if understood, but worth a NatSpec comment on the gas implication. More importantly: since these are plain public state variables with no write protection, any future function that accidentally writes to them would corrupt all subsequent storylines. Consider adding a comment or, better, making the constructor the only writer by convention (which it already is — just document the intent).
Not blocking, but the gas cost should be noted for awareness.
Observations (non-blocking)
A. Reentrancy on external BOND calls
BOND.createToken and BOND.updateBondCreator are external calls to MCV2_Bond. The state mutation (storylines[storylineId] = ...) happens after both calls, which is correct checks-effects-interactions order. No reentrancy concern here — good.
B. storylineCount starts at 1 (pre-increment)
storylineId = ++storylineCount means the first storyline ID is 1, not 0. This is fine and arguably better (0 can serve as sentinel), but worth noting for front-end integration.
C. Token symbol length
Symbol is "PLOT-" + uint2str(storylineId). At storylineId = 99999, symbol is 10 chars. MCV2_Bond on Base does not enforce a symbol length limit, so this is fine. _uint2str helper is correct.
D. openingHash is not validated against openingCID
This is by design — on-chain verification of IPFS content is not feasible. The hash serves as a commitment for off-chain verification. Correct approach.
E. hasDeadline / sunset semantics
sunset defaults to false, plotCount to 1, lastPlotTime to block.timestamp. All correct for genesis state.
F. Events
Both StorylineCreated and PlotChained(storylineId, 0, ...) are emitted. PlotChained with plotIndex=0 for genesis matches the spec. All fields present. Good.
G. Constructor validation
Step array length match and non-empty checks are present. _maxSupply is not validated (could be 0), but MCV2_Bond itself would reject a 0 maxSupply, so defense-in-depth is optional.
H. Access control
createStoryline is open to anyone (external, no modifier). This matches the spec — any address can create a storyline.
Summary
| # | Severity | Item |
|---|---|---|
| 1 | Must fix | Add require(bytes(title).length > 0) |
| 2 | Should fix | Document gas cost of 500-step array copies; consider NatSpec |
Overall structure is clean and matches the issue spec well. Fix the title validation and this is good to go.
Reject empty titles to prevent nameless tokens on MCV2_Bond. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
T2b Review: APPROVE
Title validation fix confirmed. Token creation, creator transfer, storage, events (StorylineCreated + genesis PlotChained), and CEI ordering all correct.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The follow-up fix addresses the remaining must-fix item: createStoryline() now rejects empty titles before creating the Mint Club token. Foundry CI remains green on the updated head commit.
Findings
- None.
Decision
Approve because the title-validation blocker is resolved and the PR continues to satisfy the merge criteria for issue #3.
createStoryline: MCV2_Bond token creation, creator transfer, struct storage, events. Fixes #3