Skip to content

Polish plotlink-contracts README.md#63

Merged
realproject7 merged 2 commits intomainfrom
task/447-readme-polish
Mar 23, 2026
Merged

Polish plotlink-contracts README.md#63
realproject7 merged 2 commits intomainfrom
task/447-readme-polish

Conversation

@realproject7
Copy link
Copy Markdown
Owner

Summary

  • Replace default Foundry README with project-specific documentation
  • Project overview, contract architecture, routing table
  • Deployed addresses (Base mainnet)
  • External dependencies table
  • Build / test / deploy instructions
  • Project structure overview

Fixes #255

Test plan

  • README renders correctly on GitHub
  • All addresses verified correct
  • All commands verified accurate

🤖 Generated with Claude Code

- Project overview: on-chain storytelling, bonding curves, royalties
- StoryFactory and ZapPlotLinkV2 descriptions with routing table
- Deployed addresses (Base mainnet only)
- External dependencies table
- Build, test, deploy instructions
- Project structure

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@project7-interns project7-interns self-requested a review March 23, 2026 09:56
Copy link
Copy Markdown
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 README rewrite is scoped correctly and substantially improves project-specific documentation. I verified the referenced script filenames and the PR is green.

Findings

  • [resolved] Commands and referenced script paths are valid.
    • File: README.md:43
    • Suggestion: None.

Decision

Approving because the documentation is accurate, focused, and passes checks.

Copy link
Copy Markdown
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 Code Review — PR #63 (README Polish)

Overall this is a solid replacement of the default Foundry boilerplate with project-specific documentation. The structure is clear and the routing table is a nice touch. However, I found two factual errors and one omission that should be fixed before merging.


BLOCK — Factual Errors

1. Royalty percentage is wrong

"Each storyline token trades on a Mint Club V2 bonding curve with 5% creator royalties on every buy and sell."

The actual contract (StoryFactory.sol) defines:

uint16 public constant MINT_ROYALTY = 100; // 1% (basis points, base 10000)
uint16 public constant BURN_ROYALTY = 100; // 1%

That is 1% on mint and 1% on burn, NOT 5%. Please correct this.

2. License mismatch

The README states BSD-3-Clause, but:

  • The repo LICENSE file is MIT
  • StoryFactory.sol uses // SPDX-License-Identifier: MIT
  • ZapPlotLinkV2.sol uses // SPDX-License-Identifier: BSD-3-Clause

The contracts themselves are inconsistent, but the repo LICENSE file is MIT. The README should either say "MIT" (matching the repo LICENSE) or acknowledge the mixed licensing. At minimum, do not state BSD-3-Clause as the sole license when the LICENSE file says MIT.


REQUEST CHANGES — Accuracy

3. ERC-8004 Registry listed as external dependency but unused

The README lists ERC-8004 Registry at 0x8004... as an external dependency with role "Agent writer identity". However, neither StoryFactory.sol nor ZapPlotLinkV2.sol reference this address or import any ERC-8004 interface. No deploy script or test references it either.

If this is used only by the frontend/off-chain, it should not be listed under contract external dependencies — it's misleading. If it's planned but not yet integrated, note that explicitly.


MINOR — Omission

4. PLTest.sol exists in src/ but is not listed in the Project Structure

src/PLTest.sol (testnet ERC-20 stand-in) is present in the repo but missing from the project structure tree. Either add it or note that it's a testnet utility.


Verified Correct

  • ZapPlotLinkV2 address 0x04f557F8D2806B34FC832a534c08DF514D4dfEeF — matches deployment script naming
  • StoryFactory address 0xfa5489b6710Ba2f8406b37fA8f8c3018e51FA229 — listed correctly
  • MCV2_Bond 0xc5a076cad94176c2996B32d8466Be1cE757FAa27 — matches contract constants
  • MCV2_BondPeriphery 0x492C412369Db76C9cdD9939e6C521579301473a3 — matches contract constants
  • PLOT 0xF8A2C39111FCEB9C950aAf28A9E34EBaD99b85C1 — matches deploy scripts and contract constants
  • HUNT 0x37f0c2915CeCC7e977183B8543Fc0864d03E064C — matches contract constants
  • Uniswap V4 Universal Router 0x6fF5693b99212Da76ad316178A184AB56D299b43 — matches contract constants
  • Uniswap V4 Quoter 0x0d5e0F971ED27FBfF6c2837bf31316121532048D — matches contract constants
  • Routing table (ETH single-hop, USDC multi-hop, HUNT via MCV2, PLOT direct) — matches @dev comments in ZapPlotLinkV2
  • Foundry build/test commands — correct
  • Deploy scripts reference correct filenames
  • No sensitive info (private keys read from env, not hardcoded)
  • createStoryline(), chainPlot(), donate() function descriptions — accurate

Verdict: REQUEST CHANGES

Fix items 1-3 (royalty %, license, ERC-8004 dependency). Item 4 is minor/optional.

Copy link
Copy Markdown
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: PR #63 — README Polish

ISSUES

1. [MEDIUM] Royalty percentage is incorrect

README claims "5% creator royalties" but StoryFactory.sol defines MINT_ROYALTY = 100 and BURN_ROYALTY = 100 (1% each in basis points out of 10000), not 5%.

2. [MEDIUM] License mismatch

README states BSD-3-Clause, but the repo's LICENSE file is MIT and StoryFactory.sol uses MIT. Only ZapPlotLinkV2.sol uses BSD-3-Clause. Don't claim a single license that contradicts the repo LICENSE file.

3. [LOW] ERC-8004 Registry listed as dependency but not referenced

Listed as an external dependency at 0x8004... but neither StoryFactory.sol nor ZapPlotLinkV2.sol reference it in code, interfaces, deploy scripts, or tests. Misleading to list as a contract dependency.

VERIFIED CORRECT

  • All 8 deployed/external addresses match contract constants
  • Routing table matches ZapPlotLinkV2 logic
  • Foundry commands accurate
  • Deploy script filenames correct
  • No sensitive info exposed

VERDICT: REQUEST CHANGES — Fix royalty percentage and license before merge.

- Royalty: 1% mint + 1% burn (was incorrectly stated as 5%)
- License: MIT (matching LICENSE file, was BSD-3-Clause)
- Remove ERC-8004 Registry from external deps (not referenced)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
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 Re-Review — PR #63 (README Polish)

All three requested fixes verified against the latest commit (54a5c23):

  1. Royalty % — Now reads "1% creator royalties on mint and 1% on burn". Matches StoryFactory.sol constants (MINT_ROYALTY = 100, BURN_ROYALTY = 100). FIXED.
  2. License — Now states "MIT" under ## License. Matches the repo LICENSE file and StoryFactory.sol SPDX header. FIXED.
  3. ERC-8004 Registry — Removed from the External Dependencies table. Only the 6 actually-referenced contracts remain. FIXED.

Item 4 (PLTest.sol missing from project structure) was minor/optional and is acceptable as-is.

VERDICT: APPROVE

Copy link
Copy Markdown
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 Re-Review: PR #63

APPROVED

All 3 issues fixed:

  1. Royalty corrected to "1% mint + 1% burn" — matches StoryFactory.sol constants
  2. License corrected to MIT — matches repo LICENSE file
  3. ERC-8004 Registry removed from external deps

@realproject7 realproject7 merged commit c5666b0 into main Mar 23, 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.

2 participants