MultiToken refactor -> replace pk with sk#483
Merged
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 MultiToken contract refactors its authorization model from public-key-based identity to witness-derived secret-key-based identity using persistent hashing. Ledger key types change from ChangesMultiToken Authorization Model Refactoring
Sequence DiagramsequenceDiagram
participant Caller
participant Circuit as MultiToken<br/>Circuit
participant PrivateState as Witness Context<br/>Private State
participant Ledger as Ledger Maps<br/>(_balances,<br/>_operatorApprovals)
rect rgba(220, 100, 100, 0.5)
Note over Caller,PrivateState: Old Model (Public Key)
Caller->>Circuit: transferFrom(pubKeyEither, ...)
Circuit->>Circuit: Use pubKey directly<br/>for authorization
Circuit->>Ledger: Update ledger with<br/>pubKey variant
end
rect rgba(100, 150, 220, 0.5)
Note over Caller,PrivateState: New Model (Secret Key)
Caller->>PrivateState: injectSecretKey(sk)
PrivateState->>PrivateState: Store secretKey<br/>(32 bytes)
Caller->>Circuit: transferFrom(accountIdEither, ...)
Circuit->>PrivateState: wit_MultiTokenSK()
PrivateState-->>Circuit: Return secretKey
Circuit->>Circuit: Derive accountId from secretKey<br/>via persistentHash()
Circuit->>Circuit: Canonicalize all<br/>Either<Bytes<32>, CA> values
Circuit->>Ledger: Lookup/update ledger<br/>with canonical identity
Circuit-->>Caller: Authorization complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contracts/src/token/test/simulators/MultiTokenSimulator.ts (1)
277-283: 💤 Low valueThe
undefinedcheck is redundant given type guarantees.Since
MultiTokenPrivateState.generate()andMultiTokenPrivateState.withSecretKey()always produce a definedsecretKey, and the type system enforcessecretKey: Uint8Array, this condition can never be true at runtime. Consider simplifying:Suggested simplification
getCurrentSecretKey: (): Uint8Array => { const sk = this.getPrivateState().secretKey; - if (typeof sk === 'undefined') { - throw new Error('Missing secret key'); - } return Uint8Array.from(sk); },If you prefer keeping defensive validation, use a more idiomatic check that TypeScript won't flag as unnecessary:
if (!sk || sk.length !== 32) { throw new Error('Invalid or missing secret key'); }🤖 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/MultiTokenSimulator.ts` around lines 277 - 283, The typeof undefined check in getCurrentSecretKey is redundant because MultiTokenPrivateState.generate() and MultiTokenPrivateState.withSecretKey() guarantee a defined secretKey; remove the if (typeof sk === 'undefined') branch and simply return Uint8Array.from(sk) directly. If you want defensive validation instead, replace the redundant check with an idiomatic runtime assertion such as verifying sk exists and has the expected length (e.g., if (!sk || sk.length !== 32) throw new Error('Invalid or missing secret key')) while keeping the rest of getCurrentSecretKey unchanged; refer to getCurrentSecretKey, MultiTokenPrivateState.generate, MultiTokenPrivateState.withSecretKey, and the secretKey field to locate the code.
🤖 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.
Nitpick comments:
In `@contracts/src/token/test/simulators/MultiTokenSimulator.ts`:
- Around line 277-283: The typeof undefined check in getCurrentSecretKey is
redundant because MultiTokenPrivateState.generate() and
MultiTokenPrivateState.withSecretKey() guarantee a defined secretKey; remove the
if (typeof sk === 'undefined') branch and simply return Uint8Array.from(sk)
directly. If you want defensive validation instead, replace the redundant check
with an idiomatic runtime assertion such as verifying sk exists and has the
expected length (e.g., if (!sk || sk.length !== 32) throw new Error('Invalid or
missing secret key')) while keeping the rest of getCurrentSecretKey unchanged;
refer to getCurrentSecretKey, MultiTokenPrivateState.generate,
MultiTokenPrivateState.withSecretKey, and the secretKey field to locate the
code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1241a929-f484-4b87-ab49-75960f804a14
📒 Files selected for processing (6)
contracts/src/token/MultiToken.compactcontracts/src/token/test/MultiToken.test.tscontracts/src/token/test/mocks/MockMultiToken.compactcontracts/src/token/test/simulators/MultiTokenSimulator.tscontracts/src/token/witnesses/MultiTokenWitnesses.tscontracts/src/token/witnesses/test/MultiTokenWitnesses.test.ts
0xisk
reviewed
May 13, 2026
Member
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
Thank you @andrew-fleming! Left one blocking comment.
Member
There was a problem hiding this comment.
🔵 followup: Redundant canonicalization through _unsafeTransferFrom → _unsafeTransfer → _update
_unsafeTransferFromL446 passes the already-canonicalcanonFromto_unsafeTransfer._unsafeTransferL481-483 hands it to_update._updateL379-380 canonicalizes again.
| if (!Utils_isKeyOrAddressZero(disclose(fromAddress))) { | ||
| const fromBalance = balanceOf(fromAddress, id); | ||
| if (!_isTargetZero(disclose(canonFrom))) { | ||
| const fromBalance = balanceOf(canonFrom, id); |
Member
There was a problem hiding this comment.
🔵 followup: _update re-enters exported balanceOf instead of a raw helper
_updateL383, L397- The public
balanceOfL211-222 re-asserts init and re-canonicalizes already-canonical input on each call.
| const MAX_UINT128 = 340282366920938463463374607431768211455; | ||
| assert(MAX_UINT128 - toBalance >= value, "MultiToken: arithmetic overflow"); | ||
| _balances.lookup(id).insert(disclose(to), disclose(toBalance + value as Uint<128>)); | ||
| _balances.lookup(id).insert(disclose(canonTo), disclose(toBalance + value as Uint<128>)); |
Member
There was a problem hiding this comment.
🔵 followup: Redundant as Uint<128> casts in _update
Comment on lines
+12
to
+15
| const buildAccountIdHash = (sk: Uint8Array): Uint8Array => { | ||
| const rt_type = new CompactTypeVector(1, new CompactTypeBytes(32)); | ||
| return persistentHash(rt_type, [sk]); | ||
| }; |
Member
There was a problem hiding this comment.
🔵 followup: that should mirror the circuit hasher so lets leave a comment to highlight that.
0xisk
approved these changes
May 13, 2026
Member
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
LGTM! thank you @andrew-fleming !
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
2 participants
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.
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #455
Summary by CodeRabbit