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 PR refactors the discriminator system by removing the conditional Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
+540
to
+584
| /// Test remove_data functionality. | ||
| #[test] | ||
| fn test_remove_data() { | ||
| let program_id = Pubkey::new_from_array([2u8; 32]); | ||
| let tree_info = PackedStateTreeInfo { | ||
| root_index: 0, | ||
| prove_by_index: false, | ||
| merkle_tree_pubkey_index: 0, | ||
| queue_pubkey_index: 1, | ||
| leaf_index: 100, | ||
| }; | ||
| let account_meta = CompressedAccountMeta { | ||
| tree_info, | ||
| address: [3u8; 32], | ||
| output_state_tree_index: 0, | ||
| }; | ||
|
|
||
| let mut account = LightAccount::<TestPoseidonAccount>::new_mut( | ||
| &program_id, | ||
| &account_meta, | ||
| TestPoseidonAccount::default(), | ||
| ) | ||
| .expect("Failed to create LightAccount"); | ||
|
|
||
| account.remove_data(); | ||
|
|
||
| let account_info = account | ||
| .to_account_info() | ||
| .expect("Should convert to account info"); | ||
|
|
||
| let output = account_info.output.expect("Should have output"); | ||
|
|
||
| // After remove_data, output should have zeroed hash and discriminator | ||
| let expected_output = OutAccountInfo { | ||
| discriminator: [0u8; 8], | ||
| data_hash: [0u8; 32], | ||
| output_merkle_tree_index: 0, | ||
| lamports: 0, | ||
| data: vec![], | ||
| }; | ||
| assert_eq!( | ||
| output, expected_output, | ||
| "Output after remove_data should have zeroed data_hash and discriminator" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
remove_data test provides useful coverage.
This test verifies that remove_data() zeroes both the discriminator and data_hash in the output. Consider adding an equivalent test to light_account_sha.rs for parity between the test suites.
🤖 Prompt for AI Agents
In `@sdk-libs/sdk/tests/light_account_poseidon.rs` around lines 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.
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.
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
AnchorDiscriminatorderive macro as an alternative discriminator implementation alongsideLightDiscriminator.Tests
Chores
anchor-discriminatorfeature flag; functionality now available via explicitAnchorDiscriminatormacro.✏️ Tip: You can customize this high-level summary in your review settings.