Non-fungible token unshielded

[emnul]

Brings an implementation of an ERC721-like specification to Compact

This will not compile unless you use compactc v0.23.0

Closes #123 #6 #129

[0xisk]

Thanks you @emnul! In general looks solid. However, I didn't fully yet finish reviewing all the circuits but, those are comments and questions for now.

Also at the mean time, I would ask please if you can add documentation for the circuits that doesn't have yet.

[emnul]

Also at the mean time, I would ask please if you can add documentation for the circuits that doesn't have yet.
@0xisk

For sure I was waiting until the code was in a more stable state before I started writing docs. I will be adding the docs once the tests have been completed / finalized. There are a couple assumptions I made with the use of Field values for the tokenId I want to make sure hold.

[andrew-fleming]

Good start on this, @emnul! I know we're still waiting on the bug fix in the compiler and tests, but I left some initial comments

[0xisk]

Thank you @emnul great work! I left comments but the main discussion is related to the unsafe circuits that is mentioned in this thread #19 (comment).

[0xisk]

Safe & Unsafe Transfer Strategy for ERC Token Migration

I know our team agreed not to accept ContractAddress in transfer right now—if someone sends tokens to a contract, they can’t pull them back because we lack C2C calls. My worry is that if we bake both ZswapCoinPublicKey and ContractAddress into the types today, migrating later when C2C is supported will be painful: it’ll touch the ledger definitions and break every contract that uses this token.

Instead, I’d propose we keep the “final” types from day one (i.e. Either<ZswapCoinPublicKey, ContractAddress>), but restrict the main circuits so they only succeed when the recipient is a ZswapCoinPublicKey. Then we provide a separate, clearly‐named “unsafe” variant for anyone who really needs to send to a contract (knowing that, right now, the contract can’t send funds back). That gives us two options:

1. Safe‐only circuit

   • Signature: transfer(to: Either<ZswapCoinPublicKey, ContractAddress>, amount: Uint128)
   • Behavior: if to is a contract, revert immediately; if to is a public key, do the transfer.

2. Safe + “unsafe” circuit

   • Safe version as in option 1 above (rejects contracts).
   • Add _unsafe_transfer(to: Either<ZswapCoinPublicKey, ContractAddress>, amount: Uint128) that always allows both types—contracts just assume they can’t send back until we enable C2C.

Option 2 is my preference. Once C2C support is live, we can remove the isContract check in transfer — allowing contracts to work normally — and deprecate _unsafe_transfer without touching ABIs or storage layouts. Contracts that already call _unsafe_transfer keep working, and everyone else just uses transfer.

When we are ready for a major release, we can remove _unsafe_transfer entirely:

1. Deprecation phase (minor version):

   • Mark _unsafe_transfer as deprecated in the docs and emit a warning if possible.
   • Keep its implementation intact so existing callers continue to work.

2. Major release:

   • Drop _unsafe_transfer and ensure transfer has no isContract guard.
   • By this point, anyone using _unsafe_transfer should have migrated to the now C2C-capable transfer.

---

Example (Option 2 sketch):

// Public “safe” transfer: only succeeds if `to` is a user key
export circuit transfer(
  to: Either<ZswapCoinPublicKey, ContractAddress>,
  amount: Uint<128>
): [] {
 assert (!isContractAddress(to)) "Use _unsafe_transfer to send to contracts";
  // ...normal user‐to‐user move...
}

// Explicit “unsafe” transfer: allows both keys and contracts
export circuit _unsafe_transfer(
  to: Either<ZswapCoinPublicKey, ContractAddress>,
  amount: Uint<128>
): [] {
  if (to.isZswapCoinPublicKey()) {
    // ...user‐to‐user move...
  } else {
    // ...user‐to‐contract move (caller assumes contract cannot send back)...
  }
}

With this pattern, when C2C calls exist, we simply remove the isContractAddress() guard from transfer. At that point, contracts can receive and send tokens normally, and _unsafe_transfer becomes redundant (so we can deprecate or remove it in the next major release).

[0xisk]

ERC-721 Circuits Requiring Unsafe Variants vs. Harmless Circuits

Circuits that need an unsafe variant (because they move tokens to a recipient, which could be a ContractAddress):

• transferFrom(from: Either<…>, to: Either<…>, tokenId: Uint<128>)

  • This is the main public transfer. As soon as you allow to: Either<…>, you need _unsafe_transferFrom to bypass the “reject contract” check.

• _transfer(from: Either<…>, to: Either<…>, tokenId: Uint<128>)

  • Internal direct‐transfer helper. If you switch its to type to Either<…>, add _unsafe_transfer so it can send into contracts.

• _mint(to: Either<…>, tokenId: Uint<128>)

  • Minting is effectively “transfer from zero.” If you allow to = Either<…>, you must have _unsafe_mint to let a contract receive an NFT (knowing it can’t send back until C2C exists).

---

Circuits that remain harmless (no unsafe variant needed):

• initialize(name_: Opaque<"string">, symbol_: Opaque<"string">)

• name(): Opaque<"string">

• symbol(): Opaque<"string">

• balanceOf(owner: Either<…>): Uint<128>

• ownerOf(tokenId: Uint<128>): Either<…>

• tokenURI(tokenId: Uint<128>): Opaque<"string">

• approve(to: Either<…>, tokenId: Uint<128>)

  • Approvals don’t actually send tokens, so approving a contract is “harmless” today.

• getApproved(tokenId: Uint<128>): Either<…>

• setApprovalForAll(operator: Either<…>, approved: Boolean)

• isApprovedForAll(owner: Either<…>, operator: Either<…>): Boolean

• _requireOwned(tokenId: Uint<128>): Either<…>

• _ownerOf(tokenId: Uint<128>): Either<…>

  • _update(to: Either<…>, tokenId, auth) can be written once to accept both address types, without any “unsafe” variant.

• _approve(to: Either<…>, tokenId: Uint<128>, auth: Either<…>)

• _checkAuthorized(owner: Either<…>, spender: Either<…>, tokenId: Uint<128>)

• _isAuthorized(owner: Either<…>, spender: Either<…>, tokenId: Uint<128>): Boolean

• _getApproved(tokenId: Uint<128>): Either<…>

• _setApprovalForAll(owner: Either<…>, operator: Either<…>, approved: Boolean)

• _burn(tokenId: Uint<128>)

  • Burning can only remove (send to zero), so there’s no “send into a contract” risk here.

• _setTokenURI(tokenId: Uint<128>, tokenURI: Opaque<"string">)

In short, only the circuits that actually move tokens “to” an address require an _unsafe_… counterpart; all read‐only or pure‐approval methods are safe as-is.

[andrew-fleming]

Thanks for this @0xisk! We should provide this migration plan more formally in our docs

[0xisk]

Thank you @emnul! the changes look great, just left some comments mostly are minor. Please check this PR I added more tests which I think they are not covered. #92

[emnul]

@0xisk I realize now I might have used the Initializable module incorrectly. I'm not sure if it's intended to be imported in the top level module or in the sub-modules themselves. I'm using #38 as a reference. What do you think? Also the current structure introduces some challenges with testing.

[0xisk]

Thank you @emnul LGTM! Well done!

[0xisk]

@0xisk I realize now I might have used the Initializable module incorrectly. I'm not sure if it's intended to be imported in the top level module or in the sub-modules themselves. I'm using #38 as a reference. What do you think? Also the current structure introduces some challenges with testing.

In the #38 PR, I think it is not completed yet. The idea of initializable module is make sure that all the circuits in your module are being called after the initialize() circuit, as in this PR: #95

[andrew-fleming]

Very good improvements @emnul! Sorry for the billions of comments 😅 most of them are minor readability suggestions and improvements in doc/test consistency. The big things though are:

• Change the token name
• Simplify circuit logic. Basically treat the safe variants as wrappers that only check that the recipient is not a contract address and then invoke the unsafe variant with the sanitized recipient

[andrew-fleming]

Thanks for this @0xisk! We should provide this migration plan more formally in our docs

[andrew-fleming]

Superseded by #158