Skip to content

[#43] Add owner role, updateCurve(), trust documentation#48

Merged
realproject7 merged 1 commit intomainfrom
task/43-owner-update-curve
Mar 20, 2026
Merged

[#43] Add owner role, updateCurve(), trust documentation#48
realproject7 merged 1 commit intomainfrom
task/43-owner-update-curve

Conversation

@realproject7
Copy link
Owner

Summary

  • Add address public owner (set to msg.sender in constructor)
  • Add onlyOwner modifier
  • Add updateCurve(newRanges, newPrices) — updates bonding curve params for future storylines
  • Add CurveUpdated(uint256 newStepCount) event
  • Document trust assumption on updateBondCreator call (NatSpec)
  • Existing storylines unaffected — they already have tokens on MCV2

Test plan

  • All 45 tests pass (35 StoryFactory + 10 DeployBase)
  • 7 new tests: owner set, updateCurve happy path + 4 reverts + new storyline uses updated params
  • forge fmt --check clean

Fixes #43

🤖 Generated with Claude Code

- Add owner state variable (set to msg.sender in constructor)
- Add onlyOwner modifier
- Add updateCurve(newRanges, newPrices) for future storyline curves
- Add CurveUpdated event
- Document trust assumption on updateBondCreator (NatSpec)
- 7 new tests: owner set, updateCurve happy/reverts, new storyline
  uses updated params

All 45 tests pass.

Fixes #43

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

T2b Review — APPROVED

  • Owner set in constructor, onlyOwner modifier is standard
  • updateCurve replicates constructor validation (length match, non-empty, ≤1000) — correct
  • calldata for arrays is gas-efficient
  • NatSpec trust note on updateBondCreator documents the risk clearly
  • Existing storylines unaffected since their tokens are already created on MCV2_Bond

Note: no transferOwnership — if that's needed it should be a separate ticket. Current scope matches the issue spec.

7 new tests cover all paths including integration (new storyline uses updated params). LGTM.

Copy link
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE

Summary

The PR adds the requested owner role and owner-only updateCurve() path with the expected validation guards, event emission, and trust-note documentation on updateBondCreator. The tests cover owner initialization, success, revert paths, and the post-update curve behavior for future storylines, and GitHub checks are passing.

Findings

  • None.

Decision

Approving because the change matches the issue scope, the access-control boundary is explicit, and the updated tests cover the new behavior directly.

@realproject7 realproject7 merged commit de1b501 into main Mar 20, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Contract] Owner role + updateCurve + trust documentation (#38 Group C)

2 participants