chore: expose anchor discriminator separately from LightDiscriminator #2212
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThis pull request refactors discriminator functionality by renaming the default 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 |
ananas-block
force-pushed
the
jorrit/test-sdk
branch
from
c1ed796 to
f225a42
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@sdk-libs/sdk/tests/discriminator.rs`:
- Around line 46-62: Add an inline comment in the
test_discriminators_are_different test that documents the concrete discriminator
outputs for clarity: reference TestAccount::LIGHT_DISCRIMINATOR and
TestAccountAnchor::LIGHT_DISCRIMINATOR and show (as comment-only examples) the
computed formats SHA256("TestAccount")[0..8] -> <light value> and
SHA256("account:TestAccount")[0..8] -> <anchor value> so readers can immediately
see the differing byte sequences without changing test logic; place this comment
near the existing explanation lines about the differing formats.
In `@sdk-libs/sdk/tests/light_account_poseidon.rs`:
- Around line 540-584: Add a parallel test in the SHA suite that mirrors the
Poseidon test: create a TestShaAccount instance, build a
LightAccount::<TestShaAccount> via new_mut with a
PackedStateTreeInfo/CompressedAccountMeta (matching the Poseidon test setup),
call remove_data() on the account, convert it to OutAccountInfo via
to_account_info(), extract output and assert equality with an expected
OutAccountInfo where discriminator and data_hash are zeroed (and
output_merkle_tree_index, lamports, data match the Poseidon expectations). Name
the test similarly (e.g., test_remove_data_sha) and reuse the same symbols:
LightAccount, remove_data, to_account_info, OutAccountInfo, and TestShaAccount
to ensure parity between light_account_poseidon.rs and light_account_sha.rs.
In `@sdk-libs/token-sdk/tests/error_codes.rs`:
- Around line 44-48: Remove the redundant test
test_spl_interface_required_is_17500 because its assertion that
LightTokenError::SplInterfaceRequired maps to 17500 is already covered by
test_error_codes_start_at_17500; either delete the entire
test_spl_interface_required_is_17500 function or merge any unique intent into
test_error_codes_start_at_17500 so the mapping for
LightTokenError::SplInterfaceRequired remains asserted only once.
Comment on lines
+46
to
+62
| #[test] | ||
| fn test_discriminators_are_different() { | ||
| // Light format: SHA256("TestAccount")[0..8] | ||
| let light_discriminator = TestAccount::LIGHT_DISCRIMINATOR; | ||
|
|
||
| // Anchor format: SHA256("account:TestAccountAnchor")[0..8] | ||
| // Note: We can't derive both on the same struct, so we use a different struct name | ||
| // The key is that even if we manually computed SHA256("account:TestAccount"), | ||
| // it would differ from SHA256("TestAccount") | ||
| let anchor_discriminator = TestAccountAnchor::LIGHT_DISCRIMINATOR; | ||
|
|
||
| // Verify they're different (even though they have similar names) | ||
| assert_ne!( | ||
| light_discriminator, anchor_discriminator, | ||
| "Light and Anchor discriminators should produce different values" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good documentation of the test limitation.
The comment on lines 52-54 correctly explains why we can't derive both discriminator types on the same struct—both macros implement LightDiscriminator, so you'd get a trait conflict. The test serves as a sanity check that both macros work and produce different outputs, while the hardcoded value tests above actually verify the format correctness.
Consider adding a comment-based assertion showing what the Anchor format would produce for "account:TestAccount" to make the format difference more explicit:
// For reference:
// SHA256("TestAccount")[0..8] produces: light_discriminator
// SHA256("account:TestAccount")[0..8] would produce: [different value]🤖 Prompt for AI Agents
In `@sdk-libs/sdk/tests/discriminator.rs` around lines 46 - 62, Add an inline
comment in the test_discriminators_are_different test that documents the
concrete discriminator outputs for clarity: reference
TestAccount::LIGHT_DISCRIMINATOR and TestAccountAnchor::LIGHT_DISCRIMINATOR and
show (as comment-only examples) the computed formats SHA256("TestAccount")[0..8]
-> <light value> and SHA256("account:TestAccount")[0..8] -> <anchor value> so
readers can immediately see the differing byte sequences without changing test
logic; place this comment near the existing explanation lines about the
differing formats.
Comment on lines
+44
to
+48
| #[test] | ||
| fn test_spl_interface_required_is_17500() { | ||
| let code: u32 = LightTokenError::SplInterfaceRequired.into(); | ||
| assert_eq!(code, 17500, "SplInterfaceRequired must be 17500"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Redundant test case.
test_spl_interface_required_is_17500 duplicates the assertion already made in test_error_codes_start_at_17500 (lines 10-17). Consider removing this test or consolidating them.
♻️ Suggested consolidation
-#[test]
-fn test_spl_interface_required_is_17500() {
- let code: u32 = LightTokenError::SplInterfaceRequired.into();
- assert_eq!(code, 17500, "SplInterfaceRequired must be 17500");
-}The test_error_codes_start_at_17500 test already verifies this exact assertion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn test_spl_interface_required_is_17500() { | |
| let code: u32 = LightTokenError::SplInterfaceRequired.into(); | |
| assert_eq!(code, 17500, "SplInterfaceRequired must be 17500"); | |
| } |
🤖 Prompt for AI Agents
In `@sdk-libs/token-sdk/tests/error_codes.rs` around lines 44 - 48, Remove the
redundant test test_spl_interface_required_is_17500 because its assertion that
LightTokenError::SplInterfaceRequired maps to 17500 is already covered by
test_error_codes_start_at_17500; either delete the entire
test_spl_interface_required_is_17500 function or merge any unique intent into
test_error_codes_start_at_17500 so the mapping for
LightTokenError::SplInterfaceRequired remains asserted only once.
e062cef to
55e9e88
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk-libs/macros/src/light_pdas/account/light_compressible.rs (1)
5-65:⚠️ Potential issue | 🟠 MajorLightCompressible now emits Anchor-format discriminators — confirm this breaking change. Switching to
anchor_discriminator()changes the derived hash input from"{name}"to"account:{name}", which will make existing LightCompressible accounts fail discriminator checks and deserialization if they were created with the prior format. If the intent is to keep LightCompressible aligned with the Light format, revert tolight_discriminator, or make the Anchor format an explicit/opt‑in variant.🔧 Minimal revert if Anchor format is not intended here
-use crate::{ - discriminator::anchor_discriminator, +use crate::{ + discriminator::light_discriminator, hasher::derive_light_hasher_sha, light_pdas::account::{pack_unpack::derive_compressible_pack, traits::derive_compressible}, }; @@ - let discriminator_impl = anchor_discriminator(item_struct)?; + let discriminator_impl = light_discriminator(item_struct)?;
🤖 Fix all issues with AI agents
In `@sdk-libs/compressed-token-sdk/Cargo.toml`:
- Around line 9-12: Restore the default anchor-discriminator feature in the
Cargo.toml features block: add "anchor-discriminator" to the default features
array (e.g., default = ["anchor-discriminator"]) and ensure the "anchor" feature
list and/or feature declarations include "anchor-discriminator" as needed so
Anchor-style discriminators remain enabled by default; update the features
section entries (default, v1, anchor) accordingly to reference
"anchor-discriminator".
In `@sdk-libs/token-sdk/tests/address_derivation.rs`:
- Around line 88-106: Replace the hardcoded pool seed in the test with the
shared constant: import and use POOL_SEED from light_token_types instead of
using let pool_seed: &[u8] = b"pool"; so that get_spl_interface_pda_and_bump and
the test both derive addresses from the same POOL_SEED when calling
Pubkey::find_program_address with mint.as_ref() and LIGHT_TOKEN_PROGRAM_ID.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk-libs/macros/src/discriminator.rs (1)
6-53: 🧹 Nitpick | 🔵 TrivialConsider extracting shared discriminator generation logic.
Both
light_discriminatorandanchor_discriminatorshare identical structure—the only difference is howhash_inputis computed. This duplication is minor but could drift over time if someone updates one function and forgets the other.♻️ Optional: Extract common logic into a helper
+fn generate_discriminator_impl(input: ItemStruct, hash_input: String) -> Result<TokenStream> { + let account_name = &input.ident; + let (impl_gen, type_gen, where_clause) = input.generics.split_for_impl(); + + let mut discriminator = [0u8; 8]; + discriminator.copy_from_slice(&Sha256::hash(hash_input.as_bytes()).unwrap()[..8]); + let discriminator: proc_macro2::TokenStream = format!("{discriminator:?}").parse().unwrap(); + + Ok(quote! { + impl `#impl_gen` LightDiscriminator for `#account_name` `#type_gen` `#where_clause` { + const LIGHT_DISCRIMINATOR: [u8; 8] = `#discriminator`; + const LIGHT_DISCRIMINATOR_SLICE: &'static [u8] = &Self::LIGHT_DISCRIMINATOR; + + fn discriminator() -> [u8; 8] { + Self::LIGHT_DISCRIMINATOR + } + } + }) +} + pub(crate) fn light_discriminator(input: ItemStruct) -> Result<TokenStream> { - let account_name = &input.ident; - let hash_input = account_name.to_string(); - // ... rest of implementation + let hash_input = input.ident.to_string(); + generate_discriminator_impl(input, hash_input) } pub(crate) fn anchor_discriminator(input: ItemStruct) -> Result<TokenStream> { - let account_name = &input.ident; - let hash_input = format!("account:{}", account_name); - // ... rest of implementation + let hash_input = format!("account:{}", input.ident); + generate_discriminator_impl(input, hash_input) }
ananas-block
force-pushed
the
jorrit/test-sdk
branch
from
dbcd75b to
f622082
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@sdk-libs/sdk/tests/light_account_sha.rs`:
- Around line 17-18: Remove the unused import LightDiscriminator from the import
list where AnchorDiscriminator and LightDiscriminator are imported; locate the
import statement referencing LightDiscriminator (the one used alongside
AnchorDiscriminator) and delete LightDiscriminator, or if you intentionally keep
it for documentation, add a brief comment next to the import explaining why it
remains; ensure this change leaves the TestShaAccount/AnchorDiscriminator
derives untouched.
In `@sdk-libs/token-sdk/tests/instruction_transfer.rs`:
- Around line 35-38: Create a small helper to build instruction payloads instead
of inlining byte Vecs: add a function like transfer_data(amount: u64) -> Vec<u8>
(or a generic build_instruction(discriminator: u8, amount: u64) -> Vec<u8>) and
replace the inline arrays (the vec![3u8, 100,0,0,0,0,0,0,0] and other similar
occurrences) with calls to that helper in the tests (e.g., where the Transfer
discriminator 3u8 is used); ensure the helper writes the discriminator as the
first byte and encodes the u64 amount as little-endian bytes so existing
assertions remain valid.
- Line 33: Replace uses of Pubkey::default() for the system program account in
the Transfer instruction account metas with the real Solana system program ID:
import solana_sdk::system_program and use system_program::ID wherever
AccountMeta::new_readonly(Pubkey::default(), false) (or similar) is used for the
system program; update both the implementation in the transfer instruction
generator (e.g., the function building the Transfer instruction in transfer.rs)
and the test assertions in instruction_transfer.rs to reference
system_program::ID instead of Pubkey::default().
In `@sdk-libs/token-sdk/tests/transfer_type.rs`:
- Around line 3-7: The tests import and use the non‑CPI TransferInterface but
our guideline requires using TransferInterfaceCpi for auto‑detecting token
transfer operations; replace the use and import of TransferInterface with
TransferInterfaceCpi in the test (update the use list to import
TransferInterfaceCpi), and adapt the test fixtures/constructors where
TransferInterface was created so they provide the CPI context (program id,
accounts/authority shapes, and any CPI-specific constructor or builder method)
required by TransferInterfaceCpi; if you intentionally want to test the non‑CPI
builder instead, add a short comment in the test explaining that choice and why
TransferInterfaceCpi was not used.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk-libs/macros/src/light_pdas/account/derive.rs (1)
1-9: 🧹 Nitpick | 🔵 TrivialUpdate doc comment to reflect anchor-style discriminator.
The module doc at line 5 says "LightDiscriminator (unique 8-byte discriminator)" but doesn't mention that this derive now produces anchor-compatible discriminators (
SHA256("account:{name}")). Consider clarifying this for users.📝 Suggested doc update
//! This macro generates: //! - `LightHasherSha` (SHA256 hashing via DataHasher + ToByteArray) -//! - `LightDiscriminator` (unique 8-byte discriminator) +//! - `LightDiscriminator` (Anchor-compatible 8-byte discriminator: SHA256("account:{name}")[0..8])
🤖 Fix all issues with AI agents
In `@sdk-libs/macros/src/light_pdas/account/derive.rs`:
- Around line 86-87: Update the doc comment for the LightAccount derive to
explicitly state that LightDiscriminator is an Anchor-compatible 8-byte
discriminator computed as SHA256("account:{name}")[0..8]; modify the
documentation text (the top-of-file comment where LightAccount traits are
described) to replace or augment the LightDiscriminator bullet with:
"LightDiscriminator (Anchor-compatible 8-byte discriminator via
SHA256(\"account:{name}\")[0..8])". Verify the wording near where
discriminator::anchor_discriminator(item_struct) is used so the docs match the
implementation and mention the exact format string "account:{name}".
In `@sdk-libs/token-sdk/tests/error_codes.rs`:
- Around line 50-111: The test manually asserts each LightTokenError variant has
a non-empty display; refactor to iterate so new variants are automatically
covered—either derive strum::IntoEnumIterator for LightTokenError and use
LightTokenError::iter() to loop and assert !error.to_string().is_empty()
(updating Cargo.toml to add strum and strum_macros), or, if you prefer not to
add a dependency, create a static array of all LightTokenError variants and
iterate that in the test; update the test function name
test_error_display_messages to use the iterator/array and keep the same
assertion message referencing the error (e.g., "{:?} must have a non-empty
display message") so failures identify the variant.
SwenSchaeferjohann
approved these changes
Feb 2, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- New proc_macro_derive attribute with documentation
- light_hasher.rs: Changed 2 references from discriminator() to light_discriminator()
- light_compressible.rs: Changed import and function call to light_discriminator()
- sdk-libs/macros/Cargo.toml
- sdk-libs/compressed-token-sdk/Cargo.toml
- sdk-libs/sdk/Cargo.toml
- light-sdk/src/lib.rs: Export from light_sdk_macros
- light-token/src/anchor.rs: Export from light_sdk_macros
- Remove from light-sdk dependency features in all 5 sdk-tests packages
- light_account_sha.rs: Changed from LightDiscriminator to AnchorDiscriminator
- light_account_poseidon.rs: Changed from LightDiscriminator to AnchorDiscriminator
- TEST_SHA_DISCRIMINATOR: [57, 91, 172, 77, ...] → [133, 206, 241, 201, ...]
- TEST_POSEIDON_DISCRIMINATOR: [174, 23, 156, 239, ...] → [250, 202, 237, 234, ...]
- 5 tests verifying both LightDiscriminator and AnchorDiscriminator formats and values
- test_light_discriminator: Verify SHA256("{name}") format
- test_anchor_discriminator: Verify SHA256("account:{name}") format
Summary by CodeRabbit
Release Notes
New Features
Changes
anchor-discriminatoras default feature; SDKs now use a minimal default configuration.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.