feat(clients/js): add confidential transfer with fee helper#1253
feat(clients/js): add confidential transfer with fee helper#1253catmcgee wants to merge 4 commits into
Conversation
d5d27b4 to
23ca760
Compare
f91ce2b to
c68da01
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
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
-
payer as KeyPairSignercast is unsound.buildContextStateProofPlantakespayer: TransactionSigner, then forces it toKeyPairSignerwhen callingcreateRecord. Any caller passing a wallet-adapter / non-keypair signer (which is the whole point of acceptingTransactionSignerat the public API) will hit this at runtime. Two options: either tighten the public type toKeyPairSigneron the fee helper (and propagate that constraint), or havecreateRecordaccept a generic signer if the record-program client supports it. Right now the type lies. -
All five proofs unconditionally use record accounts. In the no-fee path,
buildContextStateProofPlandefaultsuseRecordAccount = falseand the planner is left to split create+verify across transactions when needed. The fee helper passestruefor 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. -
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 assertsrejects.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 testit.skipwith 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
-
Pedersen feature-detection is duplicated 7 times. Each of
combineLoHiCommitments,combineLoHiOpenings,getZeroOpening,subtractCommitments,subtractOpenings,multiplyCommitment,multiplyOpeningdoes its owntypeof === 'function'check and throws the same message. A single feature-detection at the top ofgetConfidentialTransferWithFeeInstructionPlanwould 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. -
getSetComputeUnitLimitInstructionis hand-rolled. It builds the raw[2, u32 LE]payload inline. There's nothing wrong with it, but if a@solana-program/compute-budgetpackage is available (similar to how@solana-program/recordis being added here) it'd be more discoverable to use it. Not blocking — just calls out a hidden ComputeBudget program dependency. -
MAX_COMPUTE_UNIT_LIMIT = 1_400_000for 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. -
assertU64Amountonly checks the upper bound — it accepts negatives. In context every caller has runassertNonNegativeAmountor computed a non-negative quantity, so it works in practice, but the helper's name implies a stronger contract than it delivers. -
getAuditorElGamalPubkeyreads 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 whenauditorElgamalPubkeyis omitted. Either threadmintAccountinto the non-fee helper too (consistent API), or document why the behaviors diverge. -
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 < rawFeeAmount→claimedDeltaFee = 0n), basis_points=0 (zero fee), and aFee exceeds transfer amountrejection. These exercise distinct branches ofcalculateTransferWithFeeAmountsand the sigma proof. -
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 documentbuildContextStateProofPlanbut 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-sdkPedersen 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 sevencombineLoHi/subtract/multiplyByU64helpers). - 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.4Mper verify each verify is its own non-divisible transaction, so the floor is5 × (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. ✓
|
cool bot. addressed comments @lorisleiva |
6725f4d to
24db10b
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Re-review after the changes addressing my previous round. The big-ticket items are all fixed:
- Payer typing is now sound.
RecordBackedContextStateProofModenarrows the public API toKeyPairSigner, andbuildContextStateProofPlantakes an explicitrecordPayer?: KeyPairSignerargument threaded through only where record staging actually happens. No moreas KeyPairSignercast — 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 ofgetConfidentialTransferWithFeeInstructionPlanchecks all seven capabilities up front with a clear error message; the individualcombineLoHi*/subtract*/multiply*helpers are now plain typed casts. Much easier to reason about. assertU64Amountnow checks both bounds and is applied toamount,feeAmount,claimedDeltaFee,netTransferAmount, andnewAvailableBalance. Good defensive coverage before the values get fed intoBigUint64Array.- 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 amountbranch.
Approving. The remaining items below are follow-ups and non-blockers.
Follow-ups worth doing before or shortly after merge
- The happy-path test is still gated behind
hasPedersenArithmetic()and short-circuits torejects.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 withsubtract/multiplyByU64/combineLoHi/zerolands. Consider either flipping toit.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. - Edge cases still not covered: capped-fee (
maximumFee < rawFeeAmount→claimedDeltaFee = 0n) andtransferFeeBasisPoints = 0(zero-fee) exercise distinct branches ofcalculateTransferWithFeeAmountsand the sigma proof, and are worth adding once the happy path actually runs. - 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/recordwas, it'd be more discoverable and it'd remove the last stringly-typed program address in this file. Cosmetic. MAX_COMPUTE_UNIT_LIMIT = 1_400_000on 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.- Minor: the range-proof
buildContextStateProofPlancall passesinput.payerforcontextStateAuthorityexplicitly, but that's already the default. Harmless — could drop the argument to make therecordPayerat the end easier to read.
Notes for subsequent reviewers
- The whole fee helper is still gated on unreleased
@solana/zk-sdkPedersen 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-levelassertPedersenArithmeticAvailable()throw. - The
confidentialTransferKeys.tsreshuffle keeps compatibility with both the currentAeKey.fromSignature/ElGamalKeypair.fromSignatureshape and the newerConfidentialKeys.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
left a comment
There was a problem hiding this comment.
@catmcgee Looks good to me (and Trevor
| import { AeKey, ElGamalKeypair } from '@solana/zk-sdk/bundler'; | ||
| import * as ZkSdk from '@solana/zk-sdk/bundler'; |
There was a problem hiding this comment.
nit: Here you're actually only using ZkSdk.ConfidentialKeys. Could you make this import more granular to help with tree-shaking?
| 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'; |
Adds
getConfidentialTransferWithFeeInstructionPlanto 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.2does not expose the opening arithmetic needed to do that in browser JS.