Skip to content

Save full tokenURI data to config.json on ERC-8004 registration#165

Merged
realproject7 merged 3 commits intomainfrom
task/163-save-tokenuri-config
Apr 24, 2026
Merged

Save full tokenURI data to config.json on ERC-8004 registration#165
realproject7 merged 3 commits intomainfrom
task/163-save-tokenuri-config

Conversation

@realproject7
Copy link
Copy Markdown
Owner

Summary

  • Persists agentLlmModel and agentRegisteredAt to config.json after ERC-8004 registration, matching the on-chain tokenURI payload
  • Includes both new fields in the generate-binding response for plotlink.xyz
  • Trims input values consistently between tokenURI and config

Fixes #163

Test plan

  • Register an agent via ERC-8004 and verify ~/.plotlink-ows/config.json contains all tokenURI fields
  • Call generate-binding and verify response includes agentLlmModel and agentRegisteredAt
  • Verify existing registration flow still works without regressions

🤖 Generated with Claude Code

@realproject7
Copy link
Copy Markdown
Owner Author

@re2 Review — REQUEST CHANGES

The code logic is clean and correct — one issue to fix before approval:

🔴 Version conflict in package.json

PR #164 already merged and bumped main to 1.0.21. This branch still shows 1.0.20 → 1.0.21, which will merge-conflict. Please rebase on main and bump to 1.0.22.

✅ Code changes look good

  • registeredAt extracted to a variable so the same timestamp is used in both tokenURI and config — correct
  • .trim() now consistent between tokenURI JSON and writeConfig — good cleanup
  • agentLlmModel and agentRegisteredAt added to both config write and generate-binding response — matches acceptance criteria
  • Optional chaining on body.genre?.trim() — correct

Rebase + version fix, then 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.

Verdict: REQUEST CHANGES

Summary

The change closes most of the gap, but it still does not persist the full tokenURI payload described in issue #163. The on-chain tokenURI includes registeredBy, and that field is still omitted from both config.json and the generate-binding response.

Findings

  • [high] The registration flow still saves only a subset of the tokenURI payload. agentURI includes registeredBy: "plotlink-ows", but the subsequent writeConfig() call omits it, so the saved config does not fully reflect the registered tokenURI content.
    • File: app/routes/settings.ts:112, app/routes/settings.ts:161
    • Suggestion: persist registeredBy alongside the other tokenURI-derived fields in config.json.
  • [medium] generate-binding still returns only a subset of the cached tokenURI fields. If registeredBy is part of the tokenURI payload we cache, it should be included here as well for consistency with the issue acceptance around full tokenURI data availability.
    • File: app/routes/settings.ts:59
    • Suggestion: add the cached registeredBy field to the response, matching the saved config payload.

Decision

Requesting changes because the PR still saves and returns a partial tokenURI payload rather than the full registered content required by issue #163.

realproject7 and others added 2 commits April 24, 2026 15:46
Persist agentLlmModel and agentRegisteredAt alongside existing
fields after ERC-8004 registration. Include both in the
generate-binding response for plotlink.xyz.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@realproject7 realproject7 force-pushed the task/163-save-tokenuri-config branch from 9df8241 to 7b55c70 Compare April 24, 2026 06:47
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: REQUEST CHANGES

Summary

I re-reviewed the force-pushed head. It appears to be a rebase plus version bump only; the tokenURI completeness blocker is still present.

Findings

  • [high] app/routes/settings.ts:114 still includes registeredBy: "plotlink-ows" in the on-chain tokenURI, but app/routes/settings.ts:160 still omits that field from writeConfig(), so config.json still does not reflect the full registered tokenURI content.
    • Suggestion: persist the cached registeredBy field alongside the rest of the tokenURI-derived config.
  • [medium] app/routes/settings.ts:67 still omits the cached registeredBy field from generate-binding, so the response is still only a partial view of the saved tokenURI payload.
    • Suggestion: include registeredBy in the response if you are caching the full tokenURI payload.

Decision

Keeping this in request-changes state until the saved and returned config data matches the full tokenURI payload.

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.

Verdict: COMMENT

Summary

The latest head addresses the missing tokenURI field. I found no remaining code blockers in the registration/config caching change.

Findings

  • No remaining code blockers on the current diff.

Decision

Holding final approval until the current lint-and-typecheck run is green.

@realproject7
Copy link
Copy Markdown
Owner Author

@re2 Re-review — APPROVE ✅

All issues addressed:

  1. Version conflict (my original request) — rebased on main, bumped to 1.0.22. Fixed.
  2. Missing registeredBy (@re1's catch) — agentRegisteredBy: "plotlink-ows" now in both writeConfig and generate-binding. Fixed.

Config now matches the full on-chain tokenURI payload: name, description, genre, llmModel, registeredBy, registeredAt. Trims are consistent. CI green.

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.

Verdict: APPROVE

Summary

The registration/config caching change now matches the full tokenURI payload required by issue #163. The current head persists and returns the missing fields without changing the existing registration flow.

Findings

  • No remaining blocking issues.

Decision

Approving because the implementation meets the acceptance criteria and lint-and-typecheck passed on the current head.

@realproject7 realproject7 merged commit 464853b into main Apr 24, 2026
1 check 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.

[Feature] Save full tokenURI data to config.json on ERC-8004 registration

2 participants