PRO-3800-Gas Warning Check On Sell#431
Conversation
WalkthroughReplaces minAmount checks with a minGasAmount computed from per-wallet portfolio token data passed as a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Home as HomeScreen
participant Wallet as WalletPortfolio API
participant Conv as convertPortfolioAPIResponseToToken
participant Buy as Buy.tsx
participant Sell as Sell.tsx
participant Chain as Chain Helpers
participant UI as UI / SellButton
User->>Home: Open app
Home->>Wallet: query walletPortfolioData
Wallet-->>Home: walletPortfolioData
Home->>Conv: convertPortfolioAPIResponseToToken(walletPortfolioData)
Conv-->>Home: portfolioTokens
Home->>Buy: pass portfolioTokens
Home->>Sell: pass portfolioTokens
User->>Sell: select token / chain
Sell->>Chain: getChainId(), isNativeToken()
Sell->>Sell: compute minGasAmount from portfolioTokens (native price * balance < $1 ?)
alt minGasAmount true
Sell->>UI: show "Min. $1 native token" warning
Sell->>UI: set SellButton.notEnoughLiquidity = true (includes minGasAmount)
else
Sell->>UI: normal validation flow
end
User->>Buy: select token
Buy->>Chain: getChainId(), isNativeToken()
Buy->>Buy: compute/use portfolioTokens for gas checks (similar)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: |
81602bd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f9d7a061.x-e62.pages.dev |
| Branch Preview URL: | https://pro-3800-gas-warning-on-sell.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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/components/Sell/Sell.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: RanaBug
PR: pillarwallet/x#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.
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
PR: pillarwallet/x#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/apps/pulse/components/Sell/Sell.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/Sell/Sell.tsx (2)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/apps/pulse/utils/blockchain.ts (3)
isNativeToken(17-18)NativeSymbols(20-27)ChainNames(29-36)
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: unit-tests
| {relayError || | ||
| (notEnoughLiquidity ? 'Not enough balance' : '') || | ||
| (minAmount ? 'Min amount is $1 worth of tokens' : '')} | ||
| (minGasAmount && token ? `Min. $1 ${NativeSymbols[token.chainId]} required on ${ChainNames[token.chainId]}` : '')} | ||
| </div> |
There was a problem hiding this comment.
Handle unmapped native symbols/names in the warning.
When token.chainId isn’t present in NativeSymbols / ChainNames (e.g., Gnosis chainId 100), the UI renders Min. $1 undefined required on undefined, which is a broken user-facing message. Please add sensible fallbacks (or extend the maps) so we never display undefined to users.
- (minGasAmount && token ? `Min. $1 ${NativeSymbols[token.chainId]} required on ${ChainNames[token.chainId]}` : '')}
+ (minGasAmount && token
+ ? `Min. $1 ${NativeSymbols[token.chainId] ?? 'native token'} required on ${
+ ChainNames[token.chainId] ?? 'this chain'
+ }`
+ : '')}📝 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.
| {relayError || | |
| (notEnoughLiquidity ? 'Not enough balance' : '') || | |
| (minAmount ? 'Min amount is $1 worth of tokens' : '')} | |
| (minGasAmount && token ? `Min. $1 ${NativeSymbols[token.chainId]} required on ${ChainNames[token.chainId]}` : '')} | |
| </div> | |
| {relayError || | |
| (notEnoughLiquidity ? 'Not enough balance' : '') || | |
| (minGasAmount && token | |
| ? `Min. $1 ${NativeSymbols[token.chainId] ?? 'native token'} required on ${ChainNames[token.chainId] ?? 'this chain'}` | |
| : '')} | |
| </div> |
| } | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
I have seen this logic somewhere else. Is it worth doing that call higher up and pass it on to the components? Since it is used in Buy as well I believe?
| @@ -127,14 +159,8 @@ | |||
|
|
|||
| if (input && token) { | |||
| const inputAmount = parseFloat(input); | |||
There was a problem hiding this comment.
why was minAmount removed completely from the logic? I have seen similar logic in buy, please make sure the logic are the same in both buy and sell
There was a problem hiding this comment.
Yes but that was only asked from me to revert the changes and add only gas token check
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/apps/pulse/components/Sell/Sell.tsx (1)
348-352: Handle unmapped native symbols/names in the warning.When
token.chainIdisn't present inNativeSymbols/ChainNames(e.g., Gnosis chainId 100), the UI rendersMin. $1 undefined required on undefined, which is a broken user-facing message. Please add sensible fallbacks (or extend the maps) so we never displayundefinedto users.Apply this diff:
(minGasAmount && token - ? `Min. $1 ${NativeSymbols[token.chainId]} required on ${ChainNames[token.chainId]}` + ? `Min. $1 ${NativeSymbols[token.chainId] ?? 'native token'} required on ${ + ChainNames[token.chainId] ?? 'this chain' + }` : '')}
🧹 Nitpick comments (3)
src/apps/pulse/components/App/HomeScreen.tsx (1)
741-749: Add error handling for portfolio conversion.If
convertPortfolioAPIResponseToTokenthrows an exception,portfolioTokensremains empty, which could cause downstream components (Buy/Sell) to display incorrect warnings or block functionality.Apply this diff to add error handling:
useEffect(() => { if (!walletPortfolioData) return; - const tokens = convertPortfolioAPIResponseToToken( - walletPortfolioData.result.data - ); - - setPortfolioTokens(tokens); + try { + const tokens = convertPortfolioAPIResponseToToken( + walletPortfolioData.result.data + ); + setPortfolioTokens(tokens); + } catch (error) { + console.error('Failed to convert portfolio data:', error); + setPortfolioTokens([]); + } }, [walletPortfolioData]);src/apps/pulse/components/Buy/Buy.tsx (1)
176-180: Update console warning to reflect portfolioTokens check.The warning message "No wallet portfolio data" is now misleading since the condition checks
portfolioTokensinstead ofwalletPortfolioData.Apply this diff:
- if (!portfolioTokens || portfolioTokens.length === 0) { - console.warn('No wallet portfolio data'); + if (!portfolioTokens || portfolioTokens.length === 0) { + console.warn('No portfolio tokens available'); return; }src/apps/pulse/components/Sell/Sell.tsx (1)
130-150: Guard against zero/null values in gas amount calculation.At line 145, if either
nativeToken.priceornativeToken.balanceis explicitly0(not just falsy), the calculation evaluates to0 < 1, incorrectly triggering theminGasAmountwarning even when the user might have a native token with zero price data (due to a temporary API issue) or legitimately zero balance.Consider being more explicit about the check to avoid false positives:
const nativeToken = portfolioTokens.find( (t) => Number(getChainId(t.blockchain as MobulaChainNames)) === token.chainId && isNativeToken(t.contract) ); if (!nativeToken) { setMinGasAmount(true); return; } - if ((nativeToken?.price || 0) * (nativeToken?.balance || 0) < 1) { + const nativeTokenValue = (nativeToken.price ?? 0) * (nativeToken.balance ?? 0); + if (nativeTokenValue < 1) { setMinGasAmount(true); } else { setMinGasAmount(false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/apps/pulse/components/App/HomeScreen.tsx(5 hunks)src/apps/pulse/components/App/tests/AppWrapper.test.tsx(1 hunks)src/apps/pulse/components/Buy/Buy.tsx(6 hunks)src/apps/pulse/components/Buy/tests/Buy.test.tsx(3 hunks)src/apps/pulse/components/Sell/Sell.tsx(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
PR: pillarwallet/x#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/apps/pulse/components/Sell/Sell.tsx
🧬 Code graph analysis (3)
src/apps/pulse/components/Buy/Buy.tsx (1)
src/services/tokensData.ts (1)
PortfolioToken(31-34)
src/apps/pulse/components/App/HomeScreen.tsx (1)
src/services/tokensData.ts (1)
PortfolioToken(31-34)
src/apps/pulse/components/Sell/Sell.tsx (3)
src/services/tokensData.ts (1)
PortfolioToken(31-34)src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/apps/pulse/utils/blockchain.ts (3)
isNativeToken(17-18)NativeSymbols(20-27)ChainNames(29-36)
⏰ 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). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (2)
src/apps/pulse/components/App/tests/AppWrapper.test.tsx (1)
59-62: LGTM! Test mock aligns with new data structure.The mock data correctly reflects the updated API response shape (
assetsinstead oftokens) and includes the newwalletsandbalances_lengthfields.src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
167-190: LGTM! Comprehensive test coverage for portfolioTokens scenarios.The test updates correctly supply the new
portfolioTokensprop with appropriate mock data for different wallet states (normal balance, low balance, insufficient gas). The mock data structure aligns with thePortfolioTokentype definition.Also applies to: 545-568, 653-676
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Tests