Skip to content

Conversation

dvrvsimi
Copy link
Contributor

@dvrvsimi dvrvsimi commented Jun 1, 2025

Summary

This PR introduces a new helper for querying and decrypting the pending balance of a confidential token account, and adds comprehensive tests for confidential transfer account info logic.

Changes

  • Adds get_pending_balance and related helpers to ApplyPendingBalanceAccountInfo.
  • Adds robust tests for:
    • Pending balance decryption and combination
    • Available balance decryption and error handling
    • Total balance calculation and overflow handling
    • Utility function combine_balances (including overflow cases)
  • Ensures all cryptographic edge cases and error paths are covered.

Motivation

  • Makes it easier for client code to query pending balances in confidential transfer accounts, such cases.

@samkim-crypto samkim-crypto self-requested a review July 31, 2025 00:29
Copy link
Contributor

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

@dvrvsimi thanks for the contribution and sorry that I missed this PR. These are good changes and looks good to me overall.

What do you think about the following?

  • Add functions in the rust token-client for decrypting pending and available balance
  • For consistency, move the tests that you added to the rust client tests instead using the client function above.

@dvrvsimi
Copy link
Contributor Author

dvrvsimi commented Aug 2, 2025

@dvrvsimi thanks for the contribution and sorry that I missed this PR. These are good changes and looks good to me overall.

What do you think about the following?

  • Add functions in the rust token-client for decrypting pending and available balance
  • For consistency, move the tests that you added to the rust client tests instead using the client function above.

@samkim-crypto thank you for the review, I think these are solid suggestions and I have edited the PR accordingly.

In one of the tests (test_confidential_transfer_balance_decryption_with_large_values), I noticed that any ElGamal key can successfully decrypt pending balances that were encrypted with a different key. This might be a security issue.

@samkim-crypto samkim-crypto self-requested a review August 19, 2025 08:38
Copy link
Contributor

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

@dvrvsimi I am terribly sorry for the delay on this. Just returning from a pto.

With the way you have set up the test, the decryption with multiple key is actually expected behavior. When we deposit a public balance to a confidential balance, the amount is "encoded" (instead of "encrypted"). An encoding is essentially an encryption with the randomness value set to 0. This was done to save space in the instruction data of deposit. If this value were encrypted, then the sender would need to include a ciphertext inside the instruction data.

An encoding can be decrypted by any key. Deposit itself is a public operation: anyone can compare the public balance before and after the deposit transaction to deduce how much was actually deposited, so this is not a security issue.

It will be a bit more involved, but a proper test would set up a sender and a receiver. A sender sends tokens to the receiver. The test would then decrypt the receiver's pending balance. Unlike deposit, a proper confidential transfer will perfectly encrypt the pending balance, so the resulting pending balance ciphertext can only be decrypted by a proper key.

Also, if you can run cargo fmt on your changes and address any issues after running cargo clippy, then that will help me with the reviews as well. Thanks for your contribution!

@samkim-crypto samkim-crypto self-requested a review August 19, 2025 10:01
@dvrvsimi
Copy link
Contributor Author

dvrvsimi commented Aug 19, 2025

@dvrvsimi I am terribly sorry for the delay on this. Just returning from a pto.

With the way you have set up the test, the decryption with multiple key is actually expected behavior. When we deposit a public balance to a confidential balance, the amount is "encoded" (instead of "encrypted"). An encoding is essentially an encryption with the randomness value set to 0. This was done to save space in the instruction data of deposit. If this value were encrypted, then the sender would need to include a ciphertext inside the instruction data.

An encoding can be decrypted by any key. Deposit itself is a public operation: anyone can compare the public balance before and after the deposit transaction to deduce how much was actually deposited, so this is not a security issue.

It will be a bit more involved, but a proper test would set up a sender and a receiver. A sender sends tokens to the receiver. The test would then decrypt the receiver's pending balance. Unlike deposit, a proper confidential transfer will perfectly encrypt the pending balance, so the resulting pending balance ciphertext can only be decrypted by a proper key.

Also, if you can run cargo fmt on your changes and address any issues after running cargo clippy, then that will help me with the reviews as well. Thanks for your contribution!

It's alright, welcome back!

You are right, deposit amounts are already publicly deducible. Proper test flow added (test_confidential_transfer_pending_decryption_after_transfer) and both fmt and clippy issues have been handed.

Copy link
Contributor

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! The tests are very thorough and clear!

I just have one question below regarding what appears to be a mistake line change.

Also, when I run pnpm rust:spellcheck, then I get some errors, which causes the CI to fail. Can you add some works to https://github.com/solana-program/token-2022/blob/main/scripts/solana.dic to satisfy the CI.

Other than these, everything looks good to me!

@samkim-crypto samkim-crypto self-requested a review August 20, 2025 01:07
@dvrvsimi
Copy link
Contributor Author

Thanks for the updates! The tests are very thorough and clear!

I just have one question below regarding what appears to be a mistake line change.

Also, when I run pnpm rust:spellcheck, then I get some errors, which causes the CI to fail. Can you add some works to https://github.com/solana-program/token-2022/blob/main/scripts/solana.dic to satisfy the CI.

Other than these, everything looks good to me!

I have removed the line change and updated solana.dic, thank you!

Copy link
Contributor

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

Great! Looks good to me. Thanks a lot for these changes!

@samkim-crypto samkim-crypto merged commit 725254c into solana-program:main Aug 21, 2025
23 checks passed
@dvrvsimi
Copy link
Contributor Author

Great! Looks good to me. Thanks a lot for these changes!

thank you for the review, i was able to learn

@dvrvsimi dvrvsimi deleted the query_pending_balance branch August 21, 2025 15:02
@dvrvsimi
Copy link
Contributor Author

hello @samkim-crypto , i am reaching out here because i couldn't find a way to contact you

i found some potential issues i'd like to report and i have submitted here but i saw a PR that you opened and was wondering if i still have to create an advisory like the SECURITY.md suggests

@samkim-crypto
Copy link
Contributor

@dvrvsimi Both places are fine. If you already submitted in code4arena, then you don't need to bother creating another advisory.

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