Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

token-2022: [OS-SPL-ADV-00] Enable and fix account ordering on confidential transfer with split proofs #5931

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Nov 30, 2023

Problem

The accounts that are required for a confidential transfer with split proof are as follows:

    ///   1. `[writable]` The source SPL Token account.
    ///   2. `[]` The token mint.
    ///   3. `[writable]` The destination SPL Token account.
    ///   4. `[]` Context state account for
    ///      `VerifyCiphertextCommitmentEqualityProof`.
    ///   5. `[]` Context state account for
    ///      `VerifyBatchedGroupedCiphertext2HandlesValidityProof`.
    ///   6. `[]` Context state account for `VerifyBatchedRangeProofU128`.
    ///   7. `[signer]` The source account owner.
    ///   If `close_split_context_state_on_execution` is set, all context state
    ///     accounts must be `writable` and the following sequence
    ///     of accounts are needed:
    ///   8. `[]` The destination account for lamports from the context state
    ///      accounts.
    ///   9. `[signer]` The context state account owner.
    ///   10. `[]` The zk token proof program.

However, the actual instruction processor assumes that 7. [signer] The source account owner is assumed to be last in the order.

    ///   1. `[writable]` The source SPL Token account.
    ///   2. `[]` The token mint.
    ///   3. `[writable]` The destination SPL Token account.
    ///   4. `[]` Context state account for
    ///      `VerifyCiphertextCommitmentEqualityProof`.
    ///   5. `[]` Context state account for
    ///      `VerifyBatchedGroupedCiphertext2HandlesValidityProof`.
    ///   6. `[]` Context state account for `VerifyBatchedRangeProofU128`.
    ///   If `close_split_context_state_on_execution` is set, all context state
    ///     accounts must be `writable` and the following sequence
    ///     of accounts that are marked with asterisk are needed:
    ///   7*. `[]` The destination account for lamports from the context state
    ///      accounts.
    ///   8*. `[signer]` The context state account owner.
    ///   9*. `[]` The zk token proof program.
    ///   10. `[signer]` The source account owner.

This issue with the ordering of the accounts was left undetected because there were no tests for auto-close of context accounts. There were no tests for auto-close because auto-close was not enabled.

Solution

  • Enabled auto-close of context accounts in the instruction processor logic
  • Added tests for auto-close of context accounts
  • Fixed the issue with the ordering of the accounts

@samkim-crypto samkim-crypto added the WIP Work in progress label Nov 30, 2023
@samkim-crypto samkim-crypto changed the title [token-2022/confidential-transfer] Enable and fix account ordering on confidential transfer with split proofs token-2022: [OS-SPL-ADV-00] Enable and fix account ordering on confidential transfer with split proofs Nov 30, 2023
@samkim-crypto samkim-crypto removed the WIP Work in progress label Nov 30, 2023
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

The core logic looks good! Just some suggestions to make the tests better

Comment on lines 2672 to 2675
let close_split_context_state_accounts = CloseSplitContextStateAccounts {
lamport_destination: &alice.pubkey(),
zk_token_proof_program: &zk_token_proof_program::id(),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add checks at the end to make sure that the context accounts are cleaned up and alice is credited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is good point. I added test to check for AccountNotFound, but let me know if there is a better way to test it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Those checks are great! Can you also check that the lamports were properly added to the destination?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed it! I followed how it is done in close_account.rs and checked that the lamport destination has positive number of lamports.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great!

Comment on lines 3057 to 3060
let close_split_context_state_accounts = CloseSplitContextStateAccounts {
lamport_destination: &alice.pubkey(),
zk_token_proof_program: &zk_token_proof_program::id(),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, please add a check that it worked

@samkim-crypto samkim-crypto merged commit 5da184a into solana-labs:master Dec 2, 2023
34 checks passed
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.

2 participants