refactor: split Token into Token + WrappedToken hierarchy [SDK-96] - #323
Conversation
Public API Changes
|
a23faa9 to
1ad28cc
Compare
Restructures the high-level token API to mirror the ERC-7984 contract architecture: `Token` for the base confidential token, `WrappedToken` extending it with wrapper-specific operations. - `Token`: read + base writes (balance, transfer, set/isOperator, decryptBalanceAs, batchBalancesOf, batchDecryptBalancesAs) - `WrappedToken extends Token`: shield, unshield, unshieldAll, resumeUnshield, unwrap, unwrapAll, finalizeUnwrap, allowance, approveUnderlying, underlying `ReadonlyToken` is removed; reads live on `Token`. `createToken(addr)` takes a single address (no wrapper param); new `createWrappedToken(addr)` returns a `WrappedToken` — the wrapper IS the confidential token. Mutation factories (shield, unshield, unwrap, finalize, approveUnderlying, resumeUnshield) now take a `WrappedToken`. `delegateDecryption` / `revokeDelegation` mutation factories now take `(sdk, contractAddress)`, matching the SDK-level primitives added in SDK-95. Tests updated in-place; `delegation.test.ts` and `readonly-token.test.ts` are removed (coverage migrated to `zama-sdk.test.ts`, `token.test.ts`, `batch-decrypt-as.test.ts`); new `wrapped-token.test.ts` covers the wrapper-specific surface. Migration: `ReadonlyToken`, `createReadonlyToken`, and the two-argument `createToken(addr, wrapper)` are removed. Callers that need wrapper operations should use `createWrappedToken(wrapperAddr)`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mirrors the SDK-side refactor in the React layer.
- `useToken(address)`: returns the base `Token` (single-arg)
- `useWrappedToken(address)`: new hook returning a `WrappedToken`
- `useReadonlyToken` is removed — reads live on `Token`
Hook signatures unified:
- single-arg: `useDelegateDecryption`, `useRevokeDelegation`,
`useDecryptBalanceAs`, `useApproveUnderlying`, `useUnshield`,
`useUnshieldAll`, `useResumeUnshield`, `useUnwrap`, `useUnwrapAll`,
`useFinalizeUnwrap`, `useConfidentialTransferFrom`,
`useConfidentialSetOperator`
- config object `{ address, ... }`: `useConfidentialBalance`,
`useConfidentialBalances` (`addresses`), `useConfidentialTransfer`,
`useShield`, `useConfidentialIsOperator`, `useUnderlyingAllowance`
`UseZamaConfig` is removed from the public barrel.
Migration: hook signatures change as listed above; `useReadonlyToken`
is removed. Migrate to `useToken` (reads) or `useWrappedToken` (wrapper
ops). Object-form callers should rename `tokenAddress` → `address` and
drop the now-unused `wrapperAddress` (the wrapper IS the token).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The wrapper IS the confidential token, so the harness pages no longer
need a separate `wrapperAddress` prop. Each page falls back to
`tokenAddress` and the forms drop the dual-address signature in favour
of the new single-arg / `{ address }` hook shapes.
Touches `test/test-components`, `test/test-nextjs`, and `test/test-vite`.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removes the obsolete `ReadonlyToken` / `useReadonlyToken` reference pages and adds placeholder pages for `WrappedToken` / `useWrappedToken`. Full reference docs to follow once the API is stabilised. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move SDK-145's shield routing into WrappedToken (isPayable detection, #shieldViaTransferAndCall and #shieldViaApproveAndWrap branches). - Update wrapped-token tests with the new isPayable read mocks. - Switch shield.test.ts dynamic imports from Token to WrappedToken for the shieldPath event tests. - Regenerate API reports + LLM corpus artifacts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1ad28cc to
0251d20
Compare
…hierarchy-into-token-wrappedtoken
- wrapped-token: rewrite shield() JSDoc to document ERC-1363 routing and fix "Unshield"/"Unshield-all" error messages to "Unwrap"/"UnwrapAll" - delegation-check, index.ts: update JSDoc references from ReadonlyToken to ZamaSDK/WrappedToken - test/playwright/node: replace createReadonlyToken with createToken (regression fix lost in last rebase) - docs sweep: AGENTS.md, jsdoc skill, agent docs, gitbook references/guides/tutorials, packages/sdk README — finalize ReadonlyToken → Token/WrappedToken cleanup - regenerate llms.txt, llms-full.txt, corpus-manifest.json Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…warn, diagrams - isPayable() no longer caches false on probe error; re-probes on next call, with a console.warn so transient RPC failures stay visible - Architecture diagrams: ReadonlyToken → WrappedToken (extends Token) - use-shield tests: drop dead wrapperAddress field - 5 guides + 2 tutorials + 17 reference docs aligned with the new Token / WrappedToken hierarchy and positional / config-object hook signatures - Regenerate llms-full.txt from the updated guide and reference content Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hierarchy-into-token-wrappedtoken Resolves conflicts after #325 (centralize Zod validation at public boundaries): - ReadonlyToken.md, readonly-token.test.ts: kept SDK-96's deletion (class no longer exists in the new Token / WrappedToken hierarchy) - query/index.ts: keep WrappedToken export, drop ZamaSDKConfig export (the type was inlined / removed by #325) - token.test.ts: import the new ERC-20 / unshield validation tests added by #325 only where they apply to Token (confidentialTransfer cache and boundary cases). shield / unshield variants migrated to wrapped-token.test.ts - wrapped-token.test.ts: add the new boundary + skipBalanceCheck-with-callbacks tests (preserves coverage from #325 on the right class) - optional-signer.test.ts: drop the keypairTTL-rejection test — #325 moved that validation from the ZamaSDK constructor to buildZamaConfig; coverage remains in config/__tests__/schema.test.ts - Token.md: reword create example so it doesn't reference Token.shield (now on WrappedToken) - llms-full.txt: regenerated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hierarchy-into-token-wrappedtoken Reconcile SDK-96 Token/WrappedToken split with prerelease's services fanout (#325, #331, #333) and CVE-2026-31431 workaround. Adopt the new DecryptionService / DelegationService / EncryptionService / CachingService architecture in ZamaSDK while preserving SDK-96's typed createToken / createWrappedToken API. Delete ReadonlyToken artifacts (class, tests, docs) replaced by the Token/WrappedToken hierarchy, and migrate the token-bound delegation tests to Token. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…hierarchy-into-token-wrappedtoken Absorb latest prerelease commits (#322 docs terminology update, #332 react-viem upgrade, #336 next bump). Reconcile docs conflicts: architecture.md adopts the KeypairVault + PermissionStore terminology while keeping Token / WrappedToken hierarchy; Token.md merges the new detailed reference with the SDK-96 split (WrappedToken-only methods moved out); llms-full.txt regenerated from corpus. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Auto-fix formatting on three files flagged by `pnpm format:check` in CI after the prerelease merge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous merge commit regenerated llms-full.txt before oxfmt normalized two source guides — pick up the post-format snapshot. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ghermet
left a comment
There was a problem hiding this comment.
Automated multi-agent review (code-reviewer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer). Findings are filtered to high-confidence items; please treat them as starting points, not verdicts.
PR #323 Review Summary — Token / WrappedToken Split
Stats: 172 files, +3474 / −5928
🔴 Critical (must fix before merge)
| # | Source | Issue | Location |
|---|---|---|---|
| C1 | tests | integration.test.ts is broken — TypeError: token.unwrap is not a function. The fixture still binds token = new Token(...), but shield/unwrap/unshield/finalizeUnwrap live on WrappedToken. Vitest unit config excludes it (CI hides this), but vitest.integration.config.ts includes it. |
packages/sdk/src/token/__tests__/integration.test.ts:14,104,110,153,177 |
| C2 | code-review | ZamaSDK.cache made public so Token can reach this.sdk.cache.get(...). API extractor raises ae-forgotten-export: CachingService. Violates the "no handle leakage; SDK cache owns two-phase optimization" rule. |
packages/sdk/src/zama-sdk.ts:55-56,68; packages/sdk/src/token/token.ts:229,397; packages/sdk/etc/sdk.api.md:20029 |
| C3 | code-review | useUnderlyingAllowance passes the same address twice as tokenAddress and wrapperAddress — stale two-address query-key shape under the new single-address hook surface (a "silent migration shim"). |
packages/react-sdk/src/shield/use-underlying-allowance.ts:41-46,73-77; packages/sdk/src/query/query-keys.ts:91-104 |
| C4 | silent-failures | WrappedToken.isPayable() silently routes to approve+wrap on ANY error, including #getUnderlying() rejections. A transient RPC blip silently extracts an extra approve tx and wallet popup from the user for an ERC-1363 token that should have one-tx-shielded. |
packages/sdk/src/token/wrapped-token.ts:74-90 |
| C5 | silent-failures | AccountService.#handleWalletAccountChange fire-and-forgets relayer chain switch — void swallow("switch relayer chain", () => this.#relayer.switchChain(nextChainId)). If the switch fails, subsequent decrypts run against the previous chain's relayer and return stale plaintexts. Violates "RelayerNative parity". |
packages/sdk/src/services/account-service.ts:102-123 |
| C6 | silent-failures | isFatalBatchError is too narrow — missing ChainMismatchError, WalletAccountNotReadyError, SignerNotConfiguredError. A session-level chain switch mid-batch produces "N of M tokens failed" results where every error is identical and meaningless. |
packages/sdk/src/errors/fatal-batch.ts:14-20 |
| C7 | comments | Token.md documents a non-existent handle? parameter for decryptBalanceAs. Readers will pass it and silently lose the value. |
docs/gitbook/src/reference/sdk/Token.md:52 |
| C8 | comments | unshield-tokens.md examples use sdk.createToken("0xEncryptedERC20").unshield(...) — a TypeScript error after the split (shield-tokens.md was updated, this guide was missed). Same applies to .unshieldAll() and .resumeUnshield(). |
docs/gitbook/src/guides/unshield-tokens.md:35-37,83,107 |
🟡 Important (should fix)
Architecture / types
- Public constructors
new Token(...)/new WrappedToken(...)bypass the SDK factories (zama-sdk.ts:248-250); no eager ERC-165 check insidecreateWrappedToken— plain ERC-20 addresses can be wrapped and only revert at firstshield(). underlyingAllowanceQueryOptions(sdk, tokenAddress, config)is the only wrapper-related query that takes a bare address rather than(token: WrappedToken)— inconsistent withshieldMutationOptions,unshieldMutationOptions, etc. (packages/sdk/etc/sdk-query.api.md:844)- React hook surface re-erases wrapper-ness to a string:
useShield/useUnshield/useUnwrap/etc. takeaddress: Address, internally calluseWrappedToken(address)— type discipline established in@zama-fhe/sdk/querydoesn't survive the trip to react-sdk.
Error handling
WrappedToken.#waitAndFinalizeUnshielddoes not emitZamaSDKEvents.TransactionErroron failure (all other write paths do). Worst case: phase-1 receipt parse fails after on-chain unwrap succeeded — user loses the in-flight unshield with no event and a generic error message that doesn't mentionresumeUnshield(hash). (wrapped-token.ts:557-596)assertConfidentialBalancedocstring promises "no surprise EIP-712 popup" but the implementation can trigger one when credentials aren't cached. (token.ts:686-689,693-717)swallow()usesconsole.warn, notlogError→ invisible to Sentry. Used in 7+ load-bearing sites including chain switch and cache cleanup. (packages/sdk/src/utils/swallow.ts)Token.batchBalancesOftotal-failure aggregation throws only the first error; the rest are dropped, making debugging brutal. (token.ts:321-325)
Tests
- No assertion that
WrappedToken instanceof Token(or that inheritedbalanceOfworks on aWrappedToken). Central design claim of the PR is unguarded against future refactors. ZamaSDK.createWrappedTokenhas no direct factory test (matching the threecreateTokencases).- The three direct unit tests for
Token.decryptBalanceAsfrom the deleteddelegation.test.ts:117-164(zero-handle short-circuit, happy path, "no value returned" error) were not migrated. assertSameSdkcross-SDK guard tests deleted with no replacement.- New hooks
useToken/useWrappedTokenhave no dedicated tests.
Code review
Token.confidentialTransferFromusesrequireChainAlignment, butconfidentialTransferusesrequireAlignedWalletAccount— inconsistent pre-flight error story. (token.ts:502-503vs:566-567)Tokenclass JSDoc says delegation lives onToken, butdelegateDecryption/revokeDelegation/etc. were removed from the class surface and now live only onZamaSDK. The PR description andtoken.ts:70-81overstate the class's surface.
Comments / JSDoc
wrapped-token.ts:310,348—Orchestrates: unshield → wait → ... finalizeis self-referential (impl callsunwrap/unwrapAll).@throwsblocks dropped during migration across 8 methods (confidentialTransferFrom,setOperator,unshield,resumeUnshield,unwrap,unwrapAll,finalizeUnwrap,approveUnderlying).use-confidential-transfer.ts:42references "optional wrapper" leftover from the two-address model.
🟢 Suggestions
- Resolve or remove the self-flagged
TODO: code smellattoken.ts:369— refactor static batch methods to(sdk, tokens, options)socache/sdkcan be made private (folds in C2). - Consider
protectedonZamaSDK.cacheandToken.sdk(or narrow the published type via@internal). Token.isWrapperusesPromise.all→ rejects on first probe failure rather than returningfalse. ConsiderPromise.allSettledso it behaves like a probe.unwrapAllzero-balance guard throwsDecryptionFailedError— error type is misleading. UseInsufficientConfidentialBalanceErroror newZeroBalanceError. (wrapped-token.ts:460-462)- Section-banner comments (
// METADATA,// BALANCES,// SHIELD (ERC-20 → confidential), …) violate "don't explain WHAT" guidance and add no info beyond the JSDoc on each method. - Remove the trivial restatement docstrings (
/** Read the token name from the contract. */).
✅ Strengths
- No legacy shims, no compat re-exports.
ReadonlyToken/createReadonlyToken/useReadonlyTokenremoved cleanly — noReadonlyTokenreferences survive in.api.md. - Constructor consistency: both classes use
(sdk, address); no separatewrapperAddressfield anywhere; address normalization preserved in query keys. - Type-system-enforced separation in
@zama-fhe/sdk/query:shieldMutationOptions,unshieldMutationOptions,finalizeUnwrapMutationOptions, … are typedtoken: WrappedToken. A plainTokeninstance fails at compile time. TokenandWrappedTokenclass-level JSDoc clearly states the split; "the wrapper IS the confidential token" kills the two-address mental model.WrappedToken.isPayableJSDoc was updated to match the new "don't cache failures" behavior — textbook good behavior + comment migration.- Load-bearing rationale preserved: e.g.
// ERC7984ERC20Wrapper.onTransferReceived decodes the recipient via 'address(bytes20(data))'correctly migrated towrapped-token.ts:182-185; the sequential-write rationale on#batchDelegationOppreserved. - Test fixtures factorized well:
mockTokenandmockWrappedTokensharecreateMockTokenInternal. - Shield routing rule honored: no per-call routing flag on
ShieldOptions; path chosen via ERC-165 introspection only. chain-alignment.test.tsmigration is complete:shield/unwrapgo throughcreateWrappedToken,delegateDecryptionmoved onto SDK,batchBalancesOfstayed onToken.- Type-design ratings: 4 / 4 / 4 / 3 (Encapsulation, Invariants, Usefulness, Enforcement). Clear improvement over the prior optional-
wrappermodel.
Recommended Action Plan
- Block merge on C1–C8. C1 is a hard test regression; C2 is a CI-visible API contract break; C3 is a query-key shape bug; C4–C6 are silent-failure paths that mislead users; C7–C8 are docs that don't compile.
- Important issues in priority order: emit
TransactionErrorfrom#waitAndFinalizeUnshield; fixassertConfidentialBalancedoc-vs-impl drift; routeswallowthroughlogError; add the missing tests (instanceof Token,createWrappedTokenfactory,decryptBalanceAsdirect, hook memoization). - Re-run
pnpm test:runandpnpm test:integration(the integration run is what catches C1) before re-requesting review.
enitrat
left a comment
There was a problem hiding this comment.
some regressions observed, globally this PR should not be a lot more than taking existing functions and moving them around files
…K public API Apply changes from PR #323 review (enitrat + ghermet multi-agent review): - Token.decryptBalanceAs now delegates entirely to sdk.delegatedUserDecrypt; the per-method cache lookup and #assertDelegationActive duplicated logic already handled by DecryptionService. - Token.batchDecryptBalancesAs collapses to a single sdk.delegatedBatchDecryptHandlesAs call (instead of resolving handles, doing cache lookups, and looping per token). - Remove #assertDelegationActive from Token (lives in DelegationService now). - Repass ZamaSDK.cache to private (#cache); removes the api-extractor ae-forgotten-export warning on CachingService. - WrappedToken.isPayable restores the negative-cache semantics from prerelease (probe failures cached as false, no permanent re-probing). - useUnderlyingAllowance + zamaQueryKeys.underlyingAllowance.scope drop the duplicated wrapperAddress parameter (single-address model post-SDK-96). - Fix Token.md (drop bogus handle? parameter from decryptBalanceAs signature) and unshield-tokens.md (use sdk.createWrappedToken). - Fix integration.test.ts to use the wrappedToken fixture (and updated mock chains for the new shield path's ERC-20 balance check). - Add tests: createWrappedToken factory (matching createToken trio), WrappedToken instanceof Token assertion, and three decryptBalanceAs direct unit tests (zero-handle short-circuit, happy path, no-value-returned error).
…with prerelease Address enitrat's review feedback on PR #323: - `Token.batchDecryptBalancesAs`: rewrite to mirror prerelease's readonly-token implementation. Uses pLimit with maxConcurrency=10 default for parallel balance reads (was unbounded Promise.all), filters zero handles before contacting the relayer (skip-on-zero short-circuit), tracks balance-read failures in a Map<Address, ZamaError>, and always wraps errors in DecryptionFailedError with the underlying error as `.cause` (drops the size===1 special case). - `WrappedToken.approveUnderlying`: use `sdk.requireAlignedWalletAccount` once at the top instead of split `requireSigner` + `requireChainAlignment` + `signer.requireWalletAccount`. Matches prerelease verbatim. - Update 3 batch-decrypt-as tests to expect the wrapped DecryptionFailedError shape (with typed error preserved via `.cause`). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolve conflicts from prerelease's LifecycleService refactor (#337): - `token.ts` / `wrapped-token.ts`: migrate from method-form `this.sdk.requireAlignedWalletAccount(op)` / `this.sdk.requireChainAlignment(op)` to standalone utility form `requireAlignedWalletAccount(op, signer, provider)` / `requireChainAlignment(op, signer, provider)` from `utils/alignment`, matching the rest of the SDK post-LifecycleService extraction. - `zama-sdk.ts`: drop the inlined wallet-account listener registry (`#walletAccountListeners`) — LifecycleService owns it now. - `test-fixtures.ts`: keep SDK-96's `createToken` / `createWrappedToken` fixtures, add prerelease's new `createLifecycleService` fixture; drop `createReadonlyToken` (ReadonlyToken removed in SDK-96). - `zama-sdk.test.ts`: drop ReadonlyToken import and lifecycle test cases (now covered by the dedicated `lifecycle-service.test.ts` suite). - `token/readonly-token.ts`: confirm deletion from SDK-96 (modify/delete conflict resolved in favor of the deletion). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
one last thing from the last merge commit from main is to use the new
and we will be good to merge |
Address enitrat's review feedback on PR #323: use the standalone `requireAlignedWalletAccount` from `utils/alignment` consistently across signer-bound write paths, replacing the split `requireChainAlignment` + `signer.requireWalletAccount` pattern. - `WrappedToken.shield`: drop redundant `requireSigner` (private helpers re-acquire the signer) and source the user address from the aligned account. - `WrappedToken.unwrap` / `WrappedToken.unwrapAll`: same consolidation, keeping `requireSigner` since those paths drive `signer.writeContract` directly. - `WrappedToken.#ensureAllowance`: align the user-address lookup with the aligned account (chain mismatch was previously not surfaced here). - `Token.confidentialTransferFrom`: use `requireAlignedWalletAccount` for consistency even though the operator's address is taken from the `from` argument. - `Token.confidentialTransfer` already follows this pattern from the prerelease merge — no change needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🎉 This PR is included in version 3.0.0-alpha.36 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Restructures the high-level token API to mirror the ERC-7984 contract architecture.
Token— base confidential token (reads + transfer + operator + delegated decrypt)WrappedToken extends Token— adds wrapper-specific ops: shield, unshield, allowance, unwrap, finalizeUnwrap, etc.ReadonlyTokenis removed — reads live onToken.createToken(addr)is now single-arg; newcreateWrappedToken(addr)returns aWrappedToken(the wrapper IS the confidential token).useToken(address)/useWrappedToken(address);useReadonlyTokenremoved; single-arg /{ address, ... }shapes unified across all hooks (see commit body ofb26cadb5for the full list).WrappedToken;delegateDecryption/revokeDelegationtake(sdk, contractAddress)matching the SDK-95 SDK-level primitives.wrapperAddressprops dropped (defaults totokenAddress).ReadonlyToken.md/useReadonlyToken.mddeleted; placeholder pages added forWrappedToken.md/useWrappedToken.md.Breaking changes
ReadonlyToken,createReadonlyToken, and the two-argumentcreateToken(addr, wrapper)are removed.useReadonlyTokenis removed.{ address, ... }); object-form callers should renametokenAddress→addressand drop the now-unusedwrapperAddress.Migration: for wrapper operations, use
createWrappedToken(wrapperAddr)/useWrappedToken(wrapperAddr).Stack / context
Builds on top of SDK-95 (#307), now squash-merged into
prerelease. Reconciled with the SDK-134 credentials refactor that landed in parallel:signer.requireWalletAccount(op).addressreplacessigner.getAddress();isFatalBatchErrorreplacesisSessionError;confidentialBalanceQueryOptionskeeps SDK-134's signer-scoped third arg.Commits (ordered for review)
feat(sdk)!: split Token into Token + WrappedToken hierarchyfeat(react-sdk)!: align hooks with Token/WrappedToken splittest(harness): align test pages/forms with single-address APIdocs(gitbook): replace ReadonlyToken with WrappedToken stubschore(api-report): regenerate after Token/WrappedToken splitTest plan
pnpm typecheck— cleanpnpm lint— cleanpnpm format:check— cleanpnpm test:run— 1480 passed, 5 skipped, 0 failedpnpm api-report— regenerated, committed🤖 Generated with Claude Code