Skip to content

fix: failed Solana tx fee entries and base58 wallet filter#161

Merged
user1303836 merged 2 commits intomainfrom
fix/solana-parser-wallet-filter-bugs-145-143
Mar 13, 2026
Merged

fix: failed Solana tx fee entries and base58 wallet filter#161
user1303836 merged 2 commits intomainfrom
fix/solana-parser-wallet-filter-bugs-145-143

Conversation

@user1303836
Copy link
Owner

Summary

  • bug: Solana parser skips failed transactions entirely, losing fee entries #145 — Solana parser skips failed transactions entirely, losing fee entries: Failed Solana transactions still deduct SOL fees from the fee payer, but the parser was returning empty results for them. Now the fee entry is always emitted for the fee payer, while transfer and SPL token extraction remain correctly skipped for failed transactions. The native-balance-delta extractor (extract_solana_native_balance_deltas) also no longer drops failed transactions, so the fee payer's balance change is captured.
  • bug: allowed_wallets lowercases Solana base58 addresses, breaking filtering #143allowed_wallets lowercases Solana base58 addresses, breaking filtering: The config helper normalised every address to lowercase, which corrupts case-sensitive Solana base58 addresses. Normalisation is now chain-aware: only 0x-prefixed (EVM) addresses are lowercased; all other formats (Solana base58, Hyperliquid, etc.) are preserved as-is. The runtime check_wallet_allowed function uses the same logic.

Test plan

  • cargo fmt --all --check passes
  • cargo clippy --workspace --all-targets -- -D warnings passes
  • cargo test --workspace — all tests pass (one pre-existing flaky test test_semaphore_limits_concurrent_jobs fails on main as well, unrelated to these changes)
  • New unit tests: test_parse_failed_tx_emits_fee_entry, test_extract_native_balance_deltas_failed_tx, test_extract_token_transfers_failed_tx_skip
  • Updated integration test: test_failed_transaction_emits_fee (was test_failed_transaction_skipped)
  • New config tests: test_allowed_wallets_set_lowercases_evm_only
  • New API tests: test_check_wallet_allowed_evm_case_insensitive, test_check_wallet_allowed_solana_case_sensitive

🤖 Generated with Claude Code

…et filter

Failed Solana transactions still deduct SOL fees from the fee payer, but
the parser was returning an empty ledger for them. Now the fee entry is
always emitted for the fee payer, while transfer and SPL token extraction
remain skipped for failed transactions. The native-balance-delta extractor
also no longer drops failed transactions so the fee payer's balance change
is captured.

The allowed_wallets config normalised every address to lowercase, which
corrupts case-sensitive Solana base58 addresses. Normalisation is now
chain-aware: only 0x-prefixed (EVM) addresses are lowercased; all other
formats are preserved as-is. The runtime check_wallet_allowed function
uses the same logic.

Closes #145, closes #143

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Owner Author

@user1303836 user1303836 left a comment

Choose a reason for hiding this comment

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

Detailed Review of PR #161

I performed a thorough review of the diff and the surrounding code context. The two core fixes are well-reasoned and mostly correct, but there are issues that should be addressed before merge.


Fix 1: Solana parser fee entries on failed transactions (#145)

What's correct:

  • The parse_solana_transaction changes are logically sound: fee entries are emitted for fee payers even on failed txs, while transfer and SPL token extraction are correctly skipped behind the if !is_failed guard.
  • The extract_solana_native_balance_deltas removal of the early meta.err.is_some() return is correct. On Solana, the fee payer's balance genuinely changes (fee deducted) even when a transaction fails, and the pre/post balance arrays in meta reflect that. Non-fee-payer accounts will have change == 0 and get filtered out by the existing if change == 0 { continue; } guard, so there is no risk of emitting phantom deltas for other accounts.
  • extract_solana_token_transfers already had its own meta.err.is_some() early return (line 220-222), which is correct -- failed Solana txs don't change token state. This was not touched by the PR and doesn't need to be.
  • extract_solana_decoded_events already skips failed txs (line 388-391), also correct and untouched.
  • The test fixtures are realistic: the err field format {"InstructionError": [0, {"Custom": 1}]} matches real Solana RPC output. The balance arithmetic is consistent (pre=10000000, post=9995000, fee=5000).

What needs attention:

  1. Nit: let _ = entry_index; is dead code (line 97 of solana_parser.rs). This line suppresses an unused variable warning but serves no purpose. The entry_index variable was already used before the early return. Remove this line -- it clutters the code with no functional benefit. (The similar let _ = entry_index; on line 144 already existed and is acceptable since it marks the intentional end of index usage at the function's end.)

  2. Missing test: failed tx where tracked wallet is NOT the fee payer. The PR tests the fee-payer case but doesn't test the case where a user's wallet participates in a failed transaction as a non-fee-payer. In a failed Solana tx, a non-fee-payer's balance does not change (Solana rolls back all state changes except the fee deduction). The parser should produce zero entries for such a wallet. This is an important edge case for correctness -- please add a test that constructs a 2-account failed tx where the tracked wallet is at index 1 (not fee payer) and asserts entries.is_empty(). The current code handles this correctly (the is_fee_payer check gates the fee entry, and !is_failed gates the transfer logic), but a test proves it.

  3. Missing test: failed tx with multiple accounts in extract_solana_native_balance_deltas. The current test only has one account (the fee payer). Add a test with 2+ accounts where only the fee payer has a balance change, to verify that non-fee-payer accounts with change == 0 are correctly filtered out by the delta extractor.


Fix 2: Wallet filtering case sensitivity (#143)

What's correct:

  • The 0x/0X prefix heuristic for distinguishing EVM from Solana addresses is the right approach given that allowed_wallets is a flat comma-separated string with no chain metadata. Solana base58 addresses cannot start with 0x (the base58 alphabet does not include lowercase 'x'), so this heuristic is safe and won't produce false positives.
  • Hyperliquid addresses are also 0x-prefixed, so they are correctly lowercased.
  • The normalization is applied consistently: both allowed_wallets_set() (config time) and check_wallet_allowed() (request time) use the same 0x/0X heuristic.
  • The tests cover the key cases: EVM case-insensitive matching, Solana case-sensitive rejection, and exact Solana match.

What needs attention:

  1. (Important) adapters/src/ledger_derivation.rs has the same class of bug. Four functions in ledger_derivation.rs unconditionally lowercase wallet addresses for comparison:

    • derive_ledger_from_token_transfers (line 51): let wallet_lower = wallet.to_lowercase();
    • derive_ledger_from_native_balance_deltas (line 111): let wallet_lower = wallet.to_lowercase();
    • derive_wallet_ledger_from_token_transfers (line 443): let wallet_lower = wallet.to_lowercase();
    • derive_wallet_ledger_from_native_balance_deltas (line 523): let wallet_lower = wallet.to_lowercase();

    These compare wallet.to_lowercase() against transfer.from_address.to_lowercase() / delta.account_address.to_lowercase(). For Solana addresses, lowercasing both sides means two different Solana addresses that differ only in case would incorrectly match each other.

    In practice, the Solana data path typically preserves case consistently (since the RPC returns canonical base58), so this is less likely to cause silent data corruption today. However, it is the exact same class of bug as #143 and is in the data derivation hot path. Since this PR claims to fix the Solana case-sensitivity issue, these four call sites should be addressed in the same PR, or at minimum called out as a known remaining issue with a follow-up issue filed. The fix pattern: only lowercase when the address is 0x-prefixed (EVM/Hyperliquid).


Cross-cutting concerns

  • The PR does not deepen wallet-only assumptions. The parse_solana_transaction function is already wallet-scoped and this change stays within that scope. The extract_* functions are wallet-agnostic and work for the V2 target system.
  • The check_wallet_allowed fix works identically for both the legacy wallet endpoints and the V2 target system since all wallet-scoped routes go through the same function.
  • The V2 target system already has proper chain-aware normalization (normalize_evm_address / normalize_solana_address in core/src/v2.rs and adapters/src/compat.rs), so the target registration path is already correct.
  • No API surface or CLI changes, so no README update needed.

Verdict

The two core fixes are correct in their logic and address the reported issues. I would approve once:

  • (Must fix) The ledger_derivation.rs lowercase bug is either fixed in this PR or explicitly deferred with a filed issue.
  • (Should add) Two more edge-case tests: non-fee-payer on failed tx, multi-account delta extraction on failed tx.
  • (Nit) Remove dead let _ = entry_index; on line 97.

…-case tests

Address review feedback on PR #161:
- Fix case-sensitivity bug in 4 ledger_derivation.rs functions that
  unconditionally lowercased wallet addresses. Now uses 0x-prefix
  heuristic to only lowercase EVM addresses, preserving Solana base58.
- Add test for failed tx where tracked wallet is not the fee payer
  (should produce zero entries).
- Add test for failed tx with multiple accounts in native balance
  delta extraction (only fee payer should have non-zero delta).
- Remove dead `let _ = entry_index;` line in solana_parser.rs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 99af181fd836
@user1303836 user1303836 merged commit 7060c01 into main Mar 13, 2026
4 checks passed
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.

1 participant