Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test coverage across five modules including token state validation, indexer batch operations, configuration defaults, proof packing logic, and transaction size handling. No functional runtime code changes are introduced; all modifications are test-only additions with improved assertion coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Contributor
Author
|
@CodeRabbit review this pr pls |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forester/tests/test_indexer_interface.rs (1)
518-599:⚠️ Potential issue | 🟡 MinorBatch test no longer covers the "not-found" path
The previous version of Test 3 apparently included accounts that would return
None(e.g.,compressed_mint_pdawhich is fully compressed and has no on-chain representation). The updated test uses only hot accounts, so the code path wheregetMultipleAccountInterfacesreturns aNoneentry for a missing/cold account is no longer exercised anywhere in this test.Consider keeping at least one entry that is expected to be
None(e.g.,&compressed_mint_pda) to ensure the batch endpoint handles mixed hot/missing results correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forester/tests/test_indexer_interface.rs` around lines 518 - 599, The batch test no longer checks the "not-found" path; modify the batch_addresses passed to photon_indexer.get_multiple_account_interfaces to include &compressed_mint_pda (e.g., [ &decompressed_mint_pda, &compressible_token_account, &compressed_mint_pda, &bob_ata, &charlie_ata ] or swap into the existing 4-item vector), then update the assertions on batch_response.value to expect a None (i.e., .as_ref() should be None) for the compressed_mint_pda slot and shift the subsequent assertions for bob_ata/charlie_ata to their new indices so the test verifies a mixed hot/missing result set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@forester/tests/test_compressible_ctoken.rs`:
- Around line 328-346: The current assert_eq! uses extensions:
account_state.account.extensions.clone(), which tautologically makes the
extensions field always match; update the test to make a real assertion: either
remove the extensions field from the expected Token literal and keep the
existing assert!(account_state.account.extensions.is_some()) check, or replace
that field with the concrete expected extensions value (e.g., the known
CompressibleToken extension data) and assert equality against it; locate the
Token struct construction in test_compressible_ctoken.rs (the assert_eq!
comparing account_state.account) and modify the extensions handling accordingly.
In `@forester/tests/test_indexer_interface.rs`:
- Around line 520-527: The comment above the batch_addresses vector is stale and
mentions "compressed mint PDA (not found)" and "compressed mint seed pubkey"
which no longer match the actual items; update the comment to accurately
describe the current contents: decompressed_mint_pda,
compressible_token_account, bob_ata, and charlie_ata (both on-chain hot
accounts). Locate the comment near batch_addresses and replace the old
descriptions with a brief, correct list referencing decompressed_mint_pda,
compressible_token_account, bob_ata, and charlie_ata.
In `@sdk-libs/client/src/interface/tx_size.rs`:
- Around line 196-199: The comment next to max_size in tx_size.rs miscounts
instruction accounts: each instruction adds a third key created via
AccountMeta::new(...), so replace the misleading "accounts(compact+32*2)" part
with "accounts(compact+32*3)" and update the byte math to reflect header(3) +
accounts(compact+32*3) + blockhash(32) + ixs + sigs ≈ 212 bytes for
one-instruction tx (hence max_size = 250 still allows one but not two); update
the inline numbers to show ~212 for one and ~290 for two so the comment matches
the actual test logic using max_size.
---
Outside diff comments:
In `@forester/tests/test_indexer_interface.rs`:
- Around line 518-599: The batch test no longer checks the "not-found" path;
modify the batch_addresses passed to
photon_indexer.get_multiple_account_interfaces to include &compressed_mint_pda
(e.g., [ &decompressed_mint_pda, &compressible_token_account,
&compressed_mint_pda, &bob_ata, &charlie_ata ] or swap into the existing 4-item
vector), then update the assertions on batch_response.value to expect a None
(i.e., .as_ref() should be None) for the compressed_mint_pda slot and shift the
subsequent assertions for bob_ata/charlie_ata to their new indices so the test
verifies a mixed hot/missing result set.
Comment on lines
328
to
346
| assert_eq!( | ||
| account_state.account.owner, | ||
| owner_keypair.pubkey().to_bytes() | ||
| account_state.account, | ||
| Token { | ||
| mint: mint.to_bytes().into(), | ||
| owner: owner_keypair.pubkey().to_bytes().into(), | ||
| amount: 0, | ||
| delegate: None, | ||
| state: AccountState::Initialized, | ||
| is_native: None, | ||
| delegated_amount: 0, | ||
| close_authority: None, | ||
| account_type: ACCOUNT_TYPE_TOKEN_ACCOUNT, | ||
| extensions: account_state.account.extensions.clone(), | ||
| } | ||
| ); | ||
| assert!( | ||
| account_state.account.extensions.is_some(), | ||
| "compressible token account should have extensions" | ||
| ); |
Contributor
There was a problem hiding this comment.
Tautological extensions comparison in assert_eq!
Line 340 sets extensions: account_state.account.extensions.clone(), so that field trivially matches and contributes nothing to the equality assertion. The test looks as though it's verifying the extensions value, but it's only verifying all the other fields.
The separate is_some() guard on lines 343–346 is the sole real assertion for extensions. To make the intent clear:
🔧 Suggested refactor
assert_eq!(
account_state.account,
Token {
mint: mint.to_bytes().into(),
owner: owner_keypair.pubkey().to_bytes().into(),
amount: 0,
delegate: None,
state: AccountState::Initialized,
is_native: None,
delegated_amount: 0,
close_authority: None,
account_type: ACCOUNT_TYPE_TOKEN_ACCOUNT,
- extensions: account_state.account.extensions.clone(),
+ // extensions is not asserted here; verified separately below
+ ..account_state.account.clone()
}
);Or, if Token supports a field-by-field comparison without struct-literal syntax, exclude extensions entirely and keep the dedicated is_some() check:
+assert!(account_state.account.extensions.is_some(), "compressible token account should have extensions");
assert_eq!(account_state.account.amount, 0);
assert_eq!(account_state.account.state, AccountState::Initialized);
// … individual field assertions …🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forester/tests/test_compressible_ctoken.rs` around lines 328 - 346, The
current assert_eq! uses extensions: account_state.account.extensions.clone(),
which tautologically makes the extensions field always match; update the test to
make a real assertion: either remove the extensions field from the expected
Token literal and keep the existing
assert!(account_state.account.extensions.is_some()) check, or replace that field
with the concrete expected extensions value (e.g., the known CompressibleToken
extension data) and assert equality against it; locate the Token struct
construction in test_compressible_ctoken.rs (the assert_eq! comparing
account_state.account) and modify the extensions handling accordingly.
Comment on lines
+520
to
+527
| // Include: decompressed mint PDA, compressible token account, compressed mint PDA (not found), | ||
| // and the compressed mint seed pubkey (not a known on-chain account). | ||
| let batch_addresses = vec![ | ||
| &decompressed_mint_pda, | ||
| &compressible_token_account, | ||
| &bob_ata, | ||
| &charlie_ata, | ||
| ]; |
Contributor
There was a problem hiding this comment.
Stale comment describes the wrong accounts
The comment at lines 520–521 still refers to the previous batch contents (compressed mint PDA (not found) and compressed mint seed pubkey), but the actual batch_addresses now uses bob_ata and charlie_ata — both fully on-chain hot accounts.
✏️ Suggested fix
- // Include: decompressed mint PDA, compressible token account, compressed mint PDA (not found),
- // and the compressed mint seed pubkey (not a known on-chain account).
+ // Include: decompressed mint PDA (on-chain CMint), compressible token account (on-chain),
+ // Bob's ATA (on-chain), and Charlie's ATA (on-chain).
let batch_addresses = vec![📝 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
| // Include: decompressed mint PDA, compressible token account, compressed mint PDA (not found), | |
| // and the compressed mint seed pubkey (not a known on-chain account). | |
| let batch_addresses = vec![ | |
| &decompressed_mint_pda, | |
| &compressible_token_account, | |
| &bob_ata, | |
| &charlie_ata, | |
| ]; | |
| // Include: decompressed mint PDA (on-chain CMint), compressible token account (on-chain), | |
| // Bob's ATA (on-chain), and Charlie's ATA (on-chain). | |
| let batch_addresses = vec![ | |
| &decompressed_mint_pda, | |
| &compressible_token_account, | |
| &bob_ata, | |
| &charlie_ata, | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@forester/tests/test_indexer_interface.rs` around lines 520 - 527, The comment
above the batch_addresses vector is stale and mentions "compressed mint PDA (not
found)" and "compressed mint seed pubkey" which no longer match the actual
items; update the comment to accurately describe the current contents:
decompressed_mint_pda, compressible_token_account, bob_ata, and charlie_ata
(both on-chain hot accounts). Locate the comment near batch_addresses and
replace the old descriptions with a brief, correct list referencing
decompressed_mint_pda, compressible_token_account, bob_ata, and charlie_ata.
Comment on lines
+196
to
+199
| // Each instruction has 1 account + 10 bytes data. Estimate: | ||
| // header(3) + accounts(compact+32*2) + blockhash(32) + ixs + sigs ~ 200 bytes | ||
| // Set max_size=250 to allow one instruction per tx but not two. | ||
| let max_size = 250usize; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inaccurate account-count estimate in comment
The inline comment says accounts(compact+32*2), implying 2 keys (payer + program_id). Each instruction also contributes a third key via AccountMeta::new(Pubkey::new_unique(), false), so the real account list is [payer, program_id, account] = 3 × 32 = 96 bytes, giving ~212 bytes per single-instruction tx — not ~200. The test logic and assertions are still correct since 212 < 250 (one fits) and ~290 bytes for two (split forced), but the comment formula misleads future readers.
✏️ Suggested comment fix
- // Each instruction has 1 account + 10 bytes data. Estimate:
- // header(3) + accounts(compact+32*2) + blockhash(32) + ixs + sigs ~ 200 bytes
- // Set max_size=250 to allow one instruction per tx but not two.
+ // Each instruction has 1 account (non-signer) + 10 bytes data. Estimate per single tx:
+ // header(3) + accounts(compact+3×32=97) + blockhash(32) + ix(~14) + sigs(65) ≈ 211 bytes
+ // Two instructions share no accounts → ≈ 290 bytes.
+ // max_size=250 allows one instruction per batch but not two → 3 batches for 3 instructions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk-libs/client/src/interface/tx_size.rs` around lines 196 - 199, The comment
next to max_size in tx_size.rs miscounts instruction accounts: each instruction
adds a third key created via AccountMeta::new(...), so replace the misleading
"accounts(compact+32*2)" part with "accounts(compact+32*3)" and update the byte
math to reflect header(3) + accounts(compact+32*3) + blockhash(32) + ixs + sigs
≈ 212 bytes for one-instruction tx (hence max_size = 250 still allows one but
not two); update the inline numbers to show ~212 for one and ~290 for two so the
comment matches the actual test logic using max_size.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit