feat(trading): new trading selectors #17619
Conversation
|
🚀 Expo preview is ready!
|
a71588f to
e4eae16
Compare
e4eae16 to
5ffa0d1
Compare
WalkthroughThe changes introduce two new JSON fixture files containing data for cryptocurrencies and platforms. The new Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
suite-common/trading/src/utils/infoUtils.ts (2)
8-9: Consider handling undefined cases consistentlyWhile this function works correctly when a coin exists, it might throw an error if
getCoinInfoByCryptoIdreturns undefined (trying to call.toUpperCase()on undefined).-export const getCoinSymbolByCryptoId = (coins: Coins, cryptoId: CryptoId): string | undefined => - getCoinInfoByCryptoId(coins, cryptoId)?.symbol.toUpperCase(); +export const getCoinSymbolByCryptoId = (coins: Coins, cryptoId: CryptoId): string | undefined => { + const coinInfo = getCoinInfoByCryptoId(coins, cryptoId); + return coinInfo?.symbol?.toUpperCase(); +};
29-35: Potential unnecessary object creationThis function creates a new object with potentially undefined properties on every call. Consider handling the case where both inputs are undefined more efficiently.
export const getSymbolAndContractAddressByCryptoId = ( coins: Coins | undefined, cryptoId: CryptoId | undefined, ): { coinSymbol: string | undefined; contractAddress: string | undefined } => { + if (!coins || !cryptoId) { + return { coinSymbol: undefined, contractAddress: undefined }; + } + + return { + coinSymbol: getCoinInfoByCryptoId(coins, cryptoId)?.symbol, + contractAddress: parseCryptoId(cryptoId).contractAddress, + }; -): { coinSymbol: string | undefined; contractAddress: string | undefined } => ({ - coinSymbol: cryptoId ? getCoinInfoByCryptoId(coins ?? {}, cryptoId)?.symbol : undefined, - contractAddress: cryptoId ? parseCryptoId(cryptoId).contractAddress : undefined, -}); };suite-common/trading/src/selectors/tradingSelectors.ts (2)
1-17: Consider importing from the main package path.The import on line 4 references a development source (
libDev), which might not be ideal for production code.-import { NetworkSymbolExtended } from '@suite-common/wallet-config/libDev/src'; +import { NetworkSymbolExtended } from '@suite-common/wallet-config';This would ensure you're using the production version of the module rather than a development one.
150-163: Consider consistent type annotation style.The type annotation for this function is defined separately from the
createMemoizedSelectorcall. While functionally correct, a more consistent approach would improve code clarity.-export const selectSymbolAndContractAddressByCryptoId: ( - state: TradingRootState, - cryptoId: CryptoId, -) => { - coinSymbol: NetworkSymbolExtended | undefined; - contractAddress: string | undefined; -} = createMemoizedSelector( +export const selectSymbolAndContractAddressByCryptoId = createMemoizedSelector< + TradingRootState, + CryptoId, + { + coinSymbol: NetworkSymbolExtended | undefined; + contractAddress: string | undefined; + } +>(This makes the type annotations more consistent with TypeScript conventions and improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
suite-common/trading/src/__fixtures__/coins.json(1 hunks)suite-common/trading/src/__fixtures__/platforms.json(1 hunks)suite-common/trading/src/hooks/useTradingInfo.ts(2 hunks)suite-common/trading/src/selectors/__tests__/tradingSelectors.test.ts(1 hunks)suite-common/trading/src/selectors/tradingSelectors.ts(2 hunks)suite-common/trading/src/thunks/buy/handleRequestThunk.ts(3 hunks)suite-common/trading/src/thunks/buy/selectQuoteThunk.ts(2 hunks)suite-common/trading/src/utils/__tests__/infoUtils.test.ts(1 hunks)suite-common/trading/src/utils/infoUtils.ts(1 hunks)suite-common/trading/tsconfig.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: Unit Tests
- GitHub Check: run-e2e-suite-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
🔇 Additional comments (34)
suite-common/trading/src/__fixtures__/coins.json (1)
1-52: Well-structured cryptocurrency data objectThe new coins.json file provides a well-organized structure for cryptocurrency information, including:
- Basic cryptocurrency details (symbol, name, coingeckoId)
- Service availability flags (buy, sell, exchange capabilities)
- Support for both native coins and tokens (using a platform--contractAddress format for token IDs)
This seems to be a good foundation for handling multiple cryptocurrencies in the trading system.
suite-common/trading/src/utils/infoUtils.ts (3)
5-7: Simple and focused utility functionThis utility function has a clear single responsibility - retrieving coin information by cryptoId. The implementation is straightforward and efficient.
11-14: Simple platform info retrieval functionLike the coin information function, this utility has a clear purpose and implementation.
16-27: Good fallback mechanism for native coin symbolsThe function tries to get the native coin symbol from platform info first, then falls back to the coin info if needed. The nullish coalescing operator (
??) is used appropriately here.suite-common/trading/tsconfig.json (1)
17-18: Added necessary JSON file supportIncluding JSON files in the TypeScript compilation process is essential since the new implementation uses JSON files as data sources. This change ensures that TypeScript properly recognizes and processes the JSON files in the project.
suite-common/trading/src/__fixtures__/platforms.json (1)
1-12: Well-structured platforms dataThe platforms.json file provides a clean structure for platform information, with consistent property naming. Both Ethereum and Base platforms use "eth" as their native coin symbol, which is correctly reflected in the data.
suite-common/trading/src/thunks/buy/selectQuoteThunk.ts (2)
11-11: Good refactor for better naming conventionThe renamed import
selectCoinSymbolByCryptoIdfollows a more consistent naming pattern than the previouscryptoIdToCoinSymbol, making it clearer that this is a selector function.
42-42: Good addition of fallback valueThe null coalescing operator (
??) with 'unknown' fallback is a good defensive programming practice, ensuring the code handles cases where the symbol might not be found.suite-common/trading/src/utils/__tests__/infoUtils.test.ts (6)
1-12: Good test organization and importsThe imports are well-structured, bringing in both the necessary types from 'invity-api' and the fixture data needed for testing. All utility functions being tested are properly imported.
13-22: Clear test data setupThe
btcCoinInfoconstant provides a clean reference object for testing, making the test assertions more readable.
24-31: Well-written basic function testsThe tests for
getCoinInfoByCryptoIdandgetCoinSymbolByCryptoIdare clear and focused, each verifying a specific aspect of the function behavior.
33-39: Good platform selection testThe test for
getPlatformsInfoByCryptoIdproperly verifies that the function returns the correct platform information structure.
41-50: Efficient testing with it.eachUsing
it.eachis an excellent pattern for testing multiple scenarios efficiently. The test cases cover important variations including tokens with contract addresses.
52-84: Comprehensive edge case testingThis section thoroughly tests
getSymbolAndContractAddressByCryptoIdwith various inputs, including edge cases like undefined inputs. Well-structured with nested describes for better organization.suite-common/trading/src/thunks/buy/handleRequestThunk.ts (3)
13-13: Good refactor for better naming conventionThe renamed import
selectCoinSymbolByCryptoIdfollows a more consistent naming pattern, making it clearer that this is a selector function.
33-34: Simplified function implementationThe
getQuotesRequestfunction has been simplified from an async function with await to a direct return arrow function. This makes the code more concise without changing functionality.
129-131: Good addition of fallback valueUsing the null coalescing operator (
??) with the originalrequestData.receiveCurrencyas fallback is a good defensive programming practice, ensuring the code handles cases where the symbol might not be found.suite-common/trading/src/selectors/__tests__/tradingSelectors.test.ts (7)
1-17: Well-structured imports and type definitionsThe imports are organized logically, bringing in both fixture data and the selector functions being tested. The explicit import of
TradingRootStatehelps ensure type safety in the tests.
18-42: Good test state setupThe mock state is well-structured, containing all necessary data for testing the selectors. Using spread operators with the initial state ensures the test state matches the actual application state structure.
44-54: Clear basic selector testsThe tests for
selectTradingBuyQuotesRequestandselectTradingBuySelectedQuoteare straightforward and verify that the selectors return the expected parts of the state.
56-71: Thorough coin data selector testsThe tests for
selectCoinInfoByCryptoIdandselectCoinSymbolByCryptoIdeffectively verify that these selectors return the correct coin information and properly formatted symbol.
73-79: Good platform selector testThe test for
selectPlatformByCryptoIdproperly verifies that the selector returns the correct platform information structure.
81-90: Efficient testing with it.each for native coin symbolsUsing
it.eachis an excellent pattern for testing multiple scenarios efficiently. The test cases cover important variations including tokens with contract addresses.
92-116: Comprehensive testing of symbol and contract address selectorThis section thoroughly tests
selectSymbolAndContractAddressByCryptoIdwith various inputs. The addition of a stability test (line 111-114) is particularly good practice, ensuring that the selector function is memoized correctly and returns consistent results for the same input.suite-common/trading/src/hooks/useTradingInfo.ts (6)
26-32: Good refactoring to utility functions.The introduction of these utility functions from
infoUtils.tsimproves code modularity and reusability. This approach separates the implementation details from the hooks, making the code more maintainable.
104-104: Clean implementation using utility function.Good refactoring to leverage the utility function instead of direct access to the platforms object.
108-111: Appropriate use of utility function.The function has been refactored to use the imported utility, which is a good improvement for maintainability.
114-114: Good simplification using utility.The function now delegates the complexity to the utility function, making this hook easier to understand.
119-119: Clean implementation with utility.Good refactoring to use the utility function instead of direct manipulation.
124-124: Improved implementation with utility.Good refactoring to delegate the logic to a specialized utility function.
suite-common/trading/src/selectors/tradingSelectors.ts (4)
126-130: Well-structured selector implementation.Good implementation using the utility function to retrieve coin information.
132-136: Clean selector implementation.The new selector properly leverages the utility function for retrieving coin symbols.
138-142: Good addition of platform selector.This selector cleanly retrieves platform information using the appropriate utility.
144-148: Well-implemented native coin symbol selector.The selector correctly uses the utility to retrieve native coin symbols.
| @@ -39,7 +39,7 @@ export const selectQuoteThunk = createThunk( | |||
| // consent to continue (modal) | |||
| const result = await userConsent( | |||
| provider.name, | |||
| cryptoIdToCoinSymbol(getState(), quote.receiveCurrency), | |||
| selectCoinSymbolByCryptoId(getState(), quote.receiveCurrency) ?? 'unknown', | |||
There was a problem hiding this comment.
It probably shouldnt get here without CoinSymbol and should return few lines before, wdyt?
|
|
||
| import { createWeakMapSelector, returnStableArrayIfEmpty } from '@suite-common/redux-utils'; | ||
| import { NetworkSymbolExtended } from '@suite-common/wallet-config/libDev/src'; |
There was a problem hiding this comment.
🐰 is right here
| import { NetworkSymbolExtended } from '@suite-common/wallet-config/libDev/src'; | |
| import { NetworkSymbolExtended } from '@suite-common/wallet-config' |
| return allQuotes; | ||
| }; | ||
| const getQuotesRequest = ({ requestData, signal }: GetQuotesRequest) => | ||
| invityAPI.getBuyQuotes(requestData, signal); |
Description
Related Issue
Resolve
Screenshots: