fix: failed Solana tx fee entries and base58 wallet filter#161
fix: failed Solana tx fee entries and base58 wallet filter#161user1303836 merged 2 commits intomainfrom
Conversation
…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>
user1303836
left a comment
There was a problem hiding this comment.
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_transactionchanges are logically sound: fee entries are emitted for fee payers even on failed txs, while transfer and SPL token extraction are correctly skipped behind theif !is_failedguard. - The
extract_solana_native_balance_deltasremoval of the earlymeta.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 inmetareflect that. Non-fee-payer accounts will havechange == 0and get filtered out by the existingif change == 0 { continue; }guard, so there is no risk of emitting phantom deltas for other accounts. extract_solana_token_transfersalready had its ownmeta.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_eventsalready skips failed txs (line 388-391), also correct and untouched.- The test fixtures are realistic: the
errfield format{"InstructionError": [0, {"Custom": 1}]}matches real Solana RPC output. The balance arithmetic is consistent (pre=10000000, post=9995000, fee=5000).
What needs attention:
-
Nit:
let _ = entry_index;is dead code (line 97 ofsolana_parser.rs). This line suppresses an unused variable warning but serves no purpose. Theentry_indexvariable was already used before the early return. Remove this line -- it clutters the code with no functional benefit. (The similarlet _ = entry_index;on line 144 already existed and is acceptable since it marks the intentional end of index usage at the function's end.) -
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 (theis_fee_payercheck gates the fee entry, and!is_failedgates the transfer logic), but a test proves it. -
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 withchange == 0are correctly filtered out by the delta extractor.
Fix 2: Wallet filtering case sensitivity (#143)
What's correct:
- The
0x/0Xprefix heuristic for distinguishing EVM from Solana addresses is the right approach given thatallowed_walletsis a flat comma-separated string with no chain metadata. Solana base58 addresses cannot start with0x(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) andcheck_wallet_allowed()(request time) use the same0x/0Xheuristic. - The tests cover the key cases: EVM case-insensitive matching, Solana case-sensitive rejection, and exact Solana match.
What needs attention:
-
(Important)
adapters/src/ledger_derivation.rshas the same class of bug. Four functions inledger_derivation.rsunconditionally 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()againsttransfer.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_transactionfunction is already wallet-scoped and this change stays within that scope. Theextract_*functions are wallet-agnostic and work for the V2 target system. - The
check_wallet_allowedfix 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_addressincore/src/v2.rsandadapters/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.rslowercase 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
Summary
extract_solana_native_balance_deltas) also no longer drops failed transactions, so the fee payer's balance change is captured.allowed_walletslowercases 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: only0x-prefixed (EVM) addresses are lowercased; all other formats (Solana base58, Hyperliquid, etc.) are preserved as-is. The runtimecheck_wallet_allowedfunction uses the same logic.Test plan
cargo fmt --all --checkpassescargo clippy --workspace --all-targets -- -D warningspassescargo test --workspace— all tests pass (one pre-existing flaky testtest_semaphore_limits_concurrent_jobsfails onmainas well, unrelated to these changes)test_parse_failed_tx_emits_fee_entry,test_extract_native_balance_deltas_failed_tx,test_extract_token_transfers_failed_tx_skiptest_failed_transaction_emits_fee(wastest_failed_transaction_skipped)test_allowed_wallets_set_lowercases_evm_onlytest_check_wallet_allowed_evm_case_insensitive,test_check_wallet_allowed_solana_case_sensitive🤖 Generated with Claude Code