[token-2022] Update confidential transfer fee extension for solana 1.16 #4896

samkim-crypto · 2023-08-01T08:15:23Z

Problem

The confidential transfer fee extension is not updated for solana 1.16

Summary of changes

The most importants changes in the first, second, and last commits.

b96f57a: Updated the WithdrawWithheldTokensFromMint instruction for Solana 1.16 and for it to support context states. This is pretty straightforward except for the part about adding the withheld feeds directly into a destination available balance.

The original plan (#4817) was to divide the withheld amount into low and high parts. Transfer amounts and fees in the instruction are divided into low and high bits so that they are easier to decrypt. When the transfer amount is accumulated into a destination account, the transfer amounts are added into the lo and hi pending balance fields. However, for the case of fees, they are merged and added to the single withheld fees field. Similarly, when these withheld fees are harvest into the mint, it is added to a single withheld fee field. If the withheld fees grow large then it could be inefficient for a withdraw withheld authority to decrypt the ciphertext.

We can choose to also break up the withheld fees into lo and hi parts like the pending balance to make it easier for withdraw withheld authority to decrypt the withheld fees in an account for mint. However, I ended up deciding against it because

Having two ciphertexts for withheld fees in each account requires extra space
More importantly, the instruction WithdrawWithheldTokensFromMint requires a CiphertextCiphertextEqualityProof. If there are two ciphertexts for withheld fees, then we have to either attach two instances of this zkp or batch them, which would require a new instruction in the proof program.
In the worst case that the withdraw withheld authority cannot decrypt the withheld fees directly from the mint info, it can look up the previously list of transactions and decrypt.

Previously, when the withheld fees in the mint are withdrawn to an account, they were added into the "lo" part of the pending balance of the destination account. This is fine, but a little unnatural. If the fees are already merged, then it should really be added into the available balance. So in this PR, I made the change. Since we are making changes to the available balance, I also added the new decryptable available balance field in the instruction.

e64c9f0: Updated the WithdrawWithheldTokensFromAccount instruction for Solana 1.16 and for it to support context states as above.

0ed218e: Small PR to unfeaturize the client function for HarvestWithheldTokensToMint instruction.

dcc8574: Updated the tests for the WithdrawWithheldTokensFromMint instruction. I added the ConfidentialTokenAccountMeta helper struct in tests/confidential_transfer_fee.rs, which is a stripped down version of ConfidentialTokensAccountMeta in tests/confidential_transfer.rs. I can re-use the helper struct from tests/confidential_transfer.rs, but I decided to just have a separate helper struct in test/confidential_transfer_fee.rs that does not deal with fields like memo. The rest of the tests were straightforward.

c582a53: Updated the tests for the WithdrawWithheldTokensFromAccounts instruction.

a00c50e: Added tests that deal with context states for the WithdrawWithheldTokensFromMint and WithdrawWithheldTokensFromAccounts instructions.

3a6b3e0: Right now, the instruction WithdrawWithheldTokensFromMint could be susceptible to front-running. The zero-knowledge proof that is needed for the instruction depends on the currently withheld amount ciphertext in the mint. If the withheld amount is constantly harvested into the mint, the zkp can be nullified.

This PR added EnableHarvestToMint and DisableHarvestToMint, which acts like {Enable,Disable}ConfidentialTransfers and {Enable,Disable}NonConfidentialTransfers for harvest. If the harvest fails due to front-running, the harvest authority can disable harvest, withdraw withheld tokens, and then enable harvest.

joncinque

Looks really great overall! The core logic is good, just a few little things to sort out

token/program-2022/src/extension/confidential_transfer_fee/account_info.rs

token/program-2022/src/extension/confidential_transfer_fee/mod.rs

joncinque · 2023-08-09T21:06:36Z

token/program-2022/src/extension/confidential_transfer_fee/mod.rs

+impl ConfidentialTransferFeeConfig {
+    /// Return the account information needed to construct a `WithdrawWithheldTokensFromMint`
+    /// instruction.
+    #[cfg(not(target_os = "solana"))]
+    pub fn withheld_tokens_info(&self) -> WithheldTokensInfo {
+        WithheldTokensInfo {
+            withheld_amount: self.withheld_amount,
+        }
+    }
+}


this seems a bit backwards -- how about having a function on WithheldTokensInfo that takes &ConfidentialTransferFeeConfig? That way you can keep the #[cfg(not(target_os = "solana"))] bits all in account_info.rs

Yeah that is actually a good idea. Since WithheldTokensInfo just has one field withheld_amount, I think we can just work with the existing constructor new(withheld_amount: EncryptedWithheldAmount) -> Self rather than taking in the entire confidential transfer fee config.

But the suggestion appiles for the account info types in the regular confidential transfer extension, so I will apply the suggestion there.

token/program-2022/src/extension/confidential_transfer_fee/instruction.rs

token/program-2022/src/extension/confidential_transfer_fee/processor.rs

joncinque · 2023-08-09T21:25:45Z

token/program-2022/src/extension/confidential_transfer_fee/processor.rs

+    if !authority_info.is_signer {
+        return Err(ProgramError::MissingRequiredSignature);
+    }


The comment mentions that this works with multisigs, but this check means that it only works with single authorities. It's fine if we don't want to support multisigs, but we should be clear in the instruction interface

joncinque · 2023-08-09T21:25:56Z

token/program-2022/src/extension/confidential_transfer_fee/processor.rs

+    if !authority_info.is_signer {
+        return Err(ProgramError::MissingRequiredSignature);
+    }


joncinque · 2023-08-09T21:30:44Z

token/program-2022/src/extension/confidential_transfer_fee/processor.rs

@@ -367,41 +468,28 @@ pub(crate) fn process_instruction(
            )
        }
        ConfidentialTransferFeeInstruction::WithdrawWithheldTokensFromMint => {
-            msg!("ConfidentialTransferInstruction::WithdrawWithheldTokensFromMint");
-            #[cfg(all(feature = "zk-ops", feature = "proof-program"))]


This removes the featurizing on zk-ops, so I think it'll fail to build with zk-ops disabled. I think we might need one more release using the feature off unfortunately, so could you restore that?

Yes, sorry I missed this!

joncinque · 2023-08-09T21:31:03Z

token/program-2022/src/extension/confidential_transfer_fee/processor.rs

        }
        ConfidentialTransferFeeInstruction::WithdrawWithheldTokensFromAccounts => {
-            msg!("ConfidentialTransferInstruction::WithdrawWithheldTokensFromAccounts");
-            #[cfg(all(feature = "zk-ops", feature = "proof-program"))]


Same here, please restore the feature check

Co-authored-by: Jon Cinque <[email protected]>

…k-ops` feature

joncinque

Just one last little nit, then this is good to go!

joncinque · 2023-08-10T12:11:15Z

token/program-2022/src/extension/confidential_transfer_fee/processor.rs

-            #[cfg(not(all(feature = "zk-ops", feature = "proof_program")))]
-            {
-                Err(ProgramError::InvalidInstructionData)
+                )
            }


You'll also need the Error variant if the feature is off

Oh sorry I missed it!

joncinque · 2023-08-10T12:11:28Z

token/program-2022/src/extension/confidential_transfer_fee/processor.rs

-            #[cfg(not(all(feature = "zk-ops", feature = "proof_program")))]
-            {
-                Err(ProgramError::InvalidInstructionData)
+                )
            }


You'll also need the error variant if the feature is off

Pull request has been modified.

samkim-crypto added the WIP Work in progress label Aug 1, 2023

samkim-crypto force-pushed the token22-withdraw-withheld branch 2 times, most recently from 21545f8 to 995e1a7 Compare August 2, 2023 08:28

samkim-crypto added 7 commits August 4, 2023 07:43

update processor and client for withdraw withheld tokens from mint

b96f57a

update processor and client for withdraw withheld tokens from accounts

e64c9f0

update client for harvest tokens to mint

0ed218e

update tests for withdraw withheld tokens from mint

dcc8574

update tests for withdraw withheld tokens from accounts

c582a53

add tests with proof context states

a00c50e

clippy

e4869dd

samkim-crypto force-pushed the token22-withdraw-withheld branch from 995e1a7 to e4869dd Compare August 3, 2023 22:43

add enable and disable harvest to mint instructions

3a6b3e0

samkim-crypto mentioned this pull request Aug 8, 2023

[token-2022] Remaining tasks for the confidential transfer extension #4817

Closed

7 tasks

samkim-crypto removed the WIP Work in progress label Aug 9, 2023

samkim-crypto requested a review from joncinque August 9, 2023 08:37

joncinque reviewed Aug 9, 2023

View reviewed changes

samkim-crypto and others added 3 commits August 10, 2023 09:59

Apply suggestions from code review

a4c6bdb

Co-authored-by: Jon Cinque <[email protected]>

update harvest_to_mint_disabled to harvest_to_mint_enabled

286b8fb

move WithdrawWithheldTokensFrom{Accounts,Mint} instructions into `z…

2cb1575

…k-ops` feature

samkim-crypto force-pushed the token22-withdraw-withheld branch from 4c8d552 to 2cb1575 Compare August 10, 2023 01:58

samkim-crypto added 2 commits August 10, 2023 11:00

add support for multisig

893b585

remove ConfidentialTransferFeeConfig::withheld_tokens_info()

4eb9041

joncinque previously approved these changes Aug 10, 2023

View reviewed changes

add relevant error output on zk-ops feature

d5425a5

samkim-crypto merged commit 08197b0 into solana-labs:master Aug 11, 2023
29 checks passed

samkim-crypto mentioned this pull request Aug 11, 2023

[token-2022] Add constructors for confidential transfer account info types #4982

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[token-2022] Update confidential transfer fee extension for solana 1.16 #4896

[token-2022] Update confidential transfer fee extension for solana 1.16 #4896

samkim-crypto commented Aug 1, 2023 •

edited

Loading

joncinque left a comment

joncinque Aug 9, 2023

samkim-crypto Aug 10, 2023

samkim-crypto Aug 10, 2023

joncinque Aug 9, 2023

joncinque Aug 9, 2023

joncinque Aug 9, 2023

samkim-crypto Aug 10, 2023

joncinque Aug 9, 2023

joncinque left a comment

joncinque Aug 10, 2023

samkim-crypto Aug 11, 2023

joncinque Aug 10, 2023

[token-2022] Update confidential transfer fee extension for solana 1.16 #4896

[token-2022] Update confidential transfer fee extension for solana 1.16 #4896

Conversation

samkim-crypto commented Aug 1, 2023 • edited Loading

Problem

Summary of changes

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samkim-crypto commented Aug 1, 2023 •

edited

Loading