Conversation
WalkthroughThis PR introduces decimal handling improvements across two areas: a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 pillarx-debug with
|
| Latest commit: |
f59b4d7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3a8b2aa7.pillarx-debug.pages.dev |
| Branch Preview URL: | https://fix-bsc-usdc-decimals.pillarx-debug.pages.dev |
Deploying x with
|
| Latest commit: |
f59b4d7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b1e16f52.x-e62.pages.dev |
| Branch Preview URL: | https://fix-bsc-usdc-decimals.x-e62.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: 0
🧹 Nitpick comments (3)
src/utils/pnl.ts (1)
21-30: Chain‑aware USDC decimals look correct; consider minor cleanupsThe new
getUSDCDecimalsByChainIdhelper and its use in both PnL paths correctly derive a USDC divisor fromallStableCurrencies, fixing the BNB (18‑decimals) case while defaulting to 6 where unknown. This aligns well with the goal of removing hard‑coded1e6assumptions.Two small follow‑ups you might consider (non‑blocking):
- You can lean on the existing array type instead of manual structs to avoid duplicate typing, e.g.
const usdcToken = allStableCurrencies.find((c) => c.chainId === chainId);and then useusdcToken?.decimals ?? 6.- In
getRelayValidatedTrades,usdcDivisoris recomputed inside the innermost loop even thoughtoken.chainIdis constant per call; you could hoistconst usdcDivisor = 10 ** getUSDCDecimalsByChainId(token.chainId);once per trade for slightly cleaner code.Also applies to: 346-352, 451-456
src/apps/pulse/components/Buy/PayingToken.tsx (1)
7-7: Truncation applies to all paying tokens, not just USDCUsing
truncateDecimals(payingToken.totalRaw, 6)achieves the “max 6 decimals” behavior, but it now affects every paying token rendered by this component. The PR description calls out “USDC amount” specifically, so if this component can show non‑USDC pay tokens, you may want to gate this logic on symbol to avoid unintentionally changing formatting for other assets. Based on learnings, the float usage itself is consistent with existing patterns.Example tweak if you want USDC‑only truncation:
- <div className="text-[13px] font-normal text-white"> - {truncateDecimals(payingToken.totalRaw, 6)} - </div> + <div className="text-[13px] font-normal text-white"> + {payingToken.symbol === 'USDC' + ? truncateDecimals(payingToken.totalRaw, 6) + : payingToken.totalRaw} + </div>Also applies to: 89-89
src/apps/pulse/utils/number.ts (1)
55-68: truncateDecimals implementation matches truncation requirement; minor edge cases optionalThe implementation does what you need for the current usage: it takes a number/string, truncates the fractional part to
decimalsdigits without rounding, and leaves integers unchanged. This is consistent with the Pulse app’s accepted float‑based handling.If you ever plan to reuse this more broadly, a couple of minor hardening tweaks (optional) could be:
- Normalize
decimals <= 0to return just the integer part (currentlydecimals = 0yields"123."for"123.456").- Optionally guard against non‑finite numbers (NaN/Infinity) if those can leak in.
For the current
decimals = 6usage, it’s fine as‑is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apps/pulse/components/Buy/PayingToken.tsx(2 hunks)src/apps/pulse/utils/number.ts(1 hunks)src/utils/pnl.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/apps/pulse/utils/number.tssrc/utils/pnl.tssrc/apps/pulse/components/Buy/PayingToken.tsx
📚 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.tssrc/apps/pulse/components/Buy/PayingToken.tsx
🧬 Code graph analysis (2)
src/utils/pnl.ts (1)
src/apps/pulse/constants/tokens.ts (1)
allStableCurrencies(3-39)
src/apps/pulse/components/Buy/PayingToken.tsx (1)
src/apps/pulse/utils/number.ts (1)
truncateDecimals(55-68)
⏰ 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
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.