Skip to content

fix(grc20)!: decouple token id from symbol#5908

Merged
moul merged 12 commits into
gnolang:masterfrom
notJoon:fix/grc20-token-id
Jul 17, 2026
Merged

fix(grc20)!: decouple token id from symbol#5908
moul merged 12 commits into
gnolang:masterfrom
notJoon:fix/grc20-token-id

Conversation

@notJoon

@notJoon notJoon commented Jul 7, 2026

Copy link
Copy Markdown
Member

Description

Related to gnoswap-labs/gnoswap#1334

GRC20 event identifiers previously used <origin realm>.<symbol>. Because name and symbol are display metadata, multiple token instances created in the same realm could emit identical token attributes.

This PR separates token identity from display metadata. Token.ID() now returns <origin realm path>.<realm-local ID>, and stateful token creators are responsible for allocating non-reusable IDs.

Changes

GRC20 API and validation

  • Adds a dedicated id field to grc20.Token.
  • Changes the constructor to NewToken(name, symbol, decimals, id, cur), with the realm parameter last and no dummy argument.
  • Separates ID validation from symbol validation.
  • Accepts seqid-compatible IDs using [A-Za-z0-9_-] with a 13-byte maximum, including the full seqid.ID.String() encoding.
  • Keeps name and symbol as display metadata.

Realm-owned ID allocation

  • Updates grc20factory to maintain one persistent seqid.ID shared by every token creation.
  • Uses the realm path as the namespace and the sequence value to distinguish tokens created within that realm.
  • Uses a fixed ID such as "token" in examples that create only one token.

Tests

  • Adds a filetest that creates two tokens with the same name and symbol in one realm and verifies that their Transfer and Approval event IDs differ.
  • Adds unit coverage for duplicate symbols, ID validation, and the 13-byte seqid boundary.
  • Updates GRC20, factory, treasury, registry, and quarantined-package tests for the new API.

Callers and documentation

  • Migrates all affected examples and realms to the new constructor.
  • Documents persistent seqid.ID allocation in the constructor example and factory code.
  • Updates the Gno and gnokey documentation touched by the migration, including explicit cross(cur) examples.

Design rationale

A helper such as grc20.GenerateID() cannot guarantee uniqueness because p/ packages do not own persistent state. Instead, the realm path provides namespace isolation, while a stateful realm or factory owns a persistent sequence for IDs within that namespace.

This keeps grc20 stateless and makes the ownership of the uniqueness guarantee explicit.

Breaking changes

  • grc20.NewToken(0, cur, name, symbol, decimals) becomes grc20.NewToken(name, symbol, decimals, id, cur).
  • Token.ID() is no longer derived from symbol.
  • Existing callers must provide a stable realm-local ID.

Limitations

The grc20 package validates only the ID format. It cannot prevent a stateful caller from reusing an ID. Realms and factories that create multiple tokens must use one persistent sequence across all creation paths and must not reset or reuse it.

@github-actions github-actions Bot added 📖 documentation Improvements or additions to documentation 🧾 package/realm Tag used for new Realms or Packages. labels Jul 7, 2026
@Gno2D2 Gno2D2 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Jul 7, 2026
@Gno2D2

Gno2D2 commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: notJoon/gno-core)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🟢 User davd-gzl already reviewed PR 5908 with state APPROVED
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

@davd-gzl davd-gzl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also CI is failing

Comment thread examples/gno.land/p/demo/tokens/grc20/token.gno Outdated
@notJoon
notJoon requested a review from davd-gzl July 14, 2026 11:46
@github-actions github-actions Bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jul 14, 2026

@davd-gzl davd-gzl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM otherwise

Comment thread examples/gno.land/p/demo/tokens/grc20/token.gno Outdated
Comment thread examples/gno.land/p/demo/tokens/grc20/token.gno Outdated
Comment thread examples/gno.land/r/demo/defi/foo20/foo20.gno Outdated
Comment thread examples/gno.land/p/demo/tokens/grc20/token.gno Outdated
@notJoon

notJoon commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

@davd-gzl I just checked your review comments and applied all the suggestions. If you have a chance, please take another look. Thank you!

@Gno2D2 Gno2D2 removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Jul 15, 2026
@moul

moul commented Jul 16, 2026

Copy link
Copy Markdown
Member

Review note — no correctness/determinism blockers; the decoupling is complete (all in-repo grc20.NewToken( callers migrated, tests + docs + .txtar updated consistently) and the duplicate-symbol bug is genuinely fixed via a realm-owned seqid.ID.

Warnings worth addressing before merge:

  1. Breaking change to gno.land/p/demo/tokens/grc20 — per AGENTS.md ("Don't break gno.land/p/demo/ backwards-compat — needs discussion"), a maintainer should confirm the discussion happened. Two breaks: NewToken signature and Token.ID() output format. Correctly marked ! and references gnoswap#1334, so coordination appears intended — please link the sign-off.
  2. Emitted token event attribute changes for every token (e.g. foo20.FOOfoo20.FOO.0000000). Any off-chain indexer keyed on that attribute breaks at the upgrade boundary — belongs in release notes.
  3. PR body oversells "decoupling" — the code keeps symbol as a component (pkgPath + "." + symbol + "." + id); uniqueness no longer depends on symbol, which is the real fix. The in-code doc is accurate; just reconcile the body.

⚠️ Cross-PR interaction with #5907 (registry keys off token.ID()) and #5962 (wrappers). Coordinate merge order.

🤖 Posted autonomously by an AI review agent on @moul's behalf; not human-reviewed before posting.

@moul

moul commented Jul 16, 2026

Copy link
Copy Markdown
Member

Heads-up — this now has a semantic conflict with master after #5907 merged (not just a textual one). I tried to bring master in and stopped, because resolving it correctly is a design decision for the author, not a mechanical merge:

So a naive merge lands in one of two broken states:

  1. Keep fix(grc20reg): prevent token path overwrite and aliases #5907's Register → its key != token.ID() check is now pkgpath.symbol != pkgpath.symbol.<id>, i.e. always true → Register always panics.
  2. Keep this branch's Registersilently regresses fix(grc20reg): prevent token path overwrite and aliases #5907's overwrite/alias/nil protections (a security regression).

Additionally the auto-merge left grc20reg_test.gno / tokenhub_test.gno calling NewToken with both signatures (NewToken(0, cur, …) from #5907's new tests vs this PR's NewToken(name, symbol, decimals, id, cur)), so it won't compile until every new caller is migrated.

Please decide how the decoupled Token.ID() (now carrying a seqid.ID) should interact with the registry key under #5907's guards, then rebase — that reconciliation shouldn't be guessed. This is also the p/demo/ backwards-compat break that (per AGENTS.md) wants explicit sign-off. I left the branch untouched.

🤖 Posted autonomously by an AI agent on @moul's behalf; not human-reviewed before posting.

@notJoon

notJoon commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

I just started to resolving conflicts now

# Conflicts:
#	examples/gno.land/p/demo/tokens/grc20/token.gno
#	examples/gno.land/p/demo/tokens/grc20/types.gno
#	examples/quarantined/gno.land/r/matijamarjanovic/tokenhub/tokenhub_test.gno
@notJoon

notJoon commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

done 8e0aabc

After gnolang#5907 merged, consumers (atomicswap, treasury) that look tokens up by rlmPath.symbol broke because gnolang#5908 had switched the registry key to Token.ID() (rlmPath.symbol.<seqid>). Keep the registry keyed by rlmPath.symbol (one token per realm+symbol, preserving gnolang#5907's overwrite/alias guards); Token.ID() keeps its trailing sequence id for event/identity uniqueness only. Register now returns the key; tokenhub and tests migrated off Token.ID()-based lookups.
@moul

moul commented Jul 16, 2026

Copy link
Copy Markdown
Member

Pushed a fix for the red CI (the atomicswap / treasury unknown token failures), keeping the registry keyed by rlmPath.symbol rather than the full Token.ID():

  • grc20reg.Register now keys by fqname.Construct(rlmPath, symbol) and returns that key. Token.ID() keeps its trailing sequence id (rlmPath.symbol.<id>) purely for event/identity uniqueness — the registry no longer embeds it. The "own realm" check is preserved as strings.HasPrefix(token.ID(), key+"."), and fix(grc20reg): prevent token path overwrite and aliases #5907's overwrite/alias/nil guards are kept intact.
  • Consequence: a realm still cannot register two tokens under the same symbol (one per realm+symbol), same as fix(grc20reg): prevent token path overwrite and aliases #5907. The seqid decoupling remains visible in Token.ID() / events, just not in the registry key. I kept a test asserting both (grc20reg_test.go: distinct IDs, second same-symbol registration rejected).
  • Migrated the Token.ID()-based lookups to the returned key: tokenhub.RegisterToken (returns the key), tokenhub tests, grc20reg tests, and the grc20_registry_emit.txtar lookup arg. Consumers already using rlmPath.symbol (atomicswap, treasury) now resolve again with no change.

Verified locally: grc20reg, atomicswap, grc20factory, gov/dao/v3/treasury/test, tokenhub, p/demo/tokens/grc20, and TestTestdata/grc20_registry_emit all pass.

This is the less-disruptive of the two options (keeps existing (realm, symbol) lookups working); it does not change Token.ID() itself. If you'd instead prefer consumers to look up by full Token.ID(), that's the larger migration and a different call — let me know.

🤖 Pushed & posted autonomously by an AI agent on @moul's behalf; not human-reviewed before posting.

@moul
moul merged commit 24088cf into gnolang:master Jul 17, 2026
84 checks passed
@github-project-automation github-project-automation Bot moved this from In Review to Done in 🤝🏻 Partner: Onbloc Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📖 documentation Improvements or additions to documentation 📦 ⛰️ gno.land Issues or PRs gno.land package related 🧾 package/realm Tag used for new Realms or Packages.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants