Skip to content

feat: confidential transfers helper functions#1096

Open
catmcgee wants to merge 10 commits into
solana-program:mainfrom
catmcgee:main
Open

feat: confidential transfers helper functions#1096
catmcgee wants to merge 10 commits into
solana-program:mainfrom
catmcgee:main

Conversation

@catmcgee
Copy link
Copy Markdown

@catmcgee catmcgee commented Apr 10, 2026

Adds helper functions for

  • creating conf balance accounts (getCreateConfidentialTransferAccountInstructions, returns array of 4 instructions
  • applying a pending balance (getApplyConfidentialPendingBalanceInstructionFromToken)
  • doing a conf transfer (getConfidentialTransferInstructions, returns instruction plan, proofs too large for one tx)
  • withdrawing confidential (getConfidentialWithdrawInstructions, similar to above)

Uses @solana/zk-sdk for ElGamal/AES primitives and key derivation

@catmcgee catmcgee marked this pull request as ready for review April 10, 2026 10:40
@catmcgee catmcgee marked this pull request as draft April 22, 2026 12:50
catmcgee added 2 commits May 1, 2026 16:26
# Conflicts:
#	clients/js/package.json
#	clients/js/pnpm-lock.yaml
@catmcgee catmcgee marked this pull request as ready for review May 1, 2026 16:54
@joncinque
Copy link
Copy Markdown
Contributor

@samkim-crypto can you take a look from the program side and make sure everything is correct?

@lorisleiva can you take a look from the kit side?

@lorisleiva
Copy link
Copy Markdown
Member

Thanks for the heads up, don't know how I missed that one.

I'll give this a proper review next week but my fist impressions on the Kit side are: we should return InstructionPlans when designing helpers that combine multiple instructions and it feels like there's a lot of code repetition from the generated code when it would be best to override/wrap it instead (or even improve their IDL definitions when possible). I may be missing context for that last one so I'll report back next week. 🫡

Copy link
Copy Markdown
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

I took the time to look deeper into the changes but it is challenging to understand what was changed from the original generated instructions since this PR copy/paste/edits the generated code instead of wrapping it in higher level helpers. This also makes the generated code out of sync with the IDL which is not ideal. Additionally some code introduce module variable which will break treeshakability. We should also use the native Crypto API for cryptographic operations instead of relying on third party package for security reasons (this is what Kit itself does). Finally, CI is not green. If you wouldn't mind adjusting the PR with these changes, I'll be able to start reviewing the actual API changes, thank you!

catmcgee added 3 commits May 11, 2026 11:33
# Conflicts:
#	clients/js/pnpm-lock.yaml
…-program/zk-elgamal-proof, bind keys to owner+mint
@catmcgee catmcgee requested a review from lorisleiva May 11, 2026 17:09
* Setup toml cli env

* add env setup to publish actions
Copy link
Copy Markdown
Contributor

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

Sorry for the late review of this. The cryptographic logic all looks fine to me. I can take a stab at adding the ciphertext arithmetic logic in @solana/zk-sdk, but I think using noble should be fine for now.

Just some minor comments on rejecting negative inputs and it would also be good to add a success case for configuring the account.

Would be nice to get some inputs from @lorisleiva regarding the points on codama.

* transaction). The cleanup plan closes the context-state account to recover
* its rent.
*/
function computeNewAvailableBalance(currentBalance: bigint, amount: bigint): bigint {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we check and reject negative numbers?

);
}

function toBigIntAmount(amount: number | bigint) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be good to also check for negative amounts here for defensive programming.

* grouped ciphertext. The grouped layout is: 32-byte commitment followed
* by N 32-byte handles. The returned 64-byte array is [commitment, handle].
*/
export function extractCiphertextFromGroupedBytes(groupedCiphertext: ReadonlyUint8Array, handleIndex: number) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we should check for negative handleIndex.

Copy link
Copy Markdown
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, I knew this was gonna be a big one to review so I wanted to allocate enough time to do this thoroughly. There are rather a lot of non-idiomatic patterns in this PR which explains the high number of comments. More importantly, it seems like this PR requires IDL changes / fixes for the confidential transfer extension which I think should be tackled first (before landing this PR).

Comment on lines +1 to +3
declare module '@solana/zk-sdk/node' {
export * from '@solana/zk-sdk/dist/node/index';
}
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.

Could you please tell me more about why this (and the related changes in the clients/js/tsconfig.json file) is needed?

Comment on lines +59 to +60
const INSTRUCTIONS_SYSVAR_ADDRESS =
'Sysvar1nstructions1111111111111111111111111' as Address<'Sysvar1nstructions1111111111111111111111111'>;
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.

We already depend on @solana/sysvar which exports a SYSVAR_INSTRUCTIONS_ADDRESS constant.

Comment on lines +247 to +251
// codama bug: the generated ConfidentialTransfer encoder is missing
// `transferAmountAuditorCiphertextLo` and `transferAmountAuditorCiphertextHi`,
// which the on-chain program reads from the instruction data. Wrapped in a
// function so the encoder is not constructed at import time.
function getConfidentialTransferWithAuditorCiphertextsEncoder() {
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.

This should be fixed in the Codama IDL itself which is currently manually written in interface/idl.json.

Ideally all IDL errors should be fixed in separate PRs prior to landing this PR and should include regression tests.

As a philosophy, all hand written helpers should defer to the generated code as much as possible to avoid repetition and ensure the program is in sync with its clients.

Comment on lines +442 to +448
// codama bug: the generated `getConfigureConfidentialTransferAccountInstruction`
// emits a 5-account layout (with a `record` slot) that only matches the
// `ConfigureAccountWithRegistry` on-chain path. In `ProofInstructionOffset`
// mode the on-chain handler reads 4 accounts, so the program-ID-padded
// `record` slot ends up in the authority position. Hand-build the
// 4-account form until the IDL splits the two configure variants.
function getConfigureConfidentialTransferAccountInstructionWithProof(input: {
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.

Same here (IDL error should be fixed in separate PR).

Comment on lines +475 to +480
// codama bug: same shape as the configure helper above. The on-chain
// `verify_withdraw_proof` consumes 0–3 conditional accounts between mint
// and authority (sysvar when either offset != 0; one context-state account
// per offset == 0). The generated builder always emits a fixed 6-account
// layout with program-ID placeholders, putting accounts in the wrong slots.
function getConfidentialWithdrawInstructionWithProof(input: {
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.

Same here (IDL error should be fixed in separate PR).

Comment on lines +26 to +32
const addressEncoder = getAddressEncoder();
const ownerBytes = addressEncoder.encode(owner);
const mintBytes = addressEncoder.encode(mint);
const seed = new Uint8Array(ownerBytes.length + mintBytes.length);
seed.set(ownerBytes, 0);
seed.set(mintBytes, ownerBytes.length);
return seed;
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.

This can be refactored as:

const encoder = getTupleEncoder([getAddressEncoder(), getAddressEncoder()]);
return encoder.encode([owner, mint])

export async function deriveElGamalKeypair({
signer,
zk,
publicSeed = new Uint8Array(0),
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.

Not sure if this is a ZK convention but maybe best to be explicit about what we're asking the user to sign?

some,
type MessagePartialSigner,
} from '@solana/kit';
import * as zkSdk from '@solana/zk-sdk/node';
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.

Oh I understand the why the ZK client was designed that way now. I just don't think this is much of an interface if we're matching the ZK SDK as-is. If I understand this correctly, this means we should be able to rely on @solana/zk-sdk directly and dissolve the interface altogether.

elgamalKeypair,
aesKey,
proofMode: 'instruction-data',
} as unknown as Parameters<typeof getConfidentialWithdrawInstructions>[0]),
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.

Could we try and remove the need for this type cast. Otherwise the end-user is likely to be hit by this as well.

Comment on lines +52 to +60
// Expected to fail until the IDL emits a separate `ConfigureAccountWithRegistry`
// instruction and the generated `getConfigureConfidentialTransferAccountInstruction`
// stops including the `record` slot in inline-proof mode. AVA's `test.failing`
// passes while the test fails and starts failing once the test starts passing,
// so CI will notice and the test can be flipped back to a regular `test` when
// the upstream codegen catches up. The working flow lives in
// `getCreateConfidentialTransferAccountInstructions`, which hand-builds the
// configure instruction with the correct 4-account layout.
test.failing('it configures and approves a token account for confidential transfer using derived keys', async t => {
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.

Here again sounds like we need to tweak the IDL and its generated clients before resolving this PR. I would rather tackles things in this order than having to ship failing tests.

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.

6 participants