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] Add support for reading proofs from record accounts #7055

Merged

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Jul 26, 2024

Problem

The token22 program does not yet have support for reading zk proofs from record program.

Summary of Changes

Added support for reading zk proofs from the record program.

7f510f4: Added support for the record program in the program. The main changes here are twofold:

  • Updated the decode_proof_instruction_context function so that if the instruction data associated with the zk instruction is exactly 5 bytes, then read the proof from the supplied record account instead of reading it from instruction data. In the process, I changed the output type of the function to Result<U, ProgramError> instead of Result<&U, ProgramError> to simplify over-complication with lifetimes. All the functions that use decode_proof_instruction_context seem to immediately deref the context type anyway.
  • Extended the ProofLocation type to have support for RecordAccount. Instead of having three variants of InstructionOffset, ContextStateAccount, and RecordAccount, I decided to overload InstructionOffset to support RecordAccount since this simplifies logic in some places.

b245e14: Added functions to create and close record accounts for confidential transfers. The main interesting part here is the calculate_record_max_chunk_size function, which provides the maximum chunk size for a proof record instruction to fit inside a single transaction. I included this function in token.rs, but I can move this to somewhere if suggested.

92caf61: For each of the instructions that require zkp's I added ...with_record_account() test variants to make sure that the record account works. Exceptions are the transfer (with/without fee) instruction. I omitted adding tests for these instructions because I will need to heavily modify/remove these instructions in a follow-up PR.

@samkim-crypto samkim-crypto marked this pull request as ready for review July 26, 2024 10:31
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.

I looked through a bit, and before I keep going, I wanted to check if we could somehow combine the proof source to be one of:

  • the instructions sysvar
  • context state account
  • a record account

This might require updating proof_instruction_offset to be a bit clearer -- maybe we make it something like:

enum ProofLocation {
    ContextStateAccount
    InstructionOffset(i8),
    AccountData(u32),
}

Although it'll cause churn, I hope it'll be easier to understand -- what do you think?

token/client/src/temp_client.rs Outdated Show resolved Hide resolved
token/program-2022/src/lib.rs Outdated Show resolved Hide resolved
@samkim-crypto
Copy link
Contributor Author

I looked through a bit, and before I keep going, I wanted to check if we could somehow combine the proof source to be one of:

  • the instructions sysvar
  • context state account
  • a record account

This might require updating proof_instruction_offset to be a bit clearer -- maybe we make it something like:

enum ProofLocation {
    ContextStateAccount
    InstructionOffset(i8),
    AccountData(u32),
}

Although it'll cause churn, I hope it'll be easier to understand -- what do you think?

Yeah the current version is the result of a number of attempts to simplify things, but it is still a bit confusing. The complication is that even if the proof is read from an account, you still need the sysvar to access the zk-proof instruction in the transaction. Unlike the case of context state accounts, if the proof is stored in a record account, it is not yet verified, so an accompanying zk instruction has to be checked for the correct proof type. Furthermore, the record account address also has to be included because you still need access the proof to use it in the token-2022 processor logic.

What I initially had was something like the following

enum ProofLocation {
    ContextStateAccount(Pubkey),
    InstructionData(InstructionOffset),
    AccountData(InstructionOffset, Pubkey), // need both the offset for the accompanying zk instruction and address for the record account
}

type InstructionOffset = i8;

Then I combined InstructionData and AccountData together to simplify the logic in instruction generation, but looking at it now, maybe the three-variant enum above is the clearest way to define things. What do you think?

@joncinque
Copy link
Contributor

Ah gotcha, I knew I was missing something obvious, thanks for the explanation. In that case, it looks like you've done the best thing! I'll take a full look on Monday

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.

Just a couple of small things -- once I understood how it was working, it was pretty straightforward!

.github/workflows/pull-request-token.yml Outdated Show resolved Hide resolved
token/program-2022-test/tests/program_test.rs Outdated Show resolved Hide resolved
token/program-2022/src/proof.rs Outdated Show resolved Hide resolved
token/client/src/proof_generation.rs Outdated Show resolved Hide resolved
token/client/src/token.rs Outdated Show resolved Hide resolved
token/program-2022-test/tests/confidential_transfer_fee.rs Outdated Show resolved Hide resolved
joncinque
joncinque previously approved these changes Jul 30, 2024
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.

Just one last little thing, then this can go in!

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 revert these changes?

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 forgot to turn off the auto formatter...

@mergify mergify bot dismissed joncinque’s stale review July 30, 2024 10:19

Pull request has been modified.

@samkim-crypto samkim-crypto merged commit 8e09659 into solana-labs:master Jul 30, 2024
35 checks passed
joncinque added a commit to joncinque/solana-program-library that referenced this pull request Jul 31, 2024
joncinque added a commit that referenced this pull request Jul 31, 2024
#7084)

Revert "[token-2022] Add support for reading proofs from record accounts (#7055)"

This reverts commit 8e09659.
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