[token-2022] Confidential transfer parallel split proofs #5054
Conversation
samkim-crypto
force-pushed
the
token22-parallel-split-proofs
branch
from
713c93c
to
58cbc65
Compare
samkim-crypto
force-pushed
the
token22-parallel-split-proofs
branch
from
58cbc65
to
8589746
Compare
| source_token_account: &Pubkey, | ||
| destination_token_account: &Pubkey, | ||
| mint: &Pubkey, |
There was a problem hiding this comment.
nit: I'm just noticing this now, so you don't need to fix it in this PR, but all of the confidential transfer functions order these as "source, destination, mint", whereas the normal transfer functions order it as "source, mint, destination". We should make that consistent at some point
There was a problem hiding this comment.
Yep, thanks for the catch. I'll reorder the inputs in the confidential transfer functions in a follow-up PR.
| source_decrypt_handles, | ||
| )?; | ||
| // If `maybe_proof_context` is `None`, then this means that | ||
| // `close_split_context_state_on_execution` is true and a required context state account is |
There was a problem hiding this comment.
This should be no_op_on_uninitialized_split_context_state, right?
There was a problem hiding this comment.
Thanks for the catch! Fixed.
|
|
||
| if maybe_proof_context.is_none() { | ||
| msg!("Context states not fully initialized: return with no op"); | ||
| msg!("WARNING: transfer is NOT yet executed"); |
There was a problem hiding this comment.
nit: Maybe "WARNING" is a bit strong, since it is expected operation. Let's just say "transfer is NOT yet executed" and combine it with the earlier msg
There was a problem hiding this comment.
You are right. I removed the warning and merged the "transfer is NOT yet executed" to the previous message.
| @@ -648,7 +782,7 @@ fn process_source_for_transfer( | |||
| source_transfer_amount_lo, | |||
| source_transfer_amount_hi, | |||
| ) | |||
| .ok_or(TokenError::CiphertextArithmeticFailed)?; | |||
| .ok_or(ProgramError::InvalidInstructionData)?; | |||
There was a problem hiding this comment.
Did you mean to revert all of these errors? Unless there's a good reason, let's stick with the more specific ones
There was a problem hiding this comment.
This must have been introduced when I was refactoring. I reverted these changes.
token/client/src/token.rs
Outdated
| /// If an optional transfer instruction is provided, then the transfer instruction is attached | ||
| /// to the same transaction. | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub async fn confidential_transfer_create_equality_and_ciphertext_validity_proofs_for_transfer< |
There was a problem hiding this comment.
nit: instead of proofs can you say context_state since that's what's being done? And rather than overloading this public function to optionally add a transfer, how about have three functions:
confidential_transfer_create_equality_and_ciphertext_validity_context_states_for_transfer-- just the creation of the context state accountsconfidential_transfer_create_equality_and_ciphertext_validity_context_states_and_transfer_parallel-- specifically for parallel execution, requires the transfer instruction- some internal helper function that they both call into to create the proofs, up to you for naming!
There was a problem hiding this comment.
Yes, that is a great idea!
token/client/src/token.rs
Outdated
| /// | ||
| /// If an optional transfer instruction is provided, then the transfer instruction is attached | ||
| /// to the same transaction. | ||
| pub async fn confidential_transfer_create_range_proof_for_transfer<S: Signer>( |
There was a problem hiding this comment.
Same thing here, can we split it into three functions?
mergify
bot
dismissed
joncinque’s stale review
Pull request has been modified.
…#5054) * add `TransferWithSplitProofs` instruction * add program logic for `TransferWithSplitProofs` instruction * update token client for `TransferWithSplitProofs` instruction * update tests for `TransferWithSplitProofs` instruction * clippy * fix comments and modify warning message from no-op * fix ciphertext arithmetic error type * refactor token client proof context state functions
Summary of changes
Transferinstruction and added a separateTransferWithSplitProofsthat support transfer with split proof contexts.verify_transfer_proofinverify_proof.rs. I made it so that the function returns anOptionthat is wrapped inside anotherResult. I considered adding a custom type for the return type, but I thought this could make things even more complicated.TransferWithSplitProofsinstruction. This PR adds the following functions:confidential_transfer_create_equality_and_ciphertext_validity_proofs_for_transfer: This function creates context state accounts for the equality and ciphertext validity proofs that are needed for a transferconfidential_transfer_create_range_proof_for_transfer: This function creates context state account for the range proof that is needed for a transferconfidential_transfer_transfer_with_split_proofs: This submits theTransferWithSplitProofsinstruction assuming that all the context states have been previously created.confidential_transfer_close_context_state: This function closes a context state accountconfidential_transfer_transfer_with_split_proofs_in_parallel: This function submits two transactions in parallel. The first transaction consist of instructions needed to create the relevant equality and ciphertext validity proof with the transfer instruction attached. The second transaction consists of instructions to needed to create range proof also with the transfer instruction attached. Whichever instruction lands later will fully execute the transfer.TransferWithSplitProofsinstruction.