fix(pnl): resolve infinite % pnl and high execution price warnings #483
Conversation
Fixes two critical PnL calculation issues: 1. Infinite PnL % caused by unstable sort order for trades with identical timestamps (enforced Buy before Sell). 2. High Execution Price warnings for cross-chain swaps (e.g. WETH -> BSC USDC) caused by using source chain decimals for quote token. Updated to track usdcChainId from state changes.
WalkthroughRefactors PnL/trade reconstruction to deduplicate and group transfers by transaction, adds chain-aware USDC decimals and multi-phase USDC fallbacks, tightens validation and sorting (BUY-before-SELL on ties), extends RelayRequest stateChange data with optional symbol/decimals, and adds an early-exit for dust balances in the token PnL hook. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
Deploying x with
|
| Latest commit: |
5cef011
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dd90925a.x-e62.pages.dev |
| Branch Preview URL: | https://fix-pnl-calculation-logic.x-e62.pages.dev |
Deploying pillarx-debug with
|
| Latest commit: |
5cef011
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://51774b38.pillarx-debug.pages.dev |
| Branch Preview URL: | https://fix-pnl-calculation-logic.pillarx-debug.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/pnl.ts (1)
618-627: Duplicate sort logic - covered by earlier refactor suggestion.This is the third instance of the same sorting logic. See earlier comment about extracting a helper function.
🧹 Nitpick comments (4)
src/utils/pnl.ts (4)
116-130: Consider adding proper type definition fortoken_pricefield.The fallback logic is sound, but the
anycast on line 126 indicates theMobulaTransactionRowtype may be incomplete. Iftoken_priceis a known field from the Mobula API, consider extending the type definition to avoid runtime property access issues.- const price = (referenceTx as any).token_price || (referenceTx as any).asset?.price || 0; + const price = referenceTx.token_price ?? referenceTx.asset?.price ?? 0;Then update the
MobulaTransactionRowtype to include:token_price?: number;
419-429: Consider extracting duplicate sort logic into a helper function.This sorting logic is duplicated in three places (lines 148-157, 419-429, 618-627). Extracting it would improve maintainability.
const sortTradesByTimestampAndSide = (trades: ReconstructedTrade[]): ReconstructedTrade[] => trades.sort((a, b) => { const timeDiff = a.timestamp - b.timestamp; if (timeDiff !== 0) return timeDiff; if (a.side === 'BUY' && b.side === 'SELL') return -1; if (a.side === 'SELL' && b.side === 'BUY') return 1; return 0; });
501-503: Minor: Redundant fallback.Since
usdcChainIdis initialized totoken.chainIdon line 493, the|| token.chainIdfallback is technically redundant. However, it's harmless defensive coding.
546-559: Complex conditions could benefit from extraction.The multi-condition checks for USDC identification (lines 547-549, 556-558) are hard to read. Consider extracting a helper function.
const isUSDCCurrency = (address: string | undefined, symbol: string | undefined): boolean => !!(address && (USDC_ADDRESSES.includes(address) || symbol?.toUpperCase() === 'USDC'));Then simplify to:
- if (amountUSDC === 0 && (inAddress && - (USDC_ADDRESSES.includes(inAddress) || currencyIn.currency?.symbol?.toUpperCase() === 'USDC'))) { + if (amountUSDC === 0 && isUSDCCurrency(inAddress, currencyIn.currency?.symbol)) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/pnl.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/utils/pnl.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (4)
src/utils/pnl.ts (4)
103-104: LGTM - Simplified direction detection.Removing the USDC movement requirement makes sense given the new price-based fallback for estimating USDC amounts. The directional logic (positive tokenChange = BUY, negative = SELL) is correct.
148-157: Good fix for deterministic trade ordering.This correctly addresses the infinite PnL % issue by ensuring BUY trades are processed before SELL trades when timestamps are identical. This guarantees inventory exists before calculating weighted average cost during sells.
466-494: Good fix for cross-chain USDC decimal handling.Tracking
usdcChainIdfrom the actual USDC state change correctly addresses the issue where cross-chain swaps (e.g., WETH → BSC USDC) used incorrect decimals from the source chain.
280-287: Thehashfield is properly defined on theMobulaTransactionRowtype (src/types/api.ts, line 813). Bothhashandtx_hashare valid fields. The defensive handling oftx.hash || tx.tx_hashis correct and requires no changes.Likely an incorrect or invalid review comment.
github-actions
bot
temporarily deployed
to
Preview (fix/pnl-calculation-logic)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/pnl.ts (1)
524-545: Handle potential NaN from parseFloat.
parseFloat(metadata.currencyIn.amountUsd)andparseFloat(metadata.currencyOut.amountUsd)can returnNaNif the value is malformed, which would propagate through calculations.Apply defensive fallback:
- amountUSDC = parseFloat(metadata.currencyIn.amountUsd); + amountUSDC = parseFloat(metadata.currencyIn.amountUsd) || 0;- amountUSDC = parseFloat(metadata.currencyOut.amountUsd); + amountUSDC = parseFloat(metadata.currencyOut.amountUsd) || 0;
🧹 Nitpick comments (2)
src/utils/pnl.ts (2)
116-130: Type-unsafe access totoken_priceandasset.price.The
as anycast bypasses type checking. TheMobulaTransactionRowtype (per relevant snippets) already definestoken_priceandasset.price, so explicit typing would be safer.- const price = (referenceTx as any).token_price || (referenceTx as any).asset?.price || 0; + const price = referenceTx.token_price ?? referenceTx.asset?.price ?? 0;
559-572: Potential NaN propagation in metadata fallback branch.Same parseFloat concern applies here. Lines 559 and 568 use
parseFloat(...amountUsd || '0')which handles undefined, but ifamountUsdis a non-numeric string like"N/A", parseFloat returns NaN.- amountUSDC = parseFloat(currencyIn.amountUsd || '0'); + const parsedIn = parseFloat(currencyIn.amountUsd || '0'); + amountUSDC = Number.isFinite(parsedIn) ? parsedIn : 0;Apply similar pattern for line 568.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/__tests__/pnl_bnb.test.ts(1 hunks)src/utils/__tests__/pnl_reconstruct.test.ts(1 hunks)src/utils/pnl.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/utils/pnl.ts
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/utils/pnl.ts
🧬 Code graph analysis (2)
src/utils/__tests__/pnl_reconstruct.test.ts (2)
src/types/api.ts (1)
MobulaTransactionRow(812-834)src/utils/pnl.ts (1)
reconstructTrades(32-158)
src/utils/__tests__/pnl_bnb.test.ts (2)
src/services/relayApi.ts (1)
RelayRequest(3-99)src/utils/pnl.ts (1)
calculatePnLFromRelay(436-641)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (8)
src/utils/pnl.ts (5)
103-104: LGTM! Simplified direction detection.Using sign-based detection (
tokenChange > 0for BUY,< 0for SELL) is cleaner and correctly handles the directional logic.
148-157: Good fix for sort stability.Enforcing BUY-before-SELL when timestamps match eliminates the non-deterministic ordering that caused infinite PnL percentages. This is applied consistently across all three sorting locations.
280-287: Handle hash normalization correctly.Good defensive handling for API inconsistency returning
hashvstx_hash. Skipping entries with no hash prevents undefined key issues.
466-507: TrackusdcChainIdfrom transaction data.Capturing the chainId where USDC actually moved enables correct decimal resolution for cross-chain swaps. The default to
token.chainIdprovides a sensible fallback.
631-640: Consistent BUY-before-SELL ordering.This matches the sorting logic in
reconstructTradesandgetRelayValidatedTrades, ensuring consistent behavior across all trade reconstruction paths.src/utils/__tests__/pnl_reconstruct.test.ts (1)
5-38: Good test coverage for fallback logic.This test correctly validates the token_price fallback when USDC leg is missing. The assertion that
amountQuoteUSDCequals300(1 token × $300 price) confirms the fallback path works.Consider adding edge cases:
- BUY scenario (token inbound)
token_priceis 0 or undefined (should skip trade)- Multiple transactions in same group
src/utils/__tests__/pnl_bnb.test.ts (2)
51-73: Good regression test for metadata fallback.This test validates the critical fix: when state changes miss USDC data, the code falls back to metadata's
currencyIn.amountUsd($300) rather than using the current price ($10 × 100 = $1000). The comment on lines 70-72 clearly documents the expected vs buggy behavior.
17-38: Mock stateChanges don't include USDC movement.The test correctly simulates the scenario where USDC is not in stateChanges (only WBNB and STA token movements). This forces the code through the metadata fallback path on line 527-532 of
pnl.ts.
github-actions
bot
temporarily deployed
to
Preview (fix/pnl-calculation-logic)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/pnl.ts (1)
573-594: Handle potential NaN from parseFloat.
parseFloat(metadata.currencyIn.amountUsd)andparseFloat(metadata.currencyOut.amountUsd)could returnNaNif the value is undefined or malformed. This NaN would propagate and cause issues in calculations.🔎 Apply this diff to guard against NaN
} else if (metadata?.currencyIn?.amountUsd) { - amountUSDC = parseFloat(metadata.currencyIn.amountUsd); + amountUSDC = parseFloat(metadata.currencyIn.amountUsd) || 0; }Apply the same pattern at line 592:
} else if (metadata?.currencyOut?.amountUsd) { - amountUSDC = parseFloat(metadata.currencyOut.amountUsd); + amountUSDC = parseFloat(metadata.currencyOut.amountUsd) || 0; }
🧹 Nitpick comments (1)
src/utils/pnl.ts (1)
128-152: Consider adding type safety instead ofas anycasts.The fallback logic is sound, but using
as anybypasses type checking. Consider extendingMobulaTransactionRowto includetoken_priceif the API reliably returns it, or use a type guard.🔎 Example type-safe approach
- const price = - (referenceTx as any).token_price || - (referenceTx as any).asset?.price || - 0; + // If MobulaTransactionRow can have token_price, add it to the type + const price = + ('token_price' in referenceTx ? (referenceTx as { token_price?: number }).token_price : undefined) || + referenceTx.asset?.price || + 0;Alternatively, extend the
MobulaTransactionRowtype to includetoken_price?: number.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/the-exchange/components/CardsSwap/test/__snapshots__/CardSwap.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/services/relayApi.ts(2 hunks)src/utils/pnl.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/utils/pnl.ts
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/utils/pnl.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (10)
src/utils/pnl.ts (9)
39-51: LGTM! Deduplication prevents double-counting trades.The deduplication logic correctly identifies duplicate transactions by matching hash, from, to, amount, and asset symbol. The O(n²) complexity from
filter+findIndexis acceptable for typical transaction list sizes.
54-61: Good defensive handling of hash field variations.The fallback from
tx.tx_hashtotx.hashhandles API response inconsistencies, and skipping entries without a hash prevents downstream errors.
118-119: Simplified direction detection focuses on token movement only.Using only
tokenChangeto determine BUY/SELL is cleaner and aligns with the PR objective of fixing direction detection.
167-176: Correct fix for unstable sort order causing infinite PnL %.Enforcing BUY before SELL for identical timestamps ensures inventory exists before sell operations, preventing division by zero in PnL calculations. This directly addresses the first issue in the PR objectives.
493-539: Solid implementation of cross-chain USDC decimal handling.Tracking
usdcChainIdfrom state changes correctly addresses the high execution price warnings for cross-chain swaps. The symbol-based USDC detection (line 522) provides a robust fallback when address matching fails.
541-556: Good layered fallback strategy for determining USDC chain ID.The fallback order (stateChanges → metadata currencyIn/Out → token.chainId) handles various API response scenarios. This ensures correct decimal selection for cross-chain swaps.
608-628: Enhanced USDC detection with symbol fallback is a good improvement.Adding
currencyIn.currency?.symbol?.toUpperCase() === 'USDC'as an alternative to address matching handles cases where USDC addresses aren't in the known list.Note: Lines 608 and 620 also use
parseFloat— apply the same|| 0guard for consistency.
646-671: Good sanity checks prevent absurd values from corrupting PnL.The validation thresholds ($1 trillion USDC, $1 million per token) with logging help identify data issues without silently producing incorrect results.
687-696: Consistent sorting logic applied across all trade reconstruction functions.The same BUY-before-SELL tie-breaker is correctly applied here as in the other functions, ensuring consistent behavior.
src/services/relayApi.ts (1)
54-58: LGTM! Type additions support enhanced PnL calculations.Adding optional
symbolanddecimalsfields to the state change data structure aligns with the PnL utility's new symbol-based USDC detection logic. These fields enable more robust token identification in cross-chain scenarios.Also applies to: 70-74
github-actions
bot
temporarily deployed
to
Preview (fix/pnl-calculation-logic)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/pnl.ts (1)
573-593: Add NaN and Infinity guards to parseFloat results.parseFloat returns NaN if the argument's first character can't start a legal number literal, and NaN, Infinity, and -Infinity should be guarded against. Apply guards at lines 580 and 591:
- amountUSDC = parseFloat(metadata.currencyIn.amountUsd); + const parsed = parseFloat(metadata.currencyIn.amountUsd); + amountUSDC = isFinite(parsed) ? parsed : 0;Apply the same pattern for line 591 with
metadata.currencyOut.amountUsd.
🧹 Nitpick comments (2)
src/utils/pnl.ts (2)
54-61: Ensure consistent hash field priority across functions.The hash extraction order is inconsistent: Line 55 uses
tx.tx_hash || tx.hash, while Line 299 usestx.hash || tx.tx_hash. While functionally equivalent when only one field exists, consistent ordering improves maintainability.🔎 Standardize to one order across all functions:
- const hash = tx.tx_hash || tx.hash; + const hash = tx.hash || tx.tx_hash;
114-119: Update comment to reflect simplified direction logic.The comment mentions USDC movement for determining BUY/SELL direction, but the code now only checks
tokenChange. The comment should reflect the actual implementation.🔎 Update the comment:
- // Determine direction - // BUY: Token IN (+), USDC OUT (-) - // SELL: Token OUT (-), USDC IN (+) + // Determine direction based on token movement + // BUY: Token IN (+) + // SELL: Token OUT (-)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/pnl.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/utils/pnl.ts
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/utils/pnl.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (5)
src/utils/pnl.ts (5)
166-175: LGTM! Deterministic sort order fixes infinite PnL issue.The BUY-before-SELL ordering for identical timestamps ensures stable, deterministic sorting that prevents the infinite PnL % issue mentioned in the PR objectives. The same pattern is consistently applied at lines 437-447 and 686-695.
468-474: LGTM! Effective deduplication prevents double-counting.The relay request deduplication by ID is correctly implemented and prevents potential double-counting issues in PnL calculations.
645-670: LGTM! Sanity checks prevent absurd PnL display.The validation checks for unrealistic USDC amounts and execution prices are well-implemented and will prevent displaying incorrect PnL metrics caused by data quality issues or calculation errors.
492-555: Excellent! Chain-aware decimals fix high execution price warnings.The
usdcChainIdtracking logic addresses the PR's second objective by ensuring the correct USDC decimals are used for cross-chain swaps (e.g., WETH → BSC USDC). The fallback chain from state changes → metadata → token.chainId is robust.The symbol field check at line 521 is part of the RelayRequest schema definition and is safely handled with optional chaining, providing a solid defensive fallback alongside the tokenAddress verification.
39-51: Remove this review comment—the deduplication logic is correct.The deduplication includes
fromandtofields intentionally and correctly. Test data shows a single transaction can contain multiple distinct transfers with the same hash but different from/to addresses (e.g., user→pool, pool→user, user→miner for gas). These must be tracked separately for accurate PnL. The current deduplication byhash + from + to + amount + symbolensures each distinct transfer is preserved while preventing duplicate records of the exact same transfer.
github-actions
bot
temporarily deployed
to
Preview (fix/pnl-calculation-logic)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/hooks/useTokenPnL.ts (1)
100-111: Good optimization to skip PnL computation for dust balances.The early-exit logic correctly prevents unnecessary Relay API calls and computation for balances under $0.50. The safe handling of undefined values (
|| 0) ensures the check works even when price or balance data is missing.This complements the main fixes for high execution price warnings (chain-aware decimals in pnl.ts) by adding a client-side guard specifically for dust amounts.
Optional: Extract magic number as a named constant
For maintainability, consider extracting the threshold:
+const MIN_BALANCE_USD_FOR_PNL = 0.5; + export const useTokenPnL = (props: UseTokenPnLProps | null): TokenPnLResult => { // ... // NEW: Skip PnL calculation for small balances (< $0.50) // This avoids "Suspiciously high execution price" warnings for dust const balanceUSD = (tokenPrice || 0) * (tokenBalance || 0); - if (balanceUSD < 0.5) { + if (balanceUSD < MIN_BALANCE_USD_FOR_PNL) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useTokenPnL.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/hooks/useTokenPnL.ts
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/hooks/useTokenPnL.ts
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/hooks/useTokenPnL.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
github-actions
bot
temporarily deployed
to
Preview (fix/pnl-calculation-logic)
github-actions
bot
temporarily deployed
to
Preview (fix/pnl-calculation-logic)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/pnl.ts (1)
569-590: Add NaN guard to parseFloat calls.Lines 577 and 588 use
parseFloat()directly on metadata strings. IfamountUsdis undefined, empty, or malformed, this returnsNaNwhich will propagate through calculations.Suggested fix
- amountUSDC = parseFloat(metadata.currencyIn.amountUsd); + amountUSDC = parseFloat(metadata.currencyIn.amountUsd) || 0;- amountUSDC = parseFloat(metadata.currencyOut.amountUsd); + amountUSDC = parseFloat(metadata.currencyOut.amountUsd) || 0;
🧹 Nitpick comments (2)
src/utils/pnl.ts (2)
295-302: Minor inconsistency in hash field priority.Line 55 uses
tx.tx_hash || tx.hash, but line 296 usestx.hash || tx.tx_hash. While both work (either field being present suffices), consider using a consistent order across the codebase for clarity.Suggested fix for consistency
- const hash = tx.hash || tx.tx_hash; + const hash = tx.tx_hash || tx.hash;
683-692: LGTM!Consistent BUY-before-SELL sorting applied.
Consider extracting this sorting comparator to a shared helper since it's duplicated in three places (
reconstructTrades,getRelayValidatedTrades,calculatePnLFromRelay), but this is optional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx(1 hunks)src/hooks/useTokenPnL.ts(1 hunks)src/utils/pnl.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useTokenPnL.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/utils/pnl.ts
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/utils/pnl.ts
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/utils/pnl.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (9)
src/utils/pnl.ts (9)
39-51: LGTM on deduplication logic.The composite key (hash + from + to + amount + symbol) correctly identifies duplicate transactions. The
findIndexapproach has O(n²) complexity, but this is acceptable for typical transaction list sizes.
54-61: LGTM!Hash normalization with fallback and early return for missing hashes handles API inconsistencies cleanly.
117-121: LGTM!Basing trade direction solely on token movement (positive = BUY, negative = SELL) is the correct approach. This prevents incorrect direction inference from USDC flow in complex swaps.
128-148: LGTM!The fallback logic correctly handles cases where USDC movement isn't detected (e.g., wraps, missing internal tx data) by estimating USD value from token price. Skipping trades that still have zero USDC after fallback prevents invalid PnL calculations.
163-172: Core fix for infinite PnL issue.Enforcing BUY before SELL for identical timestamps ensures inventory exists before sell operations, preventing the WAC division-by-zero that caused infinite PnL percentages. Good fix.
434-444: LGTM!Consistent BUY-before-SELL sorting applied here as well.
465-535: Core fix for cross-chain USDC decimal issue.Tracking
usdcChainIdfrom stateChanges ensures correct decimal resolution for cross-chain swaps (e.g., WETH → BSC USDC now uses BSC's USDC decimals instead of the source chain's). The Set-based deduplication is efficient.
537-552: LGTM!The fallback chain (stateChanges → metadata → token.chainId) is well-designed. The metadata-based inference correctly uses
req.in.chainIdwhen USDC is the input currency andreq.out.chainIdwhen it's the output.
604-624: LGTM!The
|| '0'fallback and symbol-based USDC detection (currency?.symbol?.toUpperCase() === 'USDC') add robustness to the metadata parsing path.
src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx
Outdated
Show resolved
Hide resolved
src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx
Outdated
Show resolved
Hide resolved
src/utils/pnl.ts
Outdated
| if (absTokenChange === 0) return; // Dust or zero value | ||
|
|
||
| // FALLBACK: If we detected a valid token movement (BUY/SELL) but NO USDC movement | ||
| // (e.g. native BNB wrap/unwrap or missing internal tx data), try to use the token price |
There was a problem hiding this comment.
Too much AI text in the next few line of code, please reduce to what is really needed here
There was a problem hiding this comment.
Not sure what happened in the latest changes but it seems that there are even more explanation texts.... not a show stopper
src/utils/pnl.ts
Outdated
| } else if (metadata?.currencyIn?.amountUsd) { | ||
| // Fallback: If we detected token BUY via state changes but no USDC state change, | ||
| // check metadata for the inbound currency's USD value (which is what we spent). | ||
| // Actually for BUY: We receive Token (Out), we spend CurrencyIn. |
There was a problem hiding this comment.
Please clean those AI explanations, as you can see here it is talking to itself
github-actions
bot
temporarily deployed
to
Preview (fix/pnl-calculation-logic)
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 (2)
src/utils/pnl.ts (2)
40-178: Reduce excessive inline comments per previous feedback.The inline comments throughout this function are overly verbose and educational. Previous reviewers flagged "too much AI text" in comments. Many explanations are redundant with the code or use an instructional tone ("We need to...", "This represents...").
Simplify comments to be more concise and professional. For example:
- Lines 40-42: "Deduplicate by hash, from, to, amount, and symbol"
- Lines 56-59: "Group transfers by transaction hash"
- Lines 92-94: "Skip native token transfers (fees only)"
- Lines 137-146: "Estimate USDC using token price if not found in transfers"
Based on learnings from past reviews.
398-439: Incorrect USDC decimal normalization for cross-chain swaps ingetRelayValidatedTrades.USDC has 6 decimals on Ethereum but 18 decimals on Binance Smart Chain. Line 428 uses
token.chainIdto determine USDC decimals, which fails for cross-chain swaps where the asset originates on one chain but USDC settlement occurs on another. For example, in a "WETH → BSC USDC" swap:
token.chainId= 1 (Ethereum, where WETH originates)- USDC actually moved on chainId = 56 (BSC with 18 decimals)
- Using Ethereum's 6 decimals instead causes a 10^12 multiplier error in the execution price
The
inTxobject contains achainIdfield that identifies where each transaction executed. ThecalculatePnLFromRelayfunction (lines 528-602) correctly tracksusdcChainIdfrom state changes and usesgetUSDCDecimalsByChainId(finalUsdcChainId || token.chainId). Implement the same chain-aware logic here by extractingchainIdfrominTxwhen USDC is detected in state changes, similar to the pattern already proven incalculatePnLFromRelay.
🧹 Nitpick comments (1)
src/utils/pnl.ts (1)
40-54: Consider optimizing deduplication for better performance.The current O(n²) approach using
filter+findIndexworks correctly but could be slow with large transaction sets. Consider using a Set-based approach for O(n) performance:🔎 Proposed optimization
- const uniqueTransactions = transactions.filter( - (tx, index, self) => - index === - self.findIndex( - (t) => - (t.tx_hash || t.hash) === (tx.tx_hash || tx.hash) && - t.from === tx.from && - t.to === tx.to && - t.amount === tx.amount && - t.asset.symbol === tx.asset.symbol - ) - ); + const seen = new Set<string>(); + const uniqueTransactions = transactions.filter((tx) => { + const key = `${tx.tx_hash || tx.hash}-${tx.from}-${tx.to}-${tx.amount}-${tx.asset.symbol}`; + if (seen.has(key)) return false; + seen.add(key); + return true; + });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/pnl.ts(16 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/utils/pnl.ts
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/utils/pnl.ts
🧬 Code graph analysis (1)
src/utils/pnl.ts (3)
src/apps/pulse/constants/tokens.ts (1)
allStableCurrencies(3-39)src/types/api.ts (1)
MobulaTransactionRow(812-834)src/services/relayApiAsync.ts (1)
fetchRelayRequestByHash(32-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (6)
src/utils/pnl.ts (6)
25-31: Good fix for cross-chain USDC decimal handling.This function correctly addresses the PR objective by selecting chain-specific USDC decimals. The fallback to 6 decimals is reasonable since most USDC deployments use 6 decimals (BSC's 18-decimal USDC is the exception).
137-147: LGTM: Fallback USDC estimation is well-designed.The fallback mechanism correctly handles edge cases where USDC transfers aren't directly visible (e.g., bridge operations, atomic swaps). Using the first transaction's
token_priceis appropriate since all transfers in a single transaction should have the same price.
166-177: Excellent fix for infinite PnL % issue.This sorting logic directly addresses the first PR objective by enforcing stable sort order (BUY before SELL) for trades with identical timestamps. This prevents the PnL calculation from attempting to sell inventory that hasn't been acquired yet, which was causing infinite percentages.
The same pattern is correctly applied across all trade reconstruction functions in this file.
180-272: PnL calculation logic is sound.The weighted average cost (WAC) method is correctly implemented:
- Proper inventory tracking prevents sells without holdings (Line 196)
- Floating-point rounding errors are handled by clamping negative values to zero (Lines 212-213), which aligns with the team's accepted trade-offs per learnings
- Returning null for wallets without buy history prevents misleading zero metrics (Lines 255-257)
Note: The excessive comment verbosity issue flagged earlier also applies to Lines 191-271 in this function.
528-602: Excellent implementation of chain-aware USDC decimal handling.This reducer-based approach correctly tracks
usdcChainIdfrom state changes (Line 568) and implements robust fallback logic (Lines 588-598) to resolve the correct chain for USDC decimal normalization. This directly addresses the second PR objective regarding "High Execution Price warnings for cross-chain swaps."The multi-level fallback strategy is well-designed:
- Primary: Use chain from state changes where USDC actually moved
- Fallback 1: Check metadata's currencyIn/currencyOut to determine USDC chain
- Fallback 2: Use token's chain as ultimate fallback
710-735: Sanity check thresholds are reasonable.The validation bounds prevent corrupted data from affecting PnL:
- $1 trillion USDC cap catches decimal normalization errors
- $1 million per-token price cap prevents miscalculated execution prices
- Positive price requirement ensures valid trade data
These thresholds are appropriate for cryptocurrency trading and provide good safeguards without being overly restrictive.
|
@vignesha22 some checks are failing |
@RanaBug yeah its fixed now some extra spaces |
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)
src/utils/pnl.ts (1)
40-118: Reduce comment verbosity per past review feedback.Past reviews specifically requested reducing AI-generated explanatory text. Many comments here are overly verbose and explain self-evident code:
- Lines 40-42, 56-59, 79-80, 85-87, 92-94, 103-105, 107-109, 111, 114
Consider condensing to high-level section headers only:
// Deduplicate identical transactions const uniqueTransactions = transactions.filter(...) // Group transfers by transaction hash uniqueTransactions.forEach((tx) => { const hash = tx.tx_hash || tx.hash; ... }) // Reconstruct trades from grouped transfers Object.keys(groupedByTxHash).forEach((txHash) => { ... })Based on past review feedback (RanaBug: "Please clean those AI explanations").
♻️ Duplicate comments (1)
src/utils/pnl.ts (1)
622-640: Add defensive checks for parseFloat on metadata values at lines 627 and 639.When parseFloat encounters a character that cannot start a valid number, it returns NaN, which then propagates through PnL calculations. Lines 627 and 639 only guard against undefined via optional chaining, not malformed strings:
} else if (metadata?.currencyIn?.amountUsd) { amountUSDC = parseFloat(metadata.currencyIn.amountUsd); // Could be NaN if malformed }Add a fallback:
parseFloat(metadata.currencyIn.amountUsd || '0'). This matches the safer pattern already used elsewhere in the file (lines 658–683) and aligns with the codebase's consistent use ofNumber.isNaN()for validation.
🧹 Nitpick comments (1)
src/utils/pnl.ts (1)
166-177: Consider extracting duplicate sort logic to a helper function.The BUY-before-SELL sorting logic is duplicated across three functions (
reconstructTrades,getRelayValidatedTrades,calculatePnLFromRelay). Consider extracting to a reusable helper:🔎 Suggested refactor
const sortTradesWithInventoryLogic = ( trades: ReconstructedTrade[] ): ReconstructedTrade[] => { return trades.sort((a, b) => { const timeDiff = a.timestamp - b.timestamp; if (timeDiff !== 0) return timeDiff; if (a.side === 'BUY' && b.side === 'SELL') return -1; if (a.side === 'SELL' && b.side === 'BUY') return 1; return 0; }); };Then replace all three sort blocks with:
return sortTradesWithInventoryLogic(trades);Also applies to: 470-480, 752-762
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/pnl.ts(16 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/utils/pnl.ts
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/utils/pnl.ts
🧬 Code graph analysis (1)
src/utils/pnl.ts (3)
src/apps/pulse/constants/tokens.ts (1)
allStableCurrencies(3-39)src/types/api.ts (1)
MobulaTransactionRow(812-834)src/services/relayApiAsync.ts (1)
fetchRelayRequestByHash(32-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (5)
src/utils/pnl.ts (5)
15-31: LGTM: Efficient USDC lookup with proper defaults.The pre-computed USDC addresses enable O(1) lookups, and the default of 6 decimals is appropriate since most USDC deployments (Ethereum, Optimism, Polygon, Base, Arbitrum) use 6 decimals. The BSC variant with 18 decimals is properly handled via the lookup in
allStableCurrencies.
40-54: LGTM: Deduplication logic is sound.The deduplication correctly identifies duplicates by matching transaction hash, sender, receiver, amount, and token symbol. The use of
findIndexensures only the first occurrence is kept.
166-177: LGTM: Sort logic correctly fixes infinite PnL issue.The BUY-before-SELL ordering for identical timestamps directly addresses the PR objective of fixing infinite PnL %. This ensures inventory is established before sell operations, preventing division-by-zero or undefined PnL calculations.
528-602: LGTM: Robust usdcChainId tracking solves execution price issue.The multi-phase fallback for determining
finalUsdcChainIddirectly addresses the PR objective of fixing high execution price warnings for cross-chain swaps:
- Primary (most accurate): Extract chain ID from state changes where USDC actually moved (line 568)
- Fallback 1: Determine from metadata by checking if currencyIn/Out is USDC (lines 589-597)
- Fallback 2: Assume token's chain (line 602)
This ensures the correct USDC decimals are used for normalization (line 611), preventing incorrect execution prices when USDC is on a different chain than the token (e.g., WETH on Ethereum → USDC on BSC).
706-735: LGTM: Sanity checks provide good data validation.The three-tier validation prevents corrupted data from affecting PnL calculations:
- Rejects USDC amounts > $1T (catches decimal normalization errors)
- Rejects execution prices > $1M/token (catches wrong decimals)
- Rejects non-positive prices (catches data extraction failures)
These bounds are appropriate for typical DeFi tokens and include clear warning logs for debugging.
Fixes two critical PnL calculation issues: 1. Infinite PnL % caused by unstable sort order for trades with identical timestamps (enforced Buy before Sell). 2. High Execution Price warnings for cross-chain swaps (e.g. WETH -> BSC USDC) caused by using source chain decimals for quote token. Updated to track usdcChainId from state changes.
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
Bug Fixes
Performance
New Features
✏️ Tip: You can customize this high-level summary in your review settings.