refactor: ConfidentialMPT tx-building helpers in MPTTester

[PeterChen13579]

High Level Overview of Change

fixes: #7630

Context of Change

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR refactors the Confidential MPT (MPToken) transaction-building logic used by MPTTester test utilities, reducing duplicated JSON-field/proof assembly code and making the test helpers more consistent across ConfidentialMPT transaction types.

Changes:

• Added small internal helper functions to set common JSON fields (account, destination, issuance ID) and to attach generated/dummy proofs.
• Refactored convert, send, mergeInbox, and convertBack flows to build transactions via *JV(...) helpers plus shared sequencing logic.
• Simplified getEncryptedBalance by centralizing field selection and VL extraction.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The remaining findings are limited to correcting misleading test comments and do not affect correctness or behavior.

Review details

Suppressed comments (1)

src/test/app/ConfidentialTransfer_test.cpp:5996

• Same issue as above: this comment claims mpt_get_convert_back_proof rejects a claimed balance below the amount being converted, but here claimedBalance is still >= amt. The forgery is needed to bypass the helper's client-side linkage validation (claimed balance vs commitment/ciphertext) so the test can reach the ledger's balance-linkage check.

        // mpt_get_convert_back_proof refuses to build a proof with a claimed
        // balance below the amount being converted, so we forge the sigma
        // proof directly against the real commitment/ciphertext (mirroring
        // testConvertBackOverdraftBulletproofImpl) to exercise the ledger's
        // balance-linkage check instead of that client-side guard.

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

Changes are isolated to test/helpers refactors with targeted test updates, and the remaining feedback is limited to minor documentation/comment correctness.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/test/jtx/impl/mpt.cpp:1569

• This comment no longer matches the condition: proof generation is skipped not only when the spending balance is 0, but also when the encrypted spending balance is missing (prevEncryptedSenderSpending is nullopt).

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new proof-forging helper has unchecked buffer-size/underflow hazards and the getConvertBackProof API change leaves at least one verified caller path that can silently alter test intent unless updated.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/test/jtx/mpt.h:712

• getConvertBackProof now returns std::optional<Buffer> (nullopt on failure). Any remaining call sites that pass the return value straight into .proof without checking has_value() can silently change behavior (e.g., falling back to auto-generated proofs). For example, src/test/app/ConfidentialTransferExtended_test.cpp:2338 uses the return value directly. Those callers should assert/require a value (or explicitly provide a dummy proof) to preserve test intent.

    [[nodiscard]] std::optional<Buffer>
    getConvertBackProof(
        Account const& holder,
        std::uint64_t const amount,
        uint256 const& contextHash,
        PedersenProofParams const& pcParams) const;

src/test/jtx/ConfidentialTransfer.h:166

• pedersenCommitment and encryptedSpendingBalance are parsed using fixed-length reads and pointer arithmetic, but their lengths are not validated first; if a caller accidentally passes buffers of the wrong size this can read past the end of the Buffer and invoke undefined behavior.
  src/test/jtx/ConfidentialTransfer.h:184
• claimedBalance - amt can underflow (unsigned wraparound) if a caller provides amt > claimedBalance, which will produce a nonsensical remaining-balance bulletproof and can make failures hard to diagnose. Add an explicit check before subtracting.

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

A changed-region path passes unvalidated key buffers into a C crypto API, which can lead to out-of-bounds reads if unexpected key sizes occur in tests.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/test/jtx/impl/mpt.cpp:992

• getConvertBackProof passes holderPubKey->data() / holderPrivKey->data() into the crypto library without validating buffer sizes. If either key buffer has an unexpected size, this can lead to out-of-bounds reads inside the C API; other helpers in this file (e.g. getSchnorrProof) already size-check keys before calling into mpt_* functions.

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[yinyiqian1]

Approved with some nit comments

[Copilot]

🟢 Approval recommended

The refactor is localized to test/jtx utilities and associated tests, and the updated test logic accounts for the new optional-proof behavior.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The getConvertBackProof signature change leaves at least one remaining call site (outside the PR files) still expecting a Buffer, which will break compilation until updated.

Review details

Suppressed comments (1)

src/test/jtx/mpt.h:712

• Changing getConvertBackProof to return std::optional appears to leave at least one call site still expecting a Buffer (e.g. src/test/app/ConfidentialTransferExtended_test.cpp returns the result directly from a lambda), which will cause a compile error until updated to handle the optional and/or supply a dummy proof where needed.

    [[nodiscard]] std::optional<Buffer>
    getConvertBackProof(
        Account const& holder,
        std::uint64_t const amount,
        uint256 const& contextHash,
        PedersenProofParams const& pcParams) const;

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite