Skip to content

feat(clients/js): add confidential transfer with fee helper#1253

Open
catmcgee wants to merge 4 commits into
solana-program:mainfrom
catmcgee:catmcgee/confidential-transfer-with-fee-helper
Open

feat(clients/js): add confidential transfer with fee helper#1253
catmcgee wants to merge 4 commits into
solana-program:mainfrom
catmcgee:catmcgee/confidential-transfer-with-fee-helper

Conversation

@catmcgee

@catmcgee catmcgee commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Adds getConfidentialTransferWithFeeInstructionPlan to the JS client.

This is the fee-aware confidential transfer helper for browser/native JS clients. It builds the proof context accounts the program expects, reads the active fee parameters from the decoded mint, and stages the oversized U256 range proof through SPL Record so the final transfer can fit in a transaction.

This depends on solana-program/zk-elgamal-proof#440. The fee path has to derive Pedersen openings for the net transfer amount, fee amount, percentage-with-cap delta, and remaining available balance. Published @solana/zk-sdk@0.4.2 does not expose the opening arithmetic needed to do that in browser JS.

@catmcgee catmcgee marked this pull request as draft June 16, 2026 17:30
@catmcgee catmcgee force-pushed the catmcgee/confidential-transfer-with-fee-helper branch from d5d27b4 to 23ca760 Compare June 17, 2026 15:58
@catmcgee catmcgee marked this pull request as ready for review June 17, 2026 18:51
@catmcgee catmcgee force-pushed the catmcgee/confidential-transfer-with-fee-helper branch from f91ce2b to c68da01 Compare June 17, 2026 18:53
@joncinque joncinque requested a review from lorisleiva June 22, 2026 10:08
@lorisleiva

Copy link
Copy Markdown
Member

@trevor-cortex

@trevor-cortex trevor-cortex left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Adds getConfidentialTransferWithFeeInstructionPlan — the fee-aware sibling of the existing confidential transfer helper. It reads TransferFeeConfig + ConfidentialTransferFee off the decoded mint, derives the net/fee/delta Pedersen commitments and openings, and builds all five proofs (equality, transfer-amount validity, percentage-with-cap sigma, fee validity, U256 batched range) into context-state accounts staged via SPL Record. Also extends getCreateConfidentialTransferAccountInstructionPlan with includeConfidentialTransferFeeAmount, and reworks the key derivation paths in confidentialTransferKeys.ts to handle both the old (AeKey.fromSignature / ElGamalKeypair.fromSignature) and new (ConfidentialKeys) @solana/zk-sdk shapes.

The math reads correctly to me: split the gross amount into lo/hi, encrypt both under (source, dest, auditor) handles, encrypt the fee lo/hi under (dest, withheld) handles, combine lo/hi via combineLoHi, derive net = transferAmount − fee at both commitment and opening levels, build the delta opening for the sigma proof from fee·10000 − transferAmount·bps, and prove claimedDelta ∈ [0, 9999] via the standard claimedDelta + 9999 − claimedDelta complement pair. The U256 range proof bit-length vector matches the eight committed values (64, 16, 32, 16, 16, 16, 32, 64). 👍

Things that need changing

  1. payer as KeyPairSigner cast is unsound. buildContextStateProofPlan takes payer: TransactionSigner, then forces it to KeyPairSigner when calling createRecord. Any caller passing a wallet-adapter / non-keypair signer (which is the whole point of accepting TransactionSigner at the public API) will hit this at runtime. Two options: either tighten the public type to KeyPairSigner on the fee helper (and propagate that constraint), or have createRecord accept a generic signer if the record-program client supports it. Right now the type lies.

  2. All five proofs unconditionally use record accounts. In the no-fee path, buildContextStateProofPlan defaults useRecordAccount = false and the planner is left to split create+verify across transactions when needed. The fee helper passes true for every proof — including the equality proof (~200 bytes), percentage-with-cap, and the two ciphertext-validity proofs — even though only the U256 range proof actually needs the record-staging path. That adds 4 extra record accounts, their rent, the write loop, and the close instructions per transfer. Worth gating per-proof on actual size, or at least documenting why this is unconditional.

  3. Test doesn't exercise the success path on the published @solana/zk-sdk. hasPedersenArithmetic() is false against @solana/zk-sdk@0.4.2, so the test currently asserts rejects.toThrow(...) and returns. The actual transfer + balance assertions only run once the dependent zk-sdk release lands. Worth calling out in the PR description (you do, but also worth a comment in the test) and either marking the test it.skip with a TODO until the dep is published, or guarding at the suite level so it's obvious from CI output that the happy path isn't being run.

Things worth considering

  1. Pedersen feature-detection is duplicated 7 times. Each of combineLoHiCommitments, combineLoHiOpenings, getZeroOpening, subtractCommitments, subtractOpenings, multiplyCommitment, multiplyOpening does its own typeof === 'function' check and throws the same message. A single feature-detection at the top of getConfidentialTransferWithFeeInstructionPlan would give a clearer error ("this whole helper requires zk-sdk ≥ X") and let the arithmetic helpers be plain typed calls. If you keep the per-call checks, at least vary the error messages so it's obvious which capability is missing.

  2. getSetComputeUnitLimitInstruction is hand-rolled. It builds the raw [2, u32 LE] payload inline. There's nothing wrong with it, but if a @solana-program/compute-budget package is available (similar to how @solana-program/record is being added here) it'd be more discoverable to use it. Not blocking — just calls out a hidden ComputeBudget program dependency.

  3. MAX_COMPUTE_UNIT_LIMIT = 1_400_000 for every verify. This is the cluster max. It's likely fine because the verify is the only non-record instruction in its non-divisible transaction, but worth verifying whether 1.4M is actually needed for the smaller verifies (e.g. equality) — if not, lower limits would leave more headroom for the planner.

  4. assertU64Amount only checks the upper bound — it accepts negatives. In context every caller has run assertNonNegativeAmount or computed a non-negative quantity, so it works in practice, but the helper's name implies a stronger contract than it delivers.

  5. getAuditorElGamalPubkey reads the mint extension when no override is provided. Good improvement — but this benefit is one-sided: getConfidentialTransferInstructionPlan (no fee) still falls back to the default zero pubkey when auditorElgamalPubkey is omitted. Either thread mintAccount into the non-fee helper too (consistent API), or document why the behaviors diverge.

  6. No coverage of edge cases. The single test covers basis_points=150 with maximumFee=1e9 (uncapped path, net=197, fee=3). Worth adding at least: a capped case (maximumFee < rawFeeAmountclaimedDeltaFee = 0n), basis_points=0 (zero fee), and a Fee exceeds transfer amount rejection. These exercise distinct branches of calculateTransferWithFeeAmounts and the sigma proof.

  7. Cosmetic: the JSDoc block immediately above assertNonNegativeAmount ("Builds the setup-and-cleanup instruction plans for a single proof's context-state account...") is orphaned — it used to document buildContextStateProofPlan but is now several functions away. Worth moving back next to that function.

Notes for subsequent reviewers

  • The whole helper is gated behind unreleased @solana/zk-sdk Pedersen arithmetic (subtract, multiplyByU64, combineLoHi, zero()). Before merging, confirm the dep version bump lands in the same release train, and double-check that the property names match what the published zk-sdk exposes (the code probes them via duck-typing, so a typo would only surface as the runtime error from one of the seven combineLoHi/subtract/multiplyByU64 helpers).
  • Worth verifying on a real validator run that the planner actually packs the 5 record-staging setups into a reasonable number of transactions. With MAX_COMPUTE_UNIT_LIMIT = 1.4M per verify each verify is its own non-divisible transaction, so the floor is 5 × (create + N writes + verify-close) plus the main transfer — that's a lot of round trips for a single transfer.
  • The test asserts source=800, pending=197, withheld=3 for transferring 200 at 1.5% with maximumFee=1e9. Math: fee = ceil(200·150/10000) = 3, net = 197, source debited gross (200) → 1000 − 200 = 800. ✓

@catmcgee catmcgee requested a review from trevor-cortex June 29, 2026 16:26
@catmcgee

Copy link
Copy Markdown
Contributor Author

cool bot. addressed comments @lorisleiva

@catmcgee catmcgee force-pushed the catmcgee/confidential-transfer-with-fee-helper branch from 6725f4d to 24db10b Compare June 29, 2026 17:54
@joncinque joncinque changed the title feat: add confidential transfer with fee helper feat(clients/js): add confidential transfer with fee helper Jun 29, 2026
@lorisleiva

Copy link
Copy Markdown
Member

@trevor-cortex

@trevor-cortex trevor-cortex left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Re-review after the changes addressing my previous round. The big-ticket items are all fixed:

  • Payer typing is now sound. RecordBackedContextStateProofMode narrows the public API to KeyPairSigner, and buildContextStateProofPlan takes an explicit recordPayer?: KeyPairSigner argument threaded through only where record staging actually happens. No more as KeyPairSigner cast — the type no longer lies. 👍
  • Record staging is now selective. Only the U256 range proof (the one that actually can't fit) goes through createRecord + write loop + close. Equality, transfer-amount validity, percentage-with-cap, and fee validity all use the direct divisible path. That drops 4 record accounts, their rent, the write loops, and the close instructions per transfer. Big win.
  • Feature-detection is consolidated. A single assertPedersenArithmeticAvailable() at the top of getConfidentialTransferWithFeeInstructionPlan checks all seven capabilities up front with a clear error message; the individual combineLoHi* / subtract* / multiply* helpers are now plain typed casts. Much easier to reason about.
  • assertU64Amount now checks both bounds and is applied to amount, feeAmount, claimedDeltaFee, netTransferAmount, and newAvailableBalance. Good defensive coverage before the values get fed into BigUint64Array.
  • Auditor pubkey resolution is threaded through both the fee helper and the non-fee helper via the shared getAuditorElGamalPubkey. Consistent behavior.
  • Orphaned JSDoc is back where it belongs, right above buildContextStateProofPlan.
  • New negative test covers the Fee exceeds transfer amount branch.

Approving. The remaining items below are follow-ups and non-blockers.

Follow-ups worth doing before or shortly after merge

  1. The happy-path test is still gated behind hasPedersenArithmetic() and short-circuits to rejects.toThrow(PEDERSEN_ARITHMETIC_ERROR) against published @solana/zk-sdk@0.4.2. The added inline comment helps a human reader, but CI still won't exercise the real transfer + balance assertions until the zk-sdk release with subtract / multiplyByU64 / combineLoHi / zero lands. Consider either flipping to it.skip('...', ...) (or a suite-level guard) once the version is known, so the skip is loud in CI output instead of a green pass that isn't actually running the interesting code. Not blocking merge — just don't lose track of it.
  2. Edge cases still not covered: capped-fee (maximumFee < rawFeeAmountclaimedDeltaFee = 0n) and transferFeeBasisPoints = 0 (zero-fee) exercise distinct branches of calculateTransferWithFeeAmounts and the sigma proof, and are worth adding once the happy path actually runs.
  3. Hand-rolled getSetComputeUnitLimitInstruction. Still builds the [2, u32 LE] payload inline. If @solana-program/compute-budget (or equivalent) can be pulled in the way @solana-program/record was, it'd be more discoverable and it'd remove the last stringly-typed program address in this file. Cosmetic.
  4. MAX_COMPUTE_UNIT_LIMIT = 1_400_000 on the range-proof verify. Now that only the U256 range proof is bumped, this is more defensible — that verify is the big one. Still worth measuring on a real validator run whether 1.4M is actually needed or if a tighter number leaves headroom for the planner.
  5. Minor: the range-proof buildContextStateProofPlan call passes input.payer for contextStateAuthority explicitly, but that's already the default. Harmless — could drop the argument to make the recordPayer at the end easier to read.

Notes for subsequent reviewers

  • The whole fee helper is still gated on unreleased @solana/zk-sdk Pedersen arithmetic. Confirm the dep version bump lands in the same release train as this PR, and verify that the property names (subtract, multiplyByU64, combineLoHi, PedersenOpening.zero) exactly match what the published zk-sdk exposes — the code duck-types them and a rename would only surface at runtime via the single top-level assertPedersenArithmeticAvailable() throw.
  • The confidentialTransferKeys.ts reshuffle keeps compatibility with both the current AeKey.fromSignature / ElGamalKeypair.fromSignature shape and the newer ConfidentialKeys.fromSignature(...).ae()/elgamal() shape. Worth confirming the intended migration path (does the token-2022 client want to eventually drop the legacy branch, or keep both indefinitely?) so the compat layer doesn't just accumulate.
  • Math sanity-check for the happy-path test unchanged: transferring 200 at 1.5% with maximumFee = 1e9 → fee = ceil(200·150/10000) = 3, net = 197, source debited gross → 1000 − 200 = 800. ✓ Bit-lengths of the 8-commitment U256 range proof (64, 16, 32, 16, 16, 16, 32, 64) still match the committed values.

@lorisleiva lorisleiva left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@catmcgee Looks good to me (and Trevor ☺️)! Could you just tackle the merge conflicts so I can merge this? 🙏

Comment on lines 10 to +11
import { AeKey, ElGamalKeypair } from '@solana/zk-sdk/bundler';
import * as ZkSdk from '@solana/zk-sdk/bundler';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Here you're actually only using ZkSdk.ConfidentialKeys. Could you make this import more granular to help with tree-shaking?

Suggested change
import { AeKey, ElGamalKeypair } from '@solana/zk-sdk/bundler';
import * as ZkSdk from '@solana/zk-sdk/bundler';
import { AeKey, ElGamalKeypair, ConfidentialKeys } from '@solana/zk-sdk/bundler';

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants