docs(nft-zk): sync README and CONTRACT with the current release/claim contract#4
Open
CjDabrow wants to merge 2 commits into
Open
docs(nft-zk): sync README and CONTRACT with the current release/claim contract#4CjDabrow wants to merge 2 commits into
CjDabrow wants to merge 2 commits into
Conversation
… contract The nft-zk README and CONTRACT described the original vendored interface (balanceOf, approve, transferFrom, setApprovalForAll, a local plus shared secret system, getSharedSecret) that no longer exists. The current contract is the per-token commitment model with release/claim transfers, metadata URIs, no on-chain balanceOf, and a secret-derived admin (mintAdmin/rotateAdmin). Rewrite both to match the actual circuits and witnesses, add the two-step transfer flow and the ownPublicKey-is-not-auth note, and correct CONTRACT.md's license line from GPL-3.0 to Apache-2.0 (the source and repo are Apache-2.0). Docs only; no contract or behavior change.
There was a problem hiding this comment.
Pull request overview
This PR updates the contracts/nft-zk documentation to match the current NftZk.compact claim/commitment-based design (per-token owner commitments, release→claim transfers, and public per-token metadata URI), replacing outdated ERC-721-style interface descriptions.
Changes:
- Rewrites
README.mdto describe the per-token owner/claim commitment model, two-step release/claim flow, metadata URI behavior, and the secret-derived admin pattern used by the example contract. - Replaces
CONTRACT.mdwith an updated interface reference: ledger state, witnesses, circuit signatures, transfer flow, admin model, and security considerations. - Updates licensing text in docs to Apache-2.0 and links to the repository LICENSE.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| contracts/nft-zk/README.md | Updated high-level README to reflect commitment-based ownership and release/claim transfers. |
| contracts/nft-zk/CONTRACT.md | Updated full contract reference documentation to match current module/example contract interfaces and behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+36
| The core NFT-ZK logic lives in `modules/NftZk.compact` and exports its circuits with no authorization. | ||
| The example contract in `nft-zk.compact` imports the module and adds a **secret-derived admin** as one | ||
| way to gate issuance. You decide how to authorize the circuits in your own contract. |
| | `burn(tokenId)` | Owner burns their own token and its metadata. | | ||
| | `mint(claim_commitment, tokenId, uri)` | Create a new token *(module only, not exported by the example, wrap with your auth)*. | | ||
|
|
||
| The example contract (`nft-zk.compact`) adds: |
| - **Two secrets** back the flows: `getLocalSecret()` (the owner's self secret) and `getClaimSalt()` (a | ||
| fresh per-transfer salt generated by the recipient). | ||
|
|
||
| ## Transfers (two step: release then claim) |
Comment on lines
+13
to
+15
| - **Module** (`./modules/NftZk.compact`): the reusable privacy-preserving NFT functionality you import. | ||
| - **Example contract** (`nft-zk.compact`): shows how to wrap the module with a secret-derived admin. | ||
| - **Your implementation**: import the module and apply your own authorization pattern. |
| ``` | ||
|
|
||
| ### Usage Example (`nft-zk.compact`) | ||
| // module (NftZk.compact) |
| assert(contractAdmin == deriveAdminPublicKey(getAdminSecret()), "Not authorized to rotate admin."); | ||
| contractAdmin = disclose(newAdmin); | ||
| } | ||
| // example contract (nft-zk.compact) |
Comment on lines
+95
to
+99
| Admin authorization is **secret-derived**: the contract stores only the derived admin public key on the | ||
| ledger, and each admin circuit proves knowledge of the admin secret (`contractAdmin == | ||
| deriveAdminPublicKey(localSecretKey())`). It does **not** authorize with `ownPublicKey()`, which is a | ||
| prover-supplied witness and not a trustworthy caller identity. See [CONTRACT.md](./CONTRACT.md) and | ||
| [KNOWN_LIMITATIONS.md](./KNOWN_LIMITATIONS.md). |
| 4. **Secret Storage**: Store secrets securely and never expose them | ||
| 5. **Hash Collision**: While extremely unlikely, be aware of potential hash collisions | ||
| 6. **Authorization**: Verify proper authorization before sensitive operations | ||
| See [KNOWN_LIMITATIONS.md](./KNOWN_LIMITATIONS.md) for the full honest caveat list. |
- Use src/ prefixes for file path references in README and CONTRACT so links resolve against the repo layout. - Reword the two-steps transfer phrasing. - Update KNOWN_LIMITATIONS M1 (and the C2 / header framing) to the current secret-derived admin (deriveAdminPublicKey(localSecretKey())) instead of the old ownPublicKey()-based gate, so the doc set no longer contradicts the contract or the new README/CONTRACT. Docs only; no contract or behavior change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Syncs the
nft-zkdocs with the current contract. The README and CONTRACT still described the original vendored interface, which no longer matches the code, and KNOWN_LIMITATIONS still described the old admin model.Why
contracts/nft-zk/README.mdandCONTRACT.mddocumentedbalanceOf,approve,transferFrom,setApprovalForAll,getApproved, and a "local plus shared secret" system with agetSharedSecret()witness. The currentNftZk.compactis the per-token commitment model:commitOwner/commitClaim, two stepreleasethenclaimtransfers,tokenUrimetadata, no on-chainbalanceOf, and a secret-derived admin (mintAdmin/rotateAdmin) vialocalSecretKey(). Separately,KNOWN_LIMITATIONS.mdstill described the earlierownPublicKey()-based admin as the fix, which contradicts the current contract.What changed
ownPublicKey().ownPublicKey()-based gate to the current secret-derived admin (deriveAdminPublicKey(localSecretKey())), so the doc set no longer contradicts the contract or the rewritten README/CONTRACT.Notes
src/path prefixes added so references resolve against the repo layout, and the KNOWN_LIMITATIONS contradiction the reviewer flagged is now fixed.