Derive confidential transfer auditor from mint#1269
Conversation
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Makes auditorElgamalPubkey optional on getConfidentialTransferInstructionPlan by fetching it from the mint's ConfidentialTransferMint extension when omitted. Adds a small helper (getRequiredConfidentialTransferMintExtension) mirroring the existing token-account version, splits out ConfidentialTransferContextStateProofMode so transfer's rpc requires GetAccountInfoApi (withdraw is untouched), and adds an integration test that exercises the auto-fetch path on a mint configured with a real auditor key.
Overall the change is well-scoped and a real ergonomics win — the previous behavior of silently using the zero auditor when the mint actually has one configured was a foot-gun, and failing earlier with a clearer message on a mint that's missing the extension is the right call.
Things to watch / discuss
- Extra RPC roundtrip when
auditorElgamalPubkeyis omitted. Each call togetConfidentialTransferInstructionPlanwithout an auditor now triggers afetchMint. Many callers (e.g. a UI that already fetched the mint to render decimals/extensions) will have theMintin hand and just want the helper to do the right thing. Worth considering whether the input shape could optionally accept a pre-fetchedmint: Mint(similar to howsourceTokenAccount: Tokenis already passed in) to skip the extra request. The current shape is fine for v1, but documenting the perf trade-off in the doc comment, or following up with amintAccount?: Mintoverload, would be a nice next step. - PR description vs. behavior — minor. The description says the helper now fails earlier when the mint is missing the
ConfidentialTransferMintextension. That's true only whenauditorElgamalPubkeyis omitted; if a caller explicitly passes the auditor key, the mint is never fetched and an invalid mint will still surface as a downstream transaction failure. Not a regression and likely the right trade-off (don't pay an RPC tax when the caller has already supplied the needed data), but worth being explicit about in the doc comment so callers know what they're opting out of. - Type duplication.
ConfidentialTransferContextStateProofModeis identical toContextStateProofModeexceptrpcalso requiresGetAccountInfoApi. Reasonable to keep withdraw's signature stable, but if more helpers grow this requirement it'd be worth collapsing into a generic (ContextStateProofMode<ExtraApi = never>) rather than forking again.
Notes for subsequent reviewers
- The default-auditor fallback inside
getAuditorElGamalPubkey(when the mint'sauditorElgamalPubkeyisNone) preserves prior behavior — please confirm that's actually the program-side convention for "no auditor" on confidential transfers; if zero is just a sentinel here and not a meaningful key, no action needed. - The new test only asserts the transfer succeeds when the auditor is auto-fetched. It doesn't verify the auditor's ciphertext was actually produced with the configured key (vs. silently falling back to the default). A decrypt-side assertion would be stronger but is probably overkill for an integration test; flagging in case anyone wants to harden it.
- No test covers the new
'Mint account is missing the ConfidentialTransferMint extension.'error path. Easy to add as a unit-level test if you want regression coverage on the error message.
| return isSome(extension.auditorElgamalPubkey) | ||
| ? getElGamalPubkeyFromAddress(extension.auditorElgamalPubkey.value) | ||
| : getDefaultAuditorElGamalPubkey(); | ||
| } |
There was a problem hiding this comment.
Two small things worth considering here:
- Doc comment. This is now the canonical place the auditor is resolved, but there's no JSDoc explaining the resolution order (explicit input → mint extension → zero default) or the fact that omitting
auditorElgamalPubkeycosts an extra RPCfetchMint. A short comment would save future readers a trip into the helper. - Optional pre-fetched mint. Consider accepting an optional
mint?: Minton the input (parallel to howsourceTokenAccount: Tokenis already passed in pre-fetched). Callers building higher-level flows often already have the mint loaded, and this would let them skip the extra round-trip while still benefiting from the auto-resolution. Happy to defer to a follow-up PR.
| function getRequiredConfidentialTransferMintExtension(mint: Mint): ConfidentialTransferMintExtension { | ||
| if (!isSome(mint.extensions)) { | ||
| throw new Error('Mint account is missing extensions.'); | ||
| } | ||
|
|
||
| const extension = mint.extensions.value.find(candidate => candidate.__kind === 'ConfidentialTransferMint') as | ||
| | ConfidentialTransferMintExtension | ||
| | undefined; | ||
| if (!extension) { | ||
| throw new Error('Mint account is missing the ConfidentialTransferMint extension.'); | ||
| } | ||
|
|
||
| return extension; |
There was a problem hiding this comment.
Nit: this is a near-exact clone of getRequiredConfidentialTransferAccountExtension above, just for the mint extension. If a third copy ever shows up it'd be worth extracting a small generic like getRequiredExtension<K extends Extension['__kind']>(account: { extensions: ... }, kind: K). Not blocking for this PR.
| const client = await createValidatorClient(); | ||
| const payer = client.payer; | ||
| const [sourceOwner, destinationOwner] = await Promise.all([ | ||
| generateKeyPairSignerWithSol(client), |
There was a problem hiding this comment.
Reusing an ElGamalKeypair().pubkey().toBytes() as an Address via getAddressDecoder().decode(...) works (both are 32 bytes), but it's mixing two unrelated key types in test setup and could confuse future readers. A short inline comment explaining why this works — "ElGamal pubkeys and Solana addresses share the same 32-byte layout, so we reuse the address decoder to thread the key through createConfidentialMint's Address-typed field" — would help.
| const updatedDestination = await fetchAssociatedToken(client, destinationOwner.address, mint); | ||
| const destinationConfidential = getTokenExtension(updatedDestination, 'ConfidentialTransferAccount'); | ||
| expect(destinationConfidential.pendingBalanceCreditCounter).toBe(1n); | ||
| }); |
There was a problem hiding this comment.
This assertion is identical to the one in the test above and only proves the transfer succeeded — it doesn't actually verify the auto-fetched auditor key was used. If the new getAuditorElGamalPubkey path silently fell back to getDefaultAuditorElGamalPubkey() (e.g. a bug where isSome(extension.auditorElgamalPubkey) returned false despite being set), this test would still pass.
A stronger assertion would decrypt one of the destination's auditor ciphertexts with the auditor's ElGamal secret key and confirm it round-trips to the transferred amount. Worth doing if practical; otherwise consider at least asserting via a spy/log that the mint-fetch branch was taken.
|
Re-triggered, but the diff is unchanged since my previous review — same three files, same hunks. Nothing new to flag. Happy to take another pass once there are commits to look at, or to dig into any of the prior points (extra RPC roundtrip / optional pre-fetched |
|
Thanks @catmcgee! The only thing I'm not sure about is the |
3 participants
Updates getConfidentialTransferInstructionPlan so callers do not have to manually fetch the mint just to discover the configured auditor key.
Behavior:
This avoids the current confusing path where a mint with an auditor configured can produce proof/transfer data using the default auditor key, and the transaction then fails without pointing the caller at the missing auditor input.