chore: compressed repr. should store disc as part of data#2282
chore: compressed repr. should store disc as part of data#2282SwenSchaeferjohann wants to merge 3 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThe PR changes how compressed/decompressed account bytes are constructed and hashed: account data is now used directly when available, and SHA-256 inputs are prefixed with an 8-byte on-chain discriminator before hashing. No public API signatures were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk-libs/sdk-types/src/interface/program/compression/pda.rs (1)
142-148:⚠️ Potential issue | 🔴 CriticalInconsistency in
OutAccountInfo.dataformat betweenpda.rsandaccount.rspaths.The data field format is inconsistent across code paths. In
pda.rs(decompression),OutAccountInfo.dataincludes the 8-byte discriminator prefix (disc + borsh), with data_hash calculated over both. However, inaccount.rs(modification viato_account_info()),OutAccountInfo.datacontains only the borsh serialization without the discriminator prefix, and data_hash is calculated on the account object directly.Both paths populate the same
OutAccountInfostructure, so this format mismatch will break downstream consumers. Additionally, the data_hash calculations differ between paths (pda.rs hashes disc+borsh, account.rs hashes account), which could cause validation failures. Updateaccount.rsto mirror the pda.rs layout: include the discriminator prefix in the data field and recalculate data_hash consistently.
SwenSchaeferjohann
force-pushed
the
swen/fix-disc-for-accountinterface
branch
from
6a28d8f to
5bb0505
Compare
| // Serialize updated account data back (includes 8-byte discriminator) | ||
| { | ||
| let mut data = account_info | ||
| .try_borrow_mut_data() | ||
| .map_err(LightSdkTypesError::AccountError)?; | ||
| // Write discriminator first | ||
| data[..8].copy_from_slice(&A::LIGHT_DISCRIMINATOR); | ||
| // Write serialized account data after discriminator | ||
| let writer = &mut &mut data[8..]; | ||
| account_data | ||
| .serialize(writer) | ||
| .map_err(|_| LightSdkTypesError::Borsh)?; | ||
| } |
| // Hash the data (discriminator NOT included per protocol convention) | ||
| let data_bytes = borsh::to_vec(&compressed_data).map_err(|_| LightSdkTypesError::Borsh)?; | ||
| // Serialize with disc prefix: disc(8) + borsh(struct) — mirrors on-chain layout. | ||
| let borsh_bytes = borsh::to_vec(&compressed_data).map_err(|_| LightSdkTypesError::Borsh)?; |
There was a problem hiding this comment.
Note for a future pr:
I don't like this we are deserializing the complete struct, use CompressionInfo then serialize everything back. I think it would be cleaner to add a trait CompressData that defines what data is copied from the account info to the compressed account.
|
looks ok, we don't have any asserts that check everything compresses and decompresses as expected. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdk-libs/client/src/interface/account_interface.rs (2)
68-85: 🛠️ Refactor suggestion | 🟠 MajorMissing round-trip test coverage for the new data layout.
As noted in the review comments, there are no assertions verifying that compress → store → decompress produces correct results with the new disc-prefixed data layout. Given that this change affects hashing (which is security-critical for Merkle tree integrity) and data reconstruction, a round-trip integration test would catch any asymmetry between the compression/decompression paths early.
Consider adding a test that:
- Compresses an account (producing
disc || borshdata + hash)- Stores it via
AccountInterface::cold()- Verifies
data()returns the expecteddisc || borshbytes- Decompresses and verifies the hash matches
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/client/src/interface/account_interface.rs` around lines 68 - 85, Add a round-trip integration test that verifies the new disc-prefixed layout: create an Account and compress it to obtain a CompressedAccount (with disc || borsh bytes and its hash), call AccountInterface::cold(key, compressed, owner), assert that the resulting AccountInterface.data() (or the stored Account.account.data field) equals the expected disc || borsh bytes, then decompress the stored CompressedAccount and assert the decompressed bytes and recomputed hash equal the original hash; use the CompressedAccount, AccountInterface::cold, AccountInterface::data (or Account) and the same compression/decompression helpers used in production so the test catches any asymmetry between compress and decompress.
100-104:⚠️ Potential issue | 🔴 Critical
as_mint()will fail to deserialize cold mints — discriminator must be prepended.The
cold()method extracts onlycompressed.data.as_ref().map(|d| d.data.clone()), which omits the 8-byte discriminator that's stored separately inCompressedAccountData::discriminator. However,as_mint()tries to deserialize this data directly, expecting the discriminator to be present (as shown indecompress_mint.rs, which explicitly prepends it when constructing full account bytes). Cold accounts will fail deserialization; fix by prepending the discriminator inas_mint():pub fn as_mint(&self) -> Option<light_token_interface::state::Mint> { let ca = self.cold.as_ref()?; let cad = ca.data.as_ref()?; let mut buf = cad.discriminator.to_vec(); buf.extend_from_slice(&cad.data); borsh::BorshDeserialize::deserialize(&mut buf.as_slice()).ok() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/client/src/interface/account_interface.rs` around lines 100 - 104, The as_mint() implementation must prepend the 8-byte discriminator from CompressedAccountData::discriminator when deserializing cold accounts because cold() currently returns only compressed.data.as_ref().map(|d| d.data.clone()); update as_mint() to fetch self.cold, get ca.data, create a buffer starting with cad.discriminator (as bytes), extend it with cad.data, and then call borsh::BorshDeserialize::deserialize on that combined buffer (returning Option by mapping ok()); ensure you reference as_mint(), cold(), and CompressedAccountData::discriminator when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sdk-libs/client/src/interface/account_interface.rs`:
- Around line 68-85: Add a round-trip integration test that verifies the new
disc-prefixed layout: create an Account and compress it to obtain a
CompressedAccount (with disc || borsh bytes and its hash), call
AccountInterface::cold(key, compressed, owner), assert that the resulting
AccountInterface.data() (or the stored Account.account.data field) equals the
expected disc || borsh bytes, then decompress the stored CompressedAccount and
assert the decompressed bytes and recomputed hash equal the original hash; use
the CompressedAccount, AccountInterface::cold, AccountInterface::data (or
Account) and the same compression/decompression helpers used in production so
the test catches any asymmetry between compress and decompress.
- Around line 100-104: The as_mint() implementation must prepend the 8-byte
discriminator from CompressedAccountData::discriminator when deserializing cold
accounts because cold() currently returns only compressed.data.as_ref().map(|d|
d.data.clone()); update as_mint() to fetch self.cold, get ca.data, create a
buffer starting with cad.discriminator (as bytes), extend it with cad.data, and
then call borsh::BorshDeserialize::deserialize on that combined buffer
(returning Option by mapping ok()); ensure you reference as_mint(), cold(), and
CompressedAccountData::discriminator when making the change.
3 participants
Summary by CodeRabbit
Bug Fixes
Documentation