refactor(token): restore zswap public key auth for fungible balances#512
refactor(token): restore zswap public key auth for fungible balances#512AndrewWestberg wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR refactors FungibleToken's account identity model from ChangesFungibleToken Account Identity Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contracts/src/token/FungibleToken.compact (1)
675-685: 💤 Low valueDoc pseudocode doesn't match the actual implementation.
Line 679 documents the derivation as
persistentHash("midnight:zswap-pk[v1]", zswapCoinSecretKey), but the implementation hashes aZswapCoinPublicKeyPreimagestruct containing a nestedZswapCoinSecretKeywrapper. The struct layout is what consumers must replicate to verify correctness — the current pseudocode is misleading.📝 Suggested doc clarification
- * ## Key Derivation - * `zswapCoinPublicKey = persistentHash("midnight:zswap-pk[v1]", zswapCoinSecretKey)` + * ## Key Derivation + * `zswapCoinPublicKey.bytes = persistentHash(ZswapCoinPublicKeyPreimage { + * sep: "midnight:zswap-pk[v1]", + * secretKey: ZswapCoinSecretKey { bytes: zswapCoinSecretKey } + * })`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/FungibleToken.compact` around lines 675 - 685, The documentation for Zswap coin public key derivation is misleading: it states zswapCoinPublicKey = persistentHash("midnight:zswap-pk[v1]", zswapCoinSecretKey) but the code actually hashes a ZswapCoinPublicKeyPreimage struct that wraps the secret in a ZswapCoinSecretKey before calling persistentHash; update the docstring for the function that returns ZswapCoinPublicKey to describe the actual preimage layout (mention ZswapCoinPublicKeyPreimage and ZswapCoinSecretKey) and show that persistentHash("midnight:zswap-pk[v1]", ZswapCoinPublicKeyPreimage) is what is computed so consumers can reproduce the exact struct encoding used by persistentHash.contracts/src/token/test/simulators/FungibleTokenSimulator.ts (1)
16-16: ⚡ Quick winImport
ZswapCoinPublicKeyfrom the generated artifact instead of defining it locally.FungibleTokenSimulator is inconsistent with other contract simulators in the codebase (UtilsSimulator, ZOwnablePKSimulator, ShieldedTokenSimulator, etc.), which all import
ZswapCoinPublicKeyfrom their respective generated artifacts. Defining it locally here risks diverging from the contract boundary if the artifact's type shape changes during regeneration.♻️ Proposed fix
import { type ContractAddress, type Either, + type ZswapCoinPublicKey, ledger, Contract as MockFungibleToken, } from '../../../../artifacts/MockFungibleToken/contract/index.js'; - -type ZswapCoinPublicKey = { bytes: Uint8Array };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/test/simulators/FungibleTokenSimulator.ts` at line 16, FungibleTokenSimulator defines a local type alias "ZswapCoinPublicKey" which should be imported from the generated contract artifact to stay consistent with other simulators; remove the local "type ZswapCoinPublicKey = { bytes: Uint8Array }" and add an import of ZswapCoinPublicKey from the contract's generated artifact in the FungibleTokenSimulator file so all references use the artifact-provided type and remain in sync with future regenerations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/src/token/FungibleToken.compact`:
- Around line 80-95: Add an integration test that validates
deriveZswapCoinPublicKey() output against canonical Midnight zswap outputs: pick
several known secret keys (test vectors) produced/verified by Midnight (or from
ownPublicKey() in Midnight), compute expected public key bytes and assert
deriveZswapCoinPublicKey(secret) matches those pinned bytes; ensure the test
references the exact domain separator string "midnight:zswap-pk[v1]" and the
persistentHash-based derivation so any divergence in
ZswapCoinPublicKey/ZswapCoinSecretKey layout or hashing will fail CI.
---
Nitpick comments:
In `@contracts/src/token/FungibleToken.compact`:
- Around line 675-685: The documentation for Zswap coin public key derivation is
misleading: it states zswapCoinPublicKey =
persistentHash("midnight:zswap-pk[v1]", zswapCoinSecretKey) but the code
actually hashes a ZswapCoinPublicKeyPreimage struct that wraps the secret in a
ZswapCoinSecretKey before calling persistentHash; update the docstring for the
function that returns ZswapCoinPublicKey to describe the actual preimage layout
(mention ZswapCoinPublicKeyPreimage and ZswapCoinSecretKey) and show that
persistentHash("midnight:zswap-pk[v1]", ZswapCoinPublicKeyPreimage) is what is
computed so consumers can reproduce the exact struct encoding used by
persistentHash.
In `@contracts/src/token/test/simulators/FungibleTokenSimulator.ts`:
- Line 16: FungibleTokenSimulator defines a local type alias
"ZswapCoinPublicKey" which should be imported from the generated contract
artifact to stay consistent with other simulators; remove the local "type
ZswapCoinPublicKey = { bytes: Uint8Array }" and add an import of
ZswapCoinPublicKey from the contract's generated artifact in the
FungibleTokenSimulator file so all references use the artifact-provided type and
remain in sync with future regenerations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31f15169-aeff-4efc-ad8e-518c37d6e117
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
contracts/src/token/FungibleToken.compactcontracts/src/token/test/FungibleToken.test.tscontracts/src/token/test/mocks/MockFungibleToken.compactcontracts/src/token/test/simulators/FungibleTokenSimulator.tscontracts/src/token/witnesses/FungibleTokenWitnesses.tscontracts/src/token/witnesses/test/FungibleTokenWitnesses.test.tspackage.jsonpackages/compact/src/versions.tspackages/simulator/package.json
There was a problem hiding this comment.
Pull request overview
This PR refactors FungibleToken identity handling to restore ZswapCoinPublicKey-based balances while moving caller authentication to explicit Zswap secret-key-derived public key checks. It also updates simulators, mocks, witnesses, tests, and Compact/runtime versions to support the new ownership model.
Changes:
- Replaces fungible account identifiers with
ZswapCoinPublicKeyin token storage/API paths. - Adds explicit
zswapCoinSecretKeyinputs for caller-authenticated token circuits and derives public keys in-circuit. - Updates witness/private-state helpers, simulator/test infrastructure, and toolchain/runtime dependency versions.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
contracts/src/token/FungibleToken.compact |
Core token identity/auth refactor from account IDs to Zswap public keys. |
contracts/src/token/test/mocks/MockFungibleToken.compact |
Updates mock circuit signatures and forwarding to new token API. |
contracts/src/token/test/FungibleToken.test.ts |
Reworks fungible token tests around Zswap public keys and explicit secret-key auth. |
contracts/src/token/test/simulators/FungibleTokenSimulator.ts |
Updates simulator types and injects private-state Zswap secret keys into authenticated calls. |
contracts/src/token/witnesses/FungibleTokenWitnesses.ts |
Simplifies witness support to private-state key storage without caller-auth witnesses. |
contracts/src/token/witnesses/test/FungibleTokenWitnesses.test.ts |
Updates witness/private-state tests for the renamed Zswap secret key field and empty witnesses. |
packages/compact/src/versions.ts |
Bumps default Compact toolchain version. |
package.json |
Bumps root Compact runtime dependency. |
packages/simulator/package.json |
Bumps simulator Compact runtime dependency. |
yarn.lock |
Updates locked Midnight runtime packages and transitive runtime dependency versions. |
Comments suppressed due to low confidence (1)
contracts/src/token/FungibleToken.compact:460
- Correct the spelling of
receipienttorecipientin this updated parameter documentation.
* @param {Either<ZswapCoinPublicKey, ContractAddress>} to - The receipient of the transferred tokens.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| export pure circuit ZERO(): Either<Bytes<32>, ContractAddress> { | ||
| export pure circuit ZERO(): Either<ZswapCoinPublicKey, ContractAddress> { |
There was a problem hiding this comment.
Fixed. I exported ZswapCoinPublicKey explicitly from MockFungibleToken.compact and rebuilt the artifacts, so contracts/artifacts/MockFungibleToken/contract/index.d.ts now exposes export type ZswapCoinPublicKey = { bytes: Uint8Array }.
That lets the simulator and tests import the generated alias directly rather than duplicating the structural type.
| * @description Derives a Zswap coin public key from a Zswap coin secret key. | ||
| * @param {Bytes<32>} secretKey - A 32-byte cryptographically secure random value. | ||
| * @returns {Bytes<32>} accountId - The computed account identifier. | ||
| * @returns {Bytes<32>} publicKey - The derived Zswap coin public key. |
| * @param {Either<Bytes<32>, ContractAddress>} fromAddress - The owner of the tokens to transfer. | ||
| * @param {Either<Bytes<32>, ContractAddress>} to - The receipient of the transferred tokens. | ||
| * @param {Either<ZswapCoinPublicKey, ContractAddress>} fromAddress - The owner of the tokens to transfer. | ||
| * @param {Either<ZswapCoinPublicKey, ContractAddress>} to - The receipient of the transferred tokens. |
There was a problem hiding this comment.
Fixed. I corrected the parameter documentation spelling from receipient to recipient in both _transfer and _unsafeUncheckedTransfer.
| @@ -1,2 +1,2 @@ | |||
| export const COMPACT_VERSION = '0.29.0'; | |||
| export const COMPACT_VERSION = '0.31.0'; | |||
There was a problem hiding this comment.
Fixed. I synced the repo’s primary setup/docs surfaces to the new default Compact toolchain version:
README.mdpackages/compact/README.md.github/actions/setup/action.yml.devcontainer/devcontainer.json.github/ISSUE_TEMPLATE/01_bug_report.yml
I kept the update scoped to the files that directly advertise or install the default toolchain so the repo’s documented setup path now matches the wrapper default of 0.31.0.
| * @param {Bytes<32>} zswapCoinSecretKey - The caller's Zswap coin secret key. | ||
| * @returns {ZswapCoinPublicKey} zswapCoinPublicKey - The derived Zswap coin public key. | ||
| */ | ||
| export pure circuit computeAccountId(secretKey: Bytes<32>): Bytes<32> { | ||
| return persistentHash<Vector<1, Bytes<32>>>([secretKey]); | ||
| export pure circuit deriveZswapCoinPublicKey(zswapCoinSecretKey: Bytes<32>): ZswapCoinPublicKey { |
There was a problem hiding this comment.
Fixed. I restored the OpSec guidance on deriveZswapCoinPublicKey(...) and expanded the derivation docs to describe the actual ZswapCoinPublicKeyPreimage layout rather than the earlier shorthand.
The doc block now explicitly warns against logging/persisting the secret key, recommends secure randomness/storage, and calls out the risk of running the derivation in untrusted environments.
AndrewWestberg
force-pushed
the
amw/revert_to_zswap
branch
2 times, most recently
from
2e75c7b to
a92dfe8
Compare
AndrewWestberg
force-pushed
the
amw/revert_to_zswap
branch
from
a92dfe8 to
b950416
Compare
Summary
circuits.
Why
The upstream accountId = persistentHash(secretKey) refactor fixed a real security issue in the old ownPublicKey() flow, but it also changed the user-facing recipient identity away from
Midnight shielded addresses. This PR keeps the security fix while restoring ZswapCoinPublicKey-based addressing, so a caller must now prove ownership of the public key that holds funds by
supplying the corresponding secret key directly to the contract.
Key Changes
Security Impact
Testing
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyPR Checklist
Further comments
Summary by CodeRabbit
Refactor
Chores
@midnight-ntwrk/compact-runtimedependency from version 0.14.0 to 0.16.0.