Support for Confidential MPT (XLS-0096) - #3364
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds Confidential Multi-Purpose Token support across XRPL.js, including WASM-backed cryptography, codec definitions, transaction models and validators, confidential builders, optional loading, fee handling, amendment configuration, and unit/integration coverage. ChangesConfidential MPT implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| /** Free every allocation made through this marshaller. */ | ||
| public dispose(): void { | ||
| for (const ptr of this.ptrs) { | ||
| this.mod._free(ptr) |
There was a problem hiding this comment.
⚪ Severity: LOW
The dispose() method frees WASM heap allocations containing private keys and blinding factors without first zeroing the memory. Private key bytes (written via allocBytes in every proof/decrypt call) remain readable in the freed WASM heap until overwritten by later allocations, extending the window for key material extraction by same-process code (e.g., a compromised dependency reading HEAPU8).
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: To zero sensitive key material before freeing, the Marshaller must track the size of each allocation alongside its pointer. Change the ptrs field from number[] to an array of {ptr: number, size: number} tuples (line 45). Update alloc() (line 55) and allocBytes() (line 63) to push {ptr, size} instead of just ptr. Then, in dispose(), zero each region before freeing:
public dispose(): void {
for (const { ptr, size } of this.ptrs) {
this.mod.HEAPU8.fill(0, ptr, ptr + size)
this.mod._free(ptr)
}
this.ptrs.length = 0
}Specifically:
- Line 45:
private readonly ptrs: Array<{ptr: number, size: number}> = [] - Line 55:
this.ptrs.push({ptr, size}) - Line 63:
this.ptrs.push({ptr, size: data.length}) - Lines 132-137: Update
dispose()as shown above toHEAPU8.fill(0, ...)before_free().
This ensures private keys and blinding factors written via allocBytes are overwritten with zeros on the WASM heap before the memory is released back to the allocator.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/ripple-binary-codec/test/confidential-mpt.test.ts (1)
36-139: 💤 Low valueESLint func-names violations: consider using named function expressions.
The project's ESLint configuration requires named functions in test callbacks. All nine
it()and onedescribe()callback currently use anonymous functions, triggering thefunc-namesrule.🎨 Example fix for consistency
- it('round-trips ConfidentialMPTConvert (all fields)', function () { + it('round-trips ConfidentialMPTConvert (all fields)', function roundTripConfidentialMPTConvert() { assertRoundTrip({ TransactionType: 'ConfidentialMPTConvert',Apply the same pattern to the remaining eight tests and the
describe()block.🤖 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 `@packages/ripple-binary-codec/test/confidential-mpt.test.ts` around lines 36 - 139, The test callbacks currently use anonymous functions (describe(...) and each it(...)) which violates ESLint func-names; replace the anonymous callbacks with named function expressions (e.g., name the describe callback like function confidentialMptSuite() and each it callback with descriptive names such as function roundTripsConfidentialMPTConvert(), function roundTripsConfidentialMPTConvertBack(), function roundTripsConfidentialMPTSend(), function roundTripsConfidentialMPTClawback(), function roundTripsConfidentialMPTMergeInbox(), function roundTripsMPTokenLedgerEntryWithConfidentialFields(), function roundTripsMPTokenIssuanceLedgerEntryWithConfidentialFields(), and function decodesTecBadProofInTransactionMetadata()) so the symbols describe and the nine it calls keep the same behavior but satisfy func-names.packages/mpt-crypto/package.json (1)
26-31: 💤 Low valueConsider declaring devDependencies explicitly.
The scripts reference
tsc,jest, andeslint, but the package manifest does not declare these asdevDependencies. While they may be hoisted from the workspace root in a Lerna monorepo, explicitly declaring them improves clarity and ensures the package's tooling requirements are documented.📦 Suggested devDependencies block
"engines": { "node": ">= 18" - } + }, + "devDependencies": { + "typescript": "*", + "jest": "*", + "eslint": "*" + }🤖 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 `@packages/mpt-crypto/package.json` around lines 26 - 31, The package.json scripts ("build", "test", "lint", "clean") call tsc, jest and eslint but those tools are not declared in this package's devDependencies; add explicit devDependencies entries for at least typescript, jest, and eslint (and any companion types/test runtime packages you require such as `@types/jest` or ts-jest) to package.json so the package documents and installs its tooling requirements even if the workspace root hoists them.packages/xrpl/test/models/ConfidentialMPTConvert.test.ts (1)
1-172: ⚡ Quick winConsider adding test coverage for missing
IssuerEncryptedAmount.The test suite covers most required fields, but
IssuerEncryptedAmount(a required field per line 45 inConfidentialMPTConvert.ts) has no corresponding "missing field" test. While the existing coverage is reasonable, adding this test would ensure all required encryption fields are verified.✅ Suggested test case
+ it(`throws w/ missing IssuerEncryptedAmount`, function () { + assertInvalid( + { + TransactionType: 'ConfidentialMPTConvert', + Account: ACCOUNT, + MPTokenIssuanceID: MPT_ISSUANCE_ID, + MPTAmount: '100', + HolderEncryptedAmount: CIPHERTEXT, + BlindingFactor: BLINDING, + }, + 'ConfidentialMPTConvert: missing field IssuerEncryptedAmount', + ) + })🤖 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 `@packages/xrpl/test/models/ConfidentialMPTConvert.test.ts` around lines 1 - 172, Add a new test case in the ConfidentialMPTConvert suite that asserts a missing IssuerEncryptedAmount triggers the expected validation error; use the existing assertInvalid helper with a tx object containing TransactionType: 'ConfidentialMPTConvert', Account: ACCOUNT, MPTokenIssuanceID: MPT_ISSUANCE_ID, MPTAmount: '100', HolderEncryptedAmount: CIPHERTEXT, BlindingFactor: BLINDING (omit IssuerEncryptedAmount) and expect the message 'ConfidentialMPTConvert: missing field IssuerEncryptedAmount' to mirror the other "missing field" tests for consistency with validateConfidentialMPTConvert.
🤖 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 `@packages/mpt-crypto/package.json`:
- Around line 6-10: Add provenance documentation for the packaged WASM by
creating or updating a short README or inline comment that explains how
packages/mpt-crypto/wasm/mpt_crypto.wasm was produced or obtained: state the
upstream source/repo and commit/tag, the exact build command/tooling (e.g.,
Emscripten build command and environment), or the download URL and version, and
include integrity details such as a SHA256 checksum and any verification steps;
ensure this note is referenced from packages/mpt-crypto/src/module.ts (where the
Emscripten glue expects mpt_crypto.wasm) and that
packages/mpt-crypto/package.json's "files" list remains accurate.
In `@packages/mpt-crypto/src/marshal.ts`:
- Around line 52-57: The alloc function does not check for _malloc failure and
may use ptr==0; update alloc (and allocBytes) to verify const ptr =
this.mod._malloc(size) is non-zero before using it, throw or return a clear
error if ptr === 0, and avoid calling this.mod.HEAPU8.fill or pushing ptr into
this.ptrs when allocation failed (referencing the alloc and allocBytes methods,
this.mod._malloc, this.mod.HEAPU8.fill, and this.ptrs to locate the changes).
In `@packages/mpt-crypto/src/module.ts`:
- Line 124: The call to instance._mpt_secp256k1_context() returns a numeric
status that must be validated; update the code where
instance._mpt_secp256k1_context() is invoked to capture its return value, check
for non-zero (failure) and throw or return an error (or call process exit) when
initialization fails, following the same pattern used in context.ts (check
return === 0 for success). Ensure the check references the exact function name
_mpt_secp256k1_context and provides a clear error message denoting secp256k1
context initialization failure.
In `@packages/ripple-binary-codec/src/enums/definitions.json`:
- Around line 3492-3500: The BlindingFactor field currently uses nth: 39 which
collides with ReferenceHolding's nth: 39 for the Hash256 type; update
BlindingFactor's nth to the next unused ordinal (e.g., 40) in definitions.json
so each Hash256 field has a unique nth, ensuring the FieldLookup key
((Hash256_typeOrdinal << 16) | nth) no longer collides between BlindingFactor
and ReferenceHolding.
---
Nitpick comments:
In `@packages/mpt-crypto/package.json`:
- Around line 26-31: The package.json scripts ("build", "test", "lint", "clean")
call tsc, jest and eslint but those tools are not declared in this package's
devDependencies; add explicit devDependencies entries for at least typescript,
jest, and eslint (and any companion types/test runtime packages you require such
as `@types/jest` or ts-jest) to package.json so the package documents and installs
its tooling requirements even if the workspace root hoists them.
In `@packages/ripple-binary-codec/test/confidential-mpt.test.ts`:
- Around line 36-139: The test callbacks currently use anonymous functions
(describe(...) and each it(...)) which violates ESLint func-names; replace the
anonymous callbacks with named function expressions (e.g., name the describe
callback like function confidentialMptSuite() and each it callback with
descriptive names such as function roundTripsConfidentialMPTConvert(), function
roundTripsConfidentialMPTConvertBack(), function
roundTripsConfidentialMPTSend(), function roundTripsConfidentialMPTClawback(),
function roundTripsConfidentialMPTMergeInbox(), function
roundTripsMPTokenLedgerEntryWithConfidentialFields(), function
roundTripsMPTokenIssuanceLedgerEntryWithConfidentialFields(), and function
decodesTecBadProofInTransactionMetadata()) so the symbols describe and the nine
it calls keep the same behavior but satisfy func-names.
In `@packages/xrpl/test/models/ConfidentialMPTConvert.test.ts`:
- Around line 1-172: Add a new test case in the ConfidentialMPTConvert suite
that asserts a missing IssuerEncryptedAmount triggers the expected validation
error; use the existing assertInvalid helper with a tx object containing
TransactionType: 'ConfidentialMPTConvert', Account: ACCOUNT, MPTokenIssuanceID:
MPT_ISSUANCE_ID, MPTAmount: '100', HolderEncryptedAmount: CIPHERTEXT,
BlindingFactor: BLINDING (omit IssuerEncryptedAmount) and expect the message
'ConfidentialMPTConvert: missing field IssuerEncryptedAmount' to mirror the
other "missing field" tests for consistency with validateConfidentialMPTConvert.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf049123-1b64-4467-9fdf-db79775c67d5
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/mpt-crypto/wasm/mpt_crypto.wasmis excluded by!**/*.wasm
📒 Files selected for processing (59)
lerna.jsonpackages/mpt-crypto/README.mdpackages/mpt-crypto/eslint.config.jspackages/mpt-crypto/package.jsonpackages/mpt-crypto/src/constants.tspackages/mpt-crypto/src/context.tspackages/mpt-crypto/src/hex.tspackages/mpt-crypto/src/index.tspackages/mpt-crypto/src/internal.tspackages/mpt-crypto/src/marshal.tspackages/mpt-crypto/src/module.tspackages/mpt-crypto/src/primitives.tspackages/mpt-crypto/src/proofs.tspackages/mpt-crypto/src/runtime.tspackages/mpt-crypto/src/types.tspackages/mpt-crypto/tsconfig.build.jsonpackages/mpt-crypto/tsconfig.eslint.jsonpackages/mpt-crypto/tsconfig.jsonpackages/mpt-crypto/wasm/mpt_crypto.jspackages/ripple-binary-codec/HISTORY.mdpackages/ripple-binary-codec/src/enums/definitions.jsonpackages/ripple-binary-codec/test/confidential-mpt.test.tspackages/xrpl/HISTORY.mdpackages/xrpl/package.jsonpackages/xrpl/src/confidential/convert.tspackages/xrpl/src/confidential/index.tspackages/xrpl/src/confidential/ledger.tspackages/xrpl/src/confidential/loader.tspackages/xrpl/src/confidential/transfer.tspackages/xrpl/src/confidential/types.tspackages/xrpl/src/models/ledger/MPToken.tspackages/xrpl/src/models/ledger/MPTokenIssuance.tspackages/xrpl/src/models/transactions/ConfidentialMPTClawback.tspackages/xrpl/src/models/transactions/ConfidentialMPTConvert.tspackages/xrpl/src/models/transactions/ConfidentialMPTConvertBack.tspackages/xrpl/src/models/transactions/ConfidentialMPTMergeInbox.tspackages/xrpl/src/models/transactions/ConfidentialMPTSend.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/MPTokenIssuanceSet.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/index.tspackages/xrpl/src/models/transactions/transaction.tspackages/xrpl/src/sugar/autofill.tspackages/xrpl/test/client/autofill.test.tspackages/xrpl/test/integration/confidentialMPTUtils.tspackages/xrpl/test/integration/transactions/confidentialMPTClawback.test.tspackages/xrpl/test/integration/transactions/confidentialMPTConvert.test.tspackages/xrpl/test/integration/transactions/confidentialMPTConvertBack.test.tspackages/xrpl/test/integration/transactions/confidentialMPTLifecycle.test.tspackages/xrpl/test/integration/transactions/confidentialMPTMergeInbox.test.tspackages/xrpl/test/integration/transactions/confidentialMPTSend.test.tspackages/xrpl/test/models/ConfidentialMPTClawback.test.tspackages/xrpl/test/models/ConfidentialMPTConvert.test.tspackages/xrpl/test/models/ConfidentialMPTConvertBack.test.tspackages/xrpl/test/models/ConfidentialMPTMergeInbox.test.tspackages/xrpl/test/models/ConfidentialMPTSend.test.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.tspackages/xrpl/test/models/MPTokenIssuanceSet.test.tspackages/xrpl/tsconfig.build.json
| "files": [ | ||
| "dist/*", | ||
| "wasm/*", | ||
| "src/*" | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for the mpt_crypto.wasm binary in the package
fd -t f 'mpt_crypto.wasm' packages/mpt-crypto/Repository: XRPLF/xrpl.js
Length of output: 98
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WASM files =="
ls -la packages/mpt-crypto/wasm || true
echo
echo "== References to mpt_crypto.wasm =="
rg -n "mpt_crypto\.wasm" -S packages/mpt-crypto || true
echo
echo "== WASM provenance/build docs (best-effort search) =="
rg -n "(build|builds|download|provenance|source|compile|generated|toolchain|wasm)" -S packages/mpt-crypto || trueRepository: XRPLF/xrpl.js
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/mpt-crypto/src/module.ts around locateFile / wasm comment =="
sed -n '80,140p' packages/mpt-crypto/src/module.ts
echo
echo "== Search for any explicit build/download provenance keywords =="
rg -n "(vendored|download|build|compile|emcc|emscripten|source|provenance|generated)" packages/mpt-crypto || true
echo
echo "== Files array in packages/mpt-crypto/package.json =="
cat packages/mpt-crypto/package.json
echo
echo "== Any README/docs mentioning wasm =="
fd -t f -e md -e txt packages/mpt-crypto | xargs -r rg -n "wasm|mpt_crypto\.wasm" || trueRepository: XRPLF/xrpl.js
Length of output: 50769
Document the provenance of mpt_crypto.wasm (binary is present and packaged)
packages/mpt-crypto/wasm/mpt_crypto.wasmexists and is covered bypackages/mpt-crypto/package.json’s"files": ["wasm/*", ...].packages/mpt-crypto/src/module.tsnotes the Emscripten glue locatesmpt_crypto.wasmnext tompt_crypto.js, but it doesn’t explain how the WASM was built/downloaded.- Add a short README/comment describing the WASM’s provenance (build command/tooling and source it comes from, or download process/checksums).
🤖 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 `@packages/mpt-crypto/package.json` around lines 6 - 10, Add provenance
documentation for the packaged WASM by creating or updating a short README or
inline comment that explains how packages/mpt-crypto/wasm/mpt_crypto.wasm was
produced or obtained: state the upstream source/repo and commit/tag, the exact
build command/tooling (e.g., Emscripten build command and environment), or the
download URL and version, and include integrity details such as a SHA256
checksum and any verification steps; ensure this note is referenced from
packages/mpt-crypto/src/module.ts (where the Emscripten glue expects
mpt_crypto.wasm) and that packages/mpt-crypto/package.json's "files" list
remains accurate.
| const factory: ModuleFactory = require('../wasm/mpt_crypto') | ||
| const instance = await factory() | ||
| // Force one-time initialization of the shared secp256k1 context. | ||
| instance._mpt_secp256k1_context() |
There was a problem hiding this comment.
Validate the secp256k1 context initialization return code.
The _mpt_secp256k1_context() function returns a number (per the WasmModule interface), but the return value is not checked. Following the pattern used elsewhere (e.g., context.ts lines 30-33), WASM functions return 0 on success and non-zero on failure. If initialization fails, subsequent cryptographic operations may produce incorrect results or crash.
🛡️ Proposed fix to check initialization status
const factory: ModuleFactory = require('../wasm/mpt_crypto')
const instance = await factory()
// Force one-time initialization of the shared secp256k1 context.
- instance._mpt_secp256k1_context()
+ const status = instance._mpt_secp256k1_context()
+ if (status !== 0) {
+ throw new Error('secp256k1 context initialization failed')
+ }
return instance📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| instance._mpt_secp256k1_context() | |
| const factory: ModuleFactory = require('../wasm/mpt_crypto') | |
| const instance = await factory() | |
| // Force one-time initialization of the shared secp256k1 context. | |
| const status = instance._mpt_secp256k1_context() | |
| if (status !== 0) { | |
| throw new Error('secp256k1 context initialization failed') | |
| } | |
| return instance |
🤖 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 `@packages/mpt-crypto/src/module.ts` at line 124, The call to
instance._mpt_secp256k1_context() returns a numeric status that must be
validated; update the code where instance._mpt_secp256k1_context() is invoked to
capture its return value, check for non-zero (failure) and throw or return an
error (or call process exit) when initialization fails, following the same
pattern used in context.ts (check return === 0 for success). Ensure the check
references the exact function name _mpt_secp256k1_context and provides a clear
error message denoting secp256k1 context initialization failure.
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1)
174-178: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject
AuditorEncryptionKeyunlessIssuerEncryptionKeyis also set.
This only checks the auditor key’s format; add the cross-field guard so client validation rejects malformed transactions that rippled will reject.🤖 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 `@packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts` around lines 174 - 178, Update the validation flow containing validateOptionalField for AuditorEncryptionKey to require IssuerEncryptionKey whenever the auditor key is provided. Preserve the existing AuditorEncryptionKey format validation, and add the cross-field guard so transactions with an auditor key but no issuer key are rejected.
🤖 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 `@packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts`:
- Around line 222-233: Update the mutation classification in the MPToken
issuance validation flow: include IssuerEncryptionKey and AuditorEncryptionKey
in isMutate, then reuse isMutate for the Holder exclusion so confidential key
updates cannot be combined with Holder. Remove the separate
isSetConfidentialKeys condition while preserving the existing no-op checks.
---
Outside diff comments:
In `@packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts`:
- Around line 174-178: Update the validation flow containing
validateOptionalField for AuditorEncryptionKey to require IssuerEncryptionKey
whenever the auditor key is provided. Preserve the existing AuditorEncryptionKey
format validation, and add the cross-field guard so transactions with an auditor
key but no issuer key are rejected.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eec64fba-05ae-48fd-b17a-3b94cd5b6a22
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
packages/ripple-binary-codec/HISTORY.mdpackages/ripple-binary-codec/src/enums/definitions.jsonpackages/xrpl/HISTORY.mdpackages/xrpl/package.jsonpackages/xrpl/src/models/ledger/MPTokenIssuance.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/MPTokenIssuanceSet.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/index.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.tspackages/xrpl/test/models/MPTokenIssuanceSet.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
- packages/xrpl/test/models/MPTokenIssuanceSet.test.ts
- packages/xrpl/src/models/transactions/index.ts
- packages/xrpl/HISTORY.md
- packages/xrpl/src/models/ledger/MPTokenIssuance.ts
- packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
- packages/ripple-binary-codec/HISTORY.md
- packages/xrpl/src/models/transactions/common.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1)
174-178: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject
AuditorEncryptionKeyunlessIssuerEncryptionKeyis also set.
This only checks the auditor key’s format; add the cross-field guard so client validation rejects malformed transactions that rippled will reject.🤖 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 `@packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts` around lines 174 - 178, Update the validation flow containing validateOptionalField for AuditorEncryptionKey to require IssuerEncryptionKey whenever the auditor key is provided. Preserve the existing AuditorEncryptionKey format validation, and add the cross-field guard so transactions with an auditor key but no issuer key are rejected.
🤖 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 `@packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts`:
- Around line 222-233: Update the mutation classification in the MPToken
issuance validation flow: include IssuerEncryptionKey and AuditorEncryptionKey
in isMutate, then reuse isMutate for the Holder exclusion so confidential key
updates cannot be combined with Holder. Remove the separate
isSetConfidentialKeys condition while preserving the existing no-op checks.
---
Outside diff comments:
In `@packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts`:
- Around line 174-178: Update the validation flow containing
validateOptionalField for AuditorEncryptionKey to require IssuerEncryptionKey
whenever the auditor key is provided. Preserve the existing AuditorEncryptionKey
format validation, and add the cross-field guard so transactions with an auditor
key but no issuer key are rejected.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eec64fba-05ae-48fd-b17a-3b94cd5b6a22
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
packages/ripple-binary-codec/HISTORY.mdpackages/ripple-binary-codec/src/enums/definitions.jsonpackages/xrpl/HISTORY.mdpackages/xrpl/package.jsonpackages/xrpl/src/models/ledger/MPTokenIssuance.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/MPTokenIssuanceSet.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/index.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.tspackages/xrpl/test/models/MPTokenIssuanceSet.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
- packages/xrpl/test/models/MPTokenIssuanceSet.test.ts
- packages/xrpl/src/models/transactions/index.ts
- packages/xrpl/HISTORY.md
- packages/xrpl/src/models/ledger/MPTokenIssuance.ts
- packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
- packages/ripple-binary-codec/HISTORY.md
- packages/xrpl/src/models/transactions/common.ts
🛑 Comments failed to post (1)
packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1)
222-233: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '180,260p' packages/xrpl/src/models/transactions/MPTokenIssuanceSet.tsRepository: XRPLF/xrpl.js
Length of output: 3043
🏁 Script executed:
rg -n "isSetConfidentialKeys|IssuerEncryptionKey|AuditorEncryptionKey|MutableFlags|Holder|confidential" packages/xrpl/src -SRepository: XRPLF/xrpl.js
Length of output: 18723
🏁 Script executed:
sed -n '1,180p' packages/xrpl/src/models/transactions/MPTokenIssuanceSet.tsRepository: XRPLF/xrpl.js
Length of output: 6898
🏁 Script executed:
rg -n "Holder field is not allowed when mutating MPTokenIssuance|Cannot set both DomainID and Holder fields|IssuerEncryptionKey|AuditorEncryptionKey|MPTokenIssuanceSet" packages/xrpl/src test tests -SRepository: XRPLF/xrpl.js
Length of output: 11512
🏁 Script executed:
git ls-files | rg 'MPTokenIssuanceSet|confidential|XLS|mptoken'Repository: XRPLF/xrpl.js
Length of output: 1709
🏁 Script executed:
rg -n "MPTokenIssuanceSet|IssuerEncryptionKey|AuditorEncryptionKey|Holder field is not allowed when mutating|Transaction does not change the state" . -SRepository: XRPLF/xrpl.js
Length of output: 50370
🏁 Script executed:
wc -l packages/xrpl/test/models/MPTokenIssuanceSet.test.ts packages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 299
🏁 Script executed:
ast-grep outline packages/xrpl/test/models/MPTokenIssuanceSet.test.ts --view expandedRepository: XRPLF/xrpl.js
Length of output: 363
🏁 Script executed:
rg -n "Holder.*IssuerEncryptionKey|Holder.*AuditorEncryptionKey|IssuerEncryptionKey.*Holder|AuditorEncryptionKey.*Holder|Holder field is not allowed|mutating MPTokenIssuance|Transaction does not change the state" packages/xrpl/test/models/MPTokenIssuanceSet.test.ts packages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.ts -SRepository: XRPLF/xrpl.js
Length of output: 917
🏁 Script executed:
sed -n '280,390p' packages/xrpl/test/models/MPTokenIssuanceSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 3570
🏁 Script executed:
sed -n '1,260p' packages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 8835
🏁 Script executed:
rg -n "IssuerEncryptionKey|AuditorEncryptionKey|Holder" packages/xrpl/test/integration/transactions/mptokenIssuanceSet.test.ts packages/xrpl/test/models/MPTokenIssuanceSet.test.ts -SRepository: XRPLF/xrpl.js
Length of output: 1901
🏁 Script executed:
rg -n "Holder.*IssuerEncryptionKey|Holder.*AuditorEncryptionKey|IssuerEncryptionKey.*Holder|AuditorEncryptionKey.*Holder|confidential keys|registered so confidential amounts can be encrypted" packages/xrpl/docs packages/xrpl/src packages/xrpl/test -SRepository: XRPLF/xrpl.js
Length of output: 587
🏁 Script executed:
sed -n '120,170p' packages/xrpl/test/models/MPTokenIssuanceSet.test.tsRepository: XRPLF/xrpl.js
Length of output: 1722
Treat confidential key updates as mutations.
IssuerEncryptionKey/AuditorEncryptionKeyalready count as state changes in the no-op check, but they’re excluded fromisMutate, soHoldercan still be combined with them. Fold both keys intoisMutateand reuse it for the holder exclusion.🤖 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 `@packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts` around lines 222 - 233, Update the mutation classification in the MPToken issuance validation flow: include IssuerEncryptionKey and AuditorEncryptionKey in isMutate, then reuse isMutate for the Holder exclusion so confidential key updates cannot be combined with Holder. Remove the separate isSetConfidentialKeys condition while preserving the existing no-op checks.
| // value, so cost scales with the amount, not the window. These are the library's | ||
| // recommended defaults (secp256k1_mpt.h): [0, 1_000_000]. High must be < UINT64_MAX. | ||
| const DECRYPT_RANGE_LOW = 0n | ||
| const DECRYPT_RANGE_HIGH = 1_000_000n |
There was a problem hiding this comment.
⚪ Severity: LOW
The ElGamal decryption range is hardcoded to [0, 1_000_000] with no way for callers to override it. The decryptAmount function is called internally by prepareConfidentialConvertBack, prepareConfidentialSend, and prepareConfidentialClawback to recover the balance witness needed for proof generation. If a holder's confidential balance exceeds 1,000,000 (possible since issuances allow MaximumAmount up to max int64), decryption fails and those operations become impossible — effectively locking the holder's funds in the confidential balance with no client-side recovery path.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Make the decrypt range configurable by adding optional rangeLow and rangeHigh parameters to the decryptAmount function (around line 79), using the current constants as defaults. For example, change the signature to:
export async function decryptAmount(
ciphertext: string,
privateKey: string,
rangeLow: bigint = DECRYPT_RANGE_LOW,
rangeHigh: bigint = DECRYPT_RANGE_HIGH,
): Promise<bigint> {Then pass rangeLow and rangeHigh (instead of the hardcoded constants) to mod._mpt_decrypt_amount at lines 94-95. You should also re-export these default constants so callers are aware of the defaults. Finally, update the callers in transfer.ts, convert.ts, and ledger.ts to optionally accept and forward range parameters if needed.
kuan121
changed the title
- rename tfMPTCanConfidentialAmount -> tfMPTCanHoldConfidentialBalance - add confidential MutableFlags (Set 0x40, Create 0x80) + lsf/lsmf ledger flags - validate exact ZKProof sizes (Convert 64, ConvertBack 816, Send 946, Clawback 64) - validate MPTAmount range/zero (Convert allows 0; ConvertBack/Clawback reject 0) - enforce Convert HolderEncryptionKey<->ZKProof and Set Auditor->Issuer key pairing - pair ZKProof with HolderEncryptionKey in the Convert builder (temMALFORMED) - null-check WASM _malloc; document vendored WASM provenance
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| async function main() { | ||
| console.log(`Confidential MPT lifecycle on ${SERVER}\n`) |
| * @returns A confidential keypair: a 33-byte hex `publicKey` and the bare | ||
| * 32-byte hex `privateKey` scalar. | ||
| */ | ||
| export function deriveConfidentialKeypair(seed?: string): ConfidentialKeypair { |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
The documentation explicitly encourages reusing the account's signing seed ("including an account's own signing seed"). For secp256k1 wallets, Wallet.fromSeed(seed) and deriveConfidentialKeypair(seed) both call deriveKeypair(seed, { algorithm: secp256k1 }), producing the same private key scalar. This signing key is then passed into the unverified WASM binary for ElGamal decryption and proof generation across multiple builder functions, expanding the signing key's attack surface beyond the signing library to include the opaque WASM module.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: This is a cryptographic key separation concern. To properly address it:
-
Update the JSDoc (lines 16-18) to remove the encouragement to reuse the account's signing seed. Instead, warn users that reusing the signing seed expands its attack surface to include the WASM module and recommend using a distinct seed for confidential keys.
-
Ideally, add domain separation in the derivation: even when the same seed is provided, derive a distinct key for confidential use by applying an additional key derivation step (e.g., HMAC-based domain separation such as
HMAC-SHA512('confidential-mpt', rawPrivateKey)before using the scalar). This ensures the signing key and the confidential key are cryptographically independent even when derived from the same seed, following the principle of key separation across protocols. -
At minimum, if domain separation is not feasible due to cross-SDK compatibility constraints, update the documentation to strongly discourage reusing the signing seed and document the risk:
* ⚠️ For best security, use a **dedicated** seed for confidential keys rather
* than reusing your account's signing seed. Reusing the signing seed extends
* the private key's trust boundary to the WASM proof-generation module.
| * Enables the lsfMPTCanHoldConfidentialBalance flag on the issuance. Allows | ||
| * holders to hold confidential (encrypted) balances of this token. | ||
| */ | ||
| tfMPTSetCanHoldConfidentialBalance = 0x00000100, |
There was a problem hiding this comment.
@kuan121 should tfMPTSetCanHoldConfidentialBalance be part of the mutable flags instead?
https://xls.xrpl.org/xls/XLS-0096-confidential-mpt.html#12-transaction-mptokenissuanceset
section 12.2.1
There was a problem hiding this comment.
@florent-uzio As part of the latest Dynamic MPT update, we no longer have mutable flags. See this PR to update the XLS spec for Dynamic MPT. I included only tfMPTSetCanHoldConfidentialBalance here since it's relevant to Confidential MPT.
3 participants
High Level Overview of Change
This PR adds support for Confidential Multi-Purpose Tokens (XLS-0096).
Confidential MPT represents an MPT balance on-ledger as EC-ElGamal ciphertexts and uses zero-knowledge proofs, allowing validators to verify transfers (no overdraft, conservation of value) without learning the amounts. This change adds the wire-format definitions, the transaction and ledger models, field validation, fee handling, and a high-level builder API that encapsulates the cryptography. All changes are additive and do not alter existing behavior.
Components:
@xrplf/mpt-crypto(new package). A hex-in/hex-out TypeScript API over a vendored WebAssembly build of the reference C crypto library, providing ElGamal encryption, Pedersen commitments, blinding factors, context hashes, and the four proof builders (convert, convert-back, send, clawback). It is an optional peer dependency ofxrpl, loaded lazily, and is intended to be published to the npm registry.ripple-binary-codec. Definitions for the five new transaction types (ConfidentialMPTConvert,MergeInbox,ConvertBack,Send,Clawback), their fields, and thetecBAD_PROOFandtemBAD_CIPHERTEXTresult codes.xrpl). The fiveConfidentialMPT*interfaces andvalidate*functions, thetfMPTCanConfidentialAmountflag onMPTokenIssuanceCreate, and theIssuerEncryptionKeyandAuditorEncryptionKeyfields onMPTokenIssuanceSet.xrpl). Confidential fields onMPToken(inbox, spending, and version balances; issuer- and auditor-encrypted balances; holder encryption key) and onMPTokenIssuance(issuer and auditor keys, outstanding amount).xrpl/confidential). TheprepareConfidential*builders, which read the required ledger state, generate the ciphertexts, commitments, and proofs, and return an unsigned transaction.getConfidentialBalancedecrypts a holder's spendable balance.autofill). Confidential transactions carry a surcharge; the autofilled fee isbase × (10 + signerCount), matching rippled.Context of Change
This implements the client side of XLS-0096, tracking rippled PR #5860 and the xrpl-py reference implementation (branch
confidential-mpt).Key design decisions:
@xrplf/mpt-cryptoas an optional peer dependency, loaded lazily by thexrpl/confidentialsubpath, ensures thatxrplusers who do not use Confidential MPT incur no additional dependency or load cost.ripple-keypairs. No new key type is introduced, the key is recoverable from a seed, and the approach is consistent with the Java SDK (xrpl4j).Sequence, which must not be changed before submission.mpt-cryptoversion pinned by rippled; a mismatch results intecBAD_PROOF.Type of Change
Did you update HISTORY.md?
Test Plan
Unit, codec, and fee tests — run in CI with no additional setup (
npm test):packages/xrpl/test/models/ConfidentialMPT*.test.tsand theMPTokenIssuanceCreate/MPTokenIssuanceSetadditions: eachvalidate*function accepts well-formed transactions and rejects malformed fields.packages/ripple-binary-codec/test/confidential-mpt.test.ts: encode/decode round-trips for the new transaction types and fields.packages/xrpl/test/client/autofill.test.ts: the confidential fee calculation.Integration tests —
packages/xrpl/test/integration/transactions/confidentialMPT*.test.ts: one file per transaction type plus a four-party lifecycle file, sharingconfidentialMPTUtils.ts. These require a rippled with the MPTokensV1, Clawback, and ConfidentialTransfer amendments enabled. They run as part ofnpm run test:integrationagainst therippleci/xrplddevelop image once #5860 is merged, and were verified against a local standalone rippled built from #5860: all five transaction types and the full four-party lifecycle (convert → merge → send → convert-back → clawback, with auditor disclosure) pass.