feat(suite-native): wire approval and revoke screen to trade data #22138
Conversation
tomasjakl
force-pushed
the
21192-mobile-swap-refactor-approval-screen-and-wire-it-to-trade-data
branch
from
922ae03 to
8598d73
Compare
tomasjakl
force-pushed
the
21192-mobile-swap-refactor-approval-screen-and-wire-it-to-trade-data
branch
from
8598d73 to
b913f78
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
suite-native/module-trading/src/hooks/exchange/__tests__/useExchangeSelectQuote.test.ts (1)
225-225: Consider explicit params for TradingExchangePreview navigation.The tests pass an empty object
{}for TradingExchangePreview params. While technically valid sinceisApprovedis optional, consider being more explicit for clarity:-expect(mockNavigation.navigate).toHaveBeenCalledWith('TradingExchangePreview', {}); +expect(mockNavigation.navigate).toHaveBeenCalledWith('TradingExchangePreview', { isApproved: false });This makes the intent clearer and matches the param type definition.
Also applies to: 287-287, 311-311
suite-native/module-trading/src/components/exchange/ExchangeApprovalLimitSheet/__tests__/ExchangeApprovalLimitSheet.comp.test.tsx (1)
101-111: Strengthen the “passes correct props” assertionsThese two tests never interact with the sheet – they just confirm that the mocked callback exists, so a regression in the approval-type wiring would still pass. Please drive the component (e.g., press the Unlimited/Limited card) and assert that
mockOnApprovalTypeSelectreceives'INFINITE'/'MINIMAL', and optionally that the selected state updates. This will turn the tests into real safeguards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
suite-native/intl/src/messages.ts(1 hunks)suite-native/module-trading/src/components/exchange/ExchangeApprovalLimitSheet/ExchangeApprovalLimitSheet.tsx(4 hunks)suite-native/module-trading/src/components/exchange/ExchangeApprovalLimitSheet/__tests__/ExchangeApprovalLimitSheet.comp.test.tsx(4 hunks)suite-native/module-trading/src/components/exchange/ExchangePreview/ExchangePreviewView.tsx(2 hunks)suite-native/module-trading/src/hooks/exchange/__tests__/useExchangeSelectQuote.test.ts(3 hunks)suite-native/module-trading/src/hooks/exchange/useExchangeSelectQuote.ts(3 hunks)suite-native/module-trading/src/screens/TradingExchangeApprovalScreen.tsx(5 hunks)suite-native/module-trading/src/screens/TradingExchangePreviewScreen.tsx(3 hunks)suite-native/module-trading/src/screens/TradingExchangeRevokeScreen.tsx(5 hunks)suite-native/module-trading/src/screens/__tests__/TradingExchangeApprovalScreen.comp.test.tsx(2 hunks)suite-native/module-trading/src/screens/__tests__/TradingExchangePreviewScreen.comp.test.tsx(3 hunks)suite-native/module-trading/src/screens/__tests__/TradingExchangeRevokeScreen.comp.test.tsx(2 hunks)suite-native/module-trading/src/utils/general/__tests__/approvalStatusUtils.test.ts(2 hunks)suite-native/module-trading/src/utils/general/approvalStatusUtils.ts(1 hunks)suite-native/navigation/src/navigators.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-27T09:59:23.527Z
Learnt from: vytick
PR: trezor/trezor-suite#20985
File: suite-native/module-trading/src/screens/TradingExchangePreviewScreen.tsx:92-106
Timestamp: 2025-08-27T09:59:23.527Z
Learning: The TradingExchangePreviewScreen in suite-native/module-trading/src/screens/TradingExchangePreviewScreen.tsx is POC code with intentional technical debt around error handling in flow steps, as it will be heavily reworked in follow-up PRs.
Applied to files:
suite-native/module-trading/src/screens/__tests__/TradingExchangeApprovalScreen.comp.test.tsxsuite-native/module-trading/src/screens/__tests__/TradingExchangeRevokeScreen.comp.test.tsxsuite-native/module-trading/src/hooks/exchange/__tests__/useExchangeSelectQuote.test.tssuite-native/module-trading/src/components/exchange/ExchangePreview/ExchangePreviewView.tsxsuite-native/module-trading/src/screens/__tests__/TradingExchangePreviewScreen.comp.test.tsxsuite-native/navigation/src/navigators.tssuite-native/module-trading/src/hooks/exchange/useExchangeSelectQuote.tssuite-native/module-trading/src/screens/TradingExchangePreviewScreen.tsxsuite-native/module-trading/src/screens/TradingExchangeRevokeScreen.tsxsuite-native/module-trading/src/screens/TradingExchangeApprovalScreen.tsx
📚 Learning: 2025-02-04T13:18:21.530Z
Learnt from: jbazant
PR: trezor/trezor-suite#16668
File: suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx:0-0
Timestamp: 2025-02-04T13:18:21.530Z
Learning: The mock data arrays in `suite-native/module-trading/src/components/TradeableAssetsSheet/TradeableAssetsSheet.tsx` are intentionally used as visual stubs and will be replaced with real data fetching in future commits.
Applied to files:
suite-native/module-trading/src/screens/__tests__/TradingExchangeApprovalScreen.comp.test.tsxsuite-native/module-trading/src/screens/__tests__/TradingExchangePreviewScreen.comp.test.tsx
📚 Learning: 2025-02-21T23:00:49.788Z
Learnt from: jbazant
PR: trezor/trezor-suite#17164
File: suite-native/module-trading/src/components/general/TradeableAssetsSheet/__tests__/TradeableAssetListItem.comp.test.tsx:61-75
Timestamp: 2025-02-21T23:00:49.788Z
Learning: The `renderWithStore` utility from `suite-native/test-utils` accepts a `preloadedState` option to set up initial Redux state for tests. The store initialization is asynchronous, so tests should use `findBy*` queries instead of `getBy*` queries.
Applied to files:
suite-native/module-trading/src/screens/__tests__/TradingExchangeApprovalScreen.comp.test.tsx
📚 Learning: 2025-09-30T20:12:08.320Z
Learnt from: jbazant
PR: trezor/trezor-suite#22046
File: suite-native/module-trading/src/components/exchange/ExchangePreview/ExchangeFeePickerCard.tsx:13-28
Timestamp: 2025-09-30T20:12:08.320Z
Learning: In suite-native/module-trading, when a parent component like ExchangePreviewView is memoized, child components such as ExchangeFeePickerCard don't need to be memoized as they won't re-render unless the parent re-renders, and adding memo() would only introduce unnecessary overhead.
Applied to files:
suite-native/module-trading/src/components/exchange/ExchangePreview/ExchangePreviewView.tsx
📚 Learning: 2025-09-30T20:12:19.587Z
Learnt from: jbazant
PR: trezor/trezor-suite#22046
File: suite-native/module-trading/src/components/exchange/ExchangePreview/ExchangeFromAccountTradePreviewCard.tsx:16-38
Timestamp: 2025-09-30T20:12:19.587Z
Learning: In suite-native/module-trading, when a component like ExchangeFromAccountTradePreviewCard is only used from an already memoized parent component (like ExchangePreviewView), adding React.memo to the child component adds overhead without performance benefit and should be avoided.
Applied to files:
suite-native/module-trading/src/components/exchange/ExchangePreview/ExchangePreviewView.tsx
🔇 Additional comments (12)
suite-native/module-trading/src/screens/__tests__/TradingExchangeApprovalScreen.comp.test.tsx (2)
15-20: LGTM! Mock for useExchangeFlow is well-structured.The mock provides the necessary
confirmTradeandfetchFeesAndComposefunctions that the screen under test expects from the exchange flow hook.
56-69: Approve code:quoteextracted from route.params TradingExchangeApprovalScreen destructuresquoteon line 48; no changes needed.suite-native/intl/src/messages.ts (1)
2318-2318: LGTM! Translation key properly added.The new
approvalSuccessAlertmessage is correctly placed undermoduleTrading.tradingExchangePreviewScreenand provides clear user-facing text for the approval success state.suite-native/module-trading/src/screens/__tests__/TradingExchangeRevokeScreen.comp.test.tsx (2)
38-51: LGTM! Test properly updated for route-based data flow.The renderScreen function now correctly passes the route prop with
{ params: { quote: testQuote } }, aligning with the updated navigation parameter types.
75-80: Correct test update for multiple matching elements.Using
getAllByTextand expecting 2 occurrences of "0 USDC" is the proper way to test when the same text appears multiple times on screen (for current limit and new limit).suite-native/module-trading/src/hooks/exchange/__tests__/useExchangeSelectQuote.test.ts (1)
258-399: Excellent test coverage of approval status navigation paths.The new test suite thoroughly covers all approval status scenarios:
approved→ TradingExchangePreviewnot_needed→ TradingExchangePreviewneeds_approval→ TradingExchangeApprovalneeds_increasewith token support → TradingExchangeApproval with shouldIncreaseLimitneeds_increasewithout token support → TradingExchangeRevoke with shouldIncreaseLimitsuite-native/module-trading/src/screens/__tests__/TradingExchangePreviewScreen.comp.test.tsx (1)
56-66: Well-designed test helper with optional parameter.The
renderTradingExchangePreviewScreenhelper now accepts an optionalisApprovedparameter (defaulting tofalse), making it easy to test both the approval success alert and normal preview flows.suite-native/module-trading/src/screens/TradingExchangePreviewScreen.tsx (2)
39-43: LGTM! Route params properly extracted.The component correctly destructures
paramsfrom the route prop and extractsisApproved. The type system guaranteesparamsexists (as an object with optionalisApproved), so the destructuring is safe.
143-155: Approval success alert properly implemented.The conditional rendering of the approval success alert wrapped in
Animated.Viewis well-structured:
- Alert only shows when
isApprovedis truthy- Uses proper translation key for the message
- Success variant provides appropriate visual feedback
- VStack provides consistent spacing
suite-native/navigation/src/navigators.ts (2)
2-2: LGTM! Required type import added.The
ExchangeTradeimport frominvity-apiis necessary for typing the route params that now carry quote data.
342-353: Navigation param types properly defined.The updated param types for the trading exchange flow are well-structured:
TradingExchangePreview: Simple optional boolean flag for approval success stateTradingExchangeApproval: Quote data plus optional flags for limit increase and revoke statusTradingExchangeRevoke: Quote data plus optional flag for limit increaseThis enables type-safe route-based data flow across the approval/revoke screens.
suite-native/module-trading/src/components/exchange/ExchangeApprovalLimitSheet/ExchangeApprovalLimitSheet.tsx (1)
59-74: Nice token-aware formattingGreat touch splitting into
CryptoAmountFormattervsTokenAmountFormatter; this keeps network and token approvals formatted with the right precision.
suite-native/module-trading/src/components/exchange/ExchangePreview/ExchangePreviewView.tsx
Show resolved
Hide resolved
suite-native/module-trading/src/screens/TradingExchangeApprovalScreen.tsx
Show resolved
Hide resolved
| } | ||
| }, [isVisible, openModal]); | ||
| useEffect( | ||
| () => (isVisible ? openModal() : closeModal()), |
There was a problem hiding this comment.
This calls openModal when removing from parent, right? Do we want it? Why did the previous version not work?
| } | ||
|
|
||
| const limitAmount = `200.32 ${coinSymbol}`; //TODO | ||
| const limitAmount = |
There was a problem hiding this comment.
nit:
| const limitAmount = | |
| const limitAmountFormatter = |
There was a problem hiding this comment.
I would avoid unnecessary re-renders for this one. Could we use a useMemo or useCallback here, please?
| contractAddress={contractAddress} | ||
| isChecked={true} | ||
| isChecked={selectedApprovalType === 'INFINITE'} | ||
| onChange={() => { |
There was a problem hiding this comment.
nit
| onChange={() => { | |
| onChange={() => onApprovalTypeSelect('INFINITE')} |
| contractAddress={contractAddress} | ||
| isChecked={false} | ||
| isChecked={selectedApprovalType === 'MINIMAL'} | ||
| onChange={() => { |
There was a problem hiding this comment.
nit
| onChange={() => { | |
| onChange={() => onApprovalTypeSelect('MINIMAL')} |
| <ExchangeFeePickerCard quote={quote} isTxnError={isTxnError} /> | ||
| </VStack> | ||
| </ScrollView> | ||
| <> |
There was a problem hiding this comment.
Q: are there now correct spaces between children? previously 20pts
| export const TradingExchangeApprovalScreen = ({ | ||
| route: { params }, | ||
| }: TradingExchangeApprovalScreenProps) => { | ||
| const { quote, shouldIncreaseLimit, isRevoked } = params; |
There was a problem hiding this comment.
I am still not sure about passing quote via params, but lets give it a try and refactor later if necessary
|
|
||
| if (success) { | ||
| // TODO | ||
| await fetchFeesAndCompose(); |
There was a problem hiding this comment.
This wont be needed here, lets delete so we are not confused later
| return 'needs_approval'; | ||
| }; | ||
|
|
||
| export const tokenSupportsIncreasingAllowance = (contractAddress?: string): boolean => { |
There was a problem hiding this comment.
This can be easily moved to suite-common/trading from desktop and reused. Please, lets do that, so there is single source of truth for both apps.
| return ( | ||
| <Screen header={<ExchangePreviewScreenHeader />}> | ||
| <ExchangePreviewView quote={quote} txnErrorString={txnErrorString} /> | ||
| <VStack spacing="sp20" paddingVertical="sp20"> |
There was a problem hiding this comment.
Please move this into ExchangePreviewView
| ({ isVisible, onDismiss }: ExchangeApprovalLimitSheetProps) => { | ||
| const { bottomSheetRef, openModal } = useBottomSheetModal(); | ||
| ({ | ||
| isVisible, |
There was a problem hiding this comment.
What about moving useBottomSheetModal one level up instead of using isVisible? It feels redundant.. maybe we could change the logic directly in useBottomSheetControls What do you think?
There was a problem hiding this comment.
Then you could remove the the useEffect logic below as well
| const isIncreasingAllowanceSupported = | ||
| tokenSupportsIncreasingAllowance(contractAddress); | ||
|
|
||
| if (approvalStatus === 'needs_increase' && isIncreasingAllowanceSupported) { |
There was a problem hiding this comment.
nit: Maybe we can simpify it a bit?
if (approvalStatus === 'needs_increase') {
const route = isIncreasingAllowanceSupported
? TradingStackRoutes.TradingExchangeApproval
: TradingStackRoutes.TradingExchangeRevoke;
return navigation.navigate(route, {
quote: candidateQuote,
shouldIncreaseLimit: true,
});
}
// fallback
return navigation.navigate(TradingStackRoutes.TradingExchangeApproval, {
quote: candidateQuote,
});
| variant="info" | ||
| /> | ||
| {!!shouldIncreaseLimit && ( | ||
| <Animated.View> |
There was a problem hiding this comment.
nit: we can use AnimatedBox from atoms
| <Button | ||
| onPress={() => { | ||
| // TODO | ||
| if (shouldIncreaseLimit) { |
There was a problem hiding this comment.
nit: could we extract it to a separate function please?
Description
Related Issue
Resolve #21192
🔍🖥️ Suite native android test results: View in Currents