chore(trading): move buy trading part to suite-common #16866
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a Redux-based implementation for managing trading functionality in the suite-common module, effectively duplicating the existing context-based approach. Changes include updates to import paths in hooks and components, the addition of new trading constants and utility functions, and enhancements to the trading middleware to handle new conditions using updated constants. New Redux action creators, reducers, thunks, and fixtures have been added for both trading and buying functionalities. Comprehensive test suites and configurations (including a new Jest config) have been introduced to ensure robust coverage. Additionally, package and TypeScript configuration files have been updated to incorporate new dependencies and references. Public exports in the trading module have been expanded to integrate the new Redux state management without affecting the original Suite code. Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
packages/suite/src/middlewares/wallet/tradingMiddleware.ts (1)
84-120: Consider implementing retry logic for failed API calls.The middleware loads data from multiple APIs but doesn't handle retries for failed requests.
Implement retry logic for API calls:
const retryWithBackoff = async (fn: () => Promise<void>, maxRetries = 3) => { for (let i = 0; i < maxRetries; i++) { try { await fn(); return; } catch (error) { if (i === maxRetries - 1) throw error; await new Promise(resolve => setTimeout(resolve, Math.pow(2, i) * 1000)); } } }; // Usage in loadPromises loadPromises.push( retryWithBackoff(() => invityAPI.getInfo().then(info => { api.dispatch(tradingInfoAction.saveInfo(info)); }) ) );
🧹 Nitpick comments (17)
suite-common/trading/src/selectors/tradingSelectors.ts (2)
8-30: Consider reducing coupling with Suite state.While the type structure is well-defined, being a partial copy of Suite state creates tight coupling. This could lead to maintenance challenges if the Suite state structure changes.
Consider:
- Creating an interface that defines only the essential properties needed for trading functionality.
- Using type composition to extend the base trading interface when Suite-specific properties are needed.
- Adding documentation to explain the relationship between this type and the Suite state.
This would make the trading module more independent and easier to maintain.
32-32: Enhance selector performance with memoization.The current selector returns the entire state, which could cause unnecessary re-renders. Consider using
createSelectorfrom@reduxjs/toolkitto create specific, memoized selectors for better performance.Example implementation:
import { createSelector } from '@reduxjs/toolkit'; export const selectTradingState = (state: TradingRootState) => state.wallet.trading; export const selectSelectedAccount = (state: TradingRootState) => state.wallet.selectedAccount; export const selectTradingWithAccount = createSelector( [selectTradingState, selectSelectedAccount], (trading, selectedAccount) => ({ trading, selectedAccount, }) );suite-common/trading/src/reducers/buyReducer.ts (3)
24-33: Add JSDoc comments to document state properties.The interface would benefit from documentation explaining the purpose and lifecycle of each state property.
Add JSDoc comments to improve maintainability:
export interface TradingBuyState { + /** Buy information including supported currencies and provider details */ buyInfo?: BuyInfo; + /** Flag indicating if the current state resulted from a redirect */ isFromRedirect: boolean; + /** Current quote request parameters */ quotesRequest?: BuyTradeQuoteRequest; + /** List of available quotes matching the current request */ quotes: BuyTrade[] | undefined; + /** Currently selected quote for processing */ selectedQuote: BuyTrade | undefined; + /** Address that has been verified for the current transaction */ addressVerified: string | undefined; + /** Unique identifier for the current transaction */ transactionId?: string; }
35-43: Consider using null instead of undefined for optional fields.Redux serialization works better with
nullthanundefined. Usingnullalso makes it easier to distinguish between "not set" and "intentionally empty" states.export const buyInitialState: TradingBuyState = { - transactionId: undefined, + transactionId: null, isFromRedirect: false, - buyInfo: undefined, + buyInfo: null, - quotesRequest: undefined, + quotesRequest: null, - selectedQuote: undefined, + selectedQuote: null, quotes: [], - addressVerified: undefined, + addressVerified: null, };
45-74: Consider adding error handling for edge cases.The reducer handles the happy path well but could benefit from additional error handling for edge cases.
Consider adding validation and error handling:
export const prepareBuyReducer = createReducerWithExtraDeps(buyInitialState, builder => { builder .addCase(tradingBuyActions.saveBuyInfo, (state, { payload }) => { + if (!payload) { + return; + } state.buyInfo = payload; }) .addCase(tradingBuyActions.saveQuotes, (state, { payload }) => { + if (!Array.isArray(payload)) { + state.quotes = []; + return; + } state.quotes = payload; }) // ... other cases });suite-common/trading/src/thunks/tradingThunks.ts (1)
70-72: Use action creators instead of dispatching raw action objectsFor consistency and to leverage TypeScript type safety, consider using action creators when dispatching actions. This improves code maintainability and readability.
Apply this diff to use the action creator:
- dispatch({ - type: tradingAction, - payload: address, - }); + dispatch(tradingBuyActions.verifyAddress(address));suite-common/trading/src/reducers/tradingReducer.ts (1)
79-123: Remove commented-out code to improve readabilityThe reducer contains large blocks of commented-out code, which can clutter the file and reduce readability. If this code is no longer needed, consider removing it. If it's needed for future development, move it to a separate branch or document it elsewhere.
Apply this diff to remove commented-out sections:
buy: buyInitialState, - /* - exchange: { - exchangeInfo: undefined, - transactionId: undefined, - quotesRequest: undefined, - quotes: [], - addressVerified: undefined, - tradingAccountKey: undefined, - selectedQuote: undefined, - isFromRedirect: false, - }, - sell: { - sellInfo: undefined, - quotesRequest: undefined, - quotes: [], - selectedQuote: undefined, - transactionId: undefined, - isFromRedirect: false, - tradingAccountKey: undefined, - }, - */suite-common/trading/src/actions/buyActions.ts (1)
19-42: Type-safe action creators with proper payload handling.Good use of Redux Toolkit's
createActionwith explicit typing of payloads. Consider adding JSDoc comments to document the purpose of each action creator.Example for one action:
+/** + * Saves buy information including provider details and supported currencies + * @param payload Buy information containing provider and currency details + */ const saveBuyInfo = createAction(SAVE_BUY_INFO, (payload: BuyInfo) => ({ payload }));suite-common/trading/src/thunks/buyThunks.ts (1)
31-39: Optimize data transformation for better readability and performance.The current implementation using multiple array operations could be simplified.
Consider using Set operations directly:
-const tradedFiatCurrencies: string[] = []; -const tradedCoins: CryptoId[] = []; -buyInfo.providers.forEach(p => { - tradedFiatCurrencies.push(...p.tradedFiatCurrencies.map(c => c.toLowerCase())); - tradedCoins.push(...p.tradedCoins); -}); -const supportedFiatCurrencies = new Set(tradedFiatCurrencies); -const supportedCryptoCurrencies = new Set(tradedCoins); +const supportedFiatCurrencies = new Set( + buyInfo.providers.flatMap(p => p.tradedFiatCurrencies.map(c => c.toLowerCase())) +); +const supportedCryptoCurrencies = new Set( + buyInfo.providers.flatMap(p => p.tradedCoins) +);suite-common/trading/src/thunks/__tests__/tradingThunk.test.ts (1)
73-73: Address TODO comment for additional test coverage.The TODO comment indicates missing test coverage. I can help implement additional test cases.
Would you like me to generate additional test cases for:
- Error scenarios
- Edge cases
- Different device states?
suite-common/trading/src/thunks/__tests__/buyThunk.test.ts (1)
14-85: Consider adding more test cases for comprehensive coverage.The test file currently only covers the
loadInfoThunk. Consider adding test cases for other thunks that might exist in the buy trading functionality.suite-common/trading/src/utils.ts (1)
160-174: Improve robustness of route type determination.The function could be more explicit and handle edge cases better:
- Add explicit return type annotation
- Add explicit return undefined for unmatched routes
- Consider handling invalid route names
Consider this implementation:
export const getTradeTypeByRoute = ( routeName: Route['name'] | undefined, -): TradingType | undefined => { +): TradingType | undefined => { + if (!routeName) { + return undefined; + } + if (routeName?.startsWith('wallet-trading-buy')) { return 'buy'; } if (routeName?.startsWith('wallet-trading-sell')) { return 'sell'; } if (routeName?.startsWith('wallet-trading-exchange')) { return 'exchange'; } + + return undefined; };suite-common/trading/src/reducers/__fixtures__/tradingReducer.ts (3)
13-30: Initial state should be frozen to prevent accidental mutations.The initial state object is mutable, which could lead to unintended side effects in tests if the object is accidentally modified.
Apply this diff to freeze the initial state:
-const initialState: TradingState = { +const initialState: TradingState = Object.freeze({ info: { platforms: undefined, coins: undefined, paymentMethods: [], }, buy: buyInitialState, // TODO: sell: // TODO: exchange: composedTransactionInfo: {}, trades: [], isLoading: false, modalAccountKey: undefined, modalCryptoId: undefined, lastLoadedTimestamp: 0, activeSection: 'buy', prefilledFromCryptoId: undefined, -}; +});
32-56: Consider using BigInt for cryptocurrency amounts.The amounts in the trade fixtures are using strings and floating-point numbers, which can lead to precision issues when dealing with cryptocurrency values.
Consider using BigInt for amounts to ensure precise calculations:
- fiatStringAmount: '47.12', + fiatStringAmount: '4712', // Store as minor units (cents) - receiveStringAmount: '0.004705020432603938', + receiveStringAmount: '470502043260', // Store as satoshis - rate: 10014.834297738, + rate: '10014834297738', // Store as minor units
126-335: Enhance test coverage with edge cases.The test cases cover basic functionality but could be enhanced with edge cases:
- Test behavior with invalid/malformed data
- Test state transitions with concurrent actions
- Test error handling scenarios
Add the following test cases to the
tradingFixturesarray:{ description: 'test handling malformed trade data', initialState, actions: [{ type: TRADING_ACTIONS.SAVE_TRADE, payload: { ...tradeBuy, data: undefined }, }], result: { ...initialState, trades: [], }, }, { description: 'test concurrent loading states', initialState: { ...initialState, isLoading: true }, actions: [ { type: TRADING_ACTIONS.SET_LOADING, payload: { isLoading: true, lastLoadedTimestamp: 2 }, }, { type: TRADING_ACTIONS.SET_LOADING, payload: { isLoading: false, lastLoadedTimestamp: 1 }, }, ], result: { ...initialState, isLoading: true, lastLoadedTimestamp: 2, }, }packages/suite/src/middlewares/wallet/tradingMiddleware.ts (1)
64-69: Consider adding a buffer time to prevent rapid reloading.The condition for reloading data might trigger too frequently if the timestamp is close to the threshold.
Add a small buffer time to prevent rapid reloading:
- lastLoadedTimestamp + INVITY_API_RELOAD_DATA_AFTER_MS < Date.now()) + lastLoadedTimestamp + INVITY_API_RELOAD_DATA_AFTER_MS + 1000 < Date.now())suite-common/trading/src/reducers/__fixtures__/account.ts (1)
3-182: Consider using a factory function for generating account fixtures.The account fixtures contain a lot of repeated data that could be generated using a factory function.
Create a factory function to generate account fixtures:
const createAccountFixture = ( accountType: 'normal' | 'taproot', path: string, descriptor: string, key: string ): Account => ({ deviceState: 'mvbu1Gdy8SUjTenqerxUaZyYjmveZvt33q@C906E19794613145E3DF45F4:0', index: 0, path, descriptor, key, accountType, symbol: 'btc', empty: false, visible: true, balance: '0', availableBalance: '0', formattedBalance: '0', tokens: [], addresses: { change: [], used: [], unused: [], }, // ... other common properties });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
packages/suite/src/hooks/wallet/trading/form/common/useTradingInitializer.ts(1 hunks)packages/suite/src/hooks/wallet/trading/form/common/useTradingPreviousRoute.ts(1 hunks)packages/suite/src/middlewares/wallet/tradingMiddleware.ts(1 hunks)packages/suite/src/views/wallet/trading/common/TradingHeader/TradingHeader.tsx(1 hunks)suite-common/trading/jest.config.js(1 hunks)suite-common/trading/package.json(1 hunks)suite-common/trading/src/actions/buyActions.ts(1 hunks)suite-common/trading/src/actions/tradingActions.ts(1 hunks)suite-common/trading/src/constants.ts(1 hunks)suite-common/trading/src/index.ts(1 hunks)suite-common/trading/src/middlewares/tradingMiddleware.ts(1 hunks)suite-common/trading/src/reducers/__fixtures__/account.ts(1 hunks)suite-common/trading/src/reducers/__fixtures__/buyTradingReducer.ts(1 hunks)suite-common/trading/src/reducers/__fixtures__/tradingReducer.ts(1 hunks)suite-common/trading/src/reducers/__tests__/buyReducer.test.ts(1 hunks)suite-common/trading/src/reducers/__tests__/tradingReducer.test.ts(1 hunks)suite-common/trading/src/reducers/buyReducer.ts(1 hunks)suite-common/trading/src/reducers/tradingReducer.ts(1 hunks)suite-common/trading/src/selectors/tradingSelectors.ts(1 hunks)suite-common/trading/src/thunks/__tests__/buyThunk.test.ts(1 hunks)suite-common/trading/src/thunks/__tests__/tradingThunk.test.ts(1 hunks)suite-common/trading/src/thunks/buyThunks.ts(1 hunks)suite-common/trading/src/thunks/tradingThunks.ts(1 hunks)suite-common/trading/src/utils.ts(2 hunks)suite-common/trading/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- suite-common/trading/jest.config.js
- packages/suite/src/hooks/wallet/trading/form/common/useTradingPreviousRoute.ts
- suite-common/trading/src/constants.ts
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (26)
suite-common/trading/src/selectors/tradingSelectors.ts (1)
1-6: LGTM! Well-organized imports.The imports are properly scoped and align with the PR objective of moving trading functionality to suite-common.
suite-common/trading/package.json (6)
10-11: Script Updates for Type-Check and Unit Testing:
The addition of"test:unit": "yarn g:jest"alongside the"type-check": "yarn g:tsc --build"command enhances the developer workflow by providing a dedicated unit testing shortcut. Please verify that these commands match your project’s build and test environment.
14-14: Addition of Redux Toolkit Dependency:
Including"@reduxjs/toolkit": "1.9.5"aligns well with the PR’s objective to adopt a Redux-based approach. Ensure that this version meets your project's compatibility and security requirements.
15-15: Integration of Suite-Common Redux Utils:
The new dependency"@suite-common/redux-utils": "workspace:*"is appropriately added to support shared Redux utilities. Make sure that the workspace resolution works as expected within your environment.
17-17: Addition of Wallet Core Dependency:
Adding"@suite-common/wallet-core": "workspace:*"supplements the trading module functions. Confirm that this dependency does not conflict with"@suite-common/wallet-config"and that its integration is properly scoped.
19-19: Inclusion of Trezor Connect Dependency:
Incorporating"@trezor/connect": "workspace:*"supports the necessary interfacing with Trezor functionalities. Please ensure that this workspace dependency delivers the expected API and is consistent with your integration tests.
25-25: Addition of Suite-Common Test Utilities:
The newly added dev dependency"@suite-common/test-utils": "workspace:*"is intended to bolster test coverage for the trading module. Verify its compatibility with your Jest configuration and overall testing strategy.suite-common/trading/src/reducers/buyReducer.ts (1)
1-13: LGTM! Well-organized imports.The imports are logically grouped and follow a clear structure, separating external dependencies, internal utilities, and local module imports.
suite-common/trading/src/reducers/tradingReducer.ts (1)
119-123: Handle missing reducers for 'sell' and 'exchange' actionsThe default case delegates actions to
prepareBuyReducer, butprepareSellReducerandprepareExchangeReducerare commented out. Ensure that reducers for 'sell' and 'exchange' are implemented to handle those actions properly.Do you plan to implement
prepareSellReducerandprepareExchangeReducer? If so, consider adding a timeline or creating tasks to track their development.suite-common/trading/src/index.ts (1)
6-11: LGTM! Verify that all exported modules are properly implemented.The new exports follow a logical structure and align with the PR objectives of moving buy trading functionality to suite-common.
Run the following script to verify that all exported modules exist and are properly implemented:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that all exported modules exist and are properly implemented. # Test: Check if all exported modules exist for module in "reducers/tradingReducer" "middlewares/tradingMiddleware" "actions/tradingActions" "actions/buyActions" "thunks/buyThunks" "thunks/tradingThunks"; do if ! fd -t f "$module\.(ts|tsx)$" suite-common/trading/src/; then echo "Missing module: $module" exit 1 fi doneLength of output: 1399
🏁 Script executed:
#!/bin/bash # Description: Verify that all exported modules exist, are non-empty, and contain at least one 'export' statement, confirming they are properly implemented. modules=("reducers/tradingReducer" "middlewares/tradingMiddleware" "actions/tradingActions" "actions/buyActions" "thunks/buyThunks" "thunks/tradingThunks") errors=0 for module in "${modules[@]}"; do # Find the file with .ts or .tsx extension for the module file=$(fd -t f "$module\.(ts|tsx)$" suite-common/trading/src/ | head -n 1) if [ -z "$file" ]; then echo "Error: Missing file for module '$module'" errors=1 continue fi # Check that the file is non-empty if [ ! -s "$file" ]; then echo "Error: File '$file' is empty" errors=1 continue fi # Verify the file contains an 'export' statement to ensure proper implementation if ! grep -q "export" "$file"; then echo "Error: File '$file' does not contain any export statement" errors=1 continue fi echo "Module '$module' found in file '$file' and passes implementation check." done if [ $errors -ne 0 ]; then exit 1 fi echo "All modules are properly implemented."Length of output: 1929
🏁 Script executed:
#!/bin/bash # Description: Search for exported module files using their basename. modules=("reducers/tradingReducer" "middlewares/tradingMiddleware" "actions/tradingActions" "actions/buyActions" "thunks/buyThunks" "thunks/tradingThunks") for module in "${modules[@]}"; do name=$(basename "$module") echo "Searching for module '$module' using filename pattern '$name.(ts|tsx)$'" fd -t f "$name\.(ts|tsx)$" suite-common/trading/src/ echo "-----------------------------" doneLength of output: 3082
🏁 Script executed:
#!/bin/bash # Description: Verify that each exported module (excluding __fixtures__ directories) exists, # is non-empty, and contains at least one 'export' statement. modules=("reducers/tradingReducer" "middlewares/tradingMiddleware" "actions/tradingActions" "actions/buyActions" "thunks/buyThunks" "thunks/tradingThunks") errors=0 for module in "${modules[@]}"; do name=$(basename "$module") # Exclude __fixtures__ directories to ensure we pick the actual implementation file. file=$(fd -t f --exclude __fixtures__ "$name\.(ts|tsx)$" suite-common/trading/src/ | head -n 1) if [ -z "$file" ]; then echo "Error: Missing file for module '$module' (excluding __fixtures__ directories)" errors=1 continue fi if [ ! -s "$file" ]; then echo "Error: File '$file' is empty" errors=1 continue fi if ! grep -q "export" "$file"; then echo "Error: File '$file' does not contain any 'export' statement" errors=1 continue fi echo "Module '$module' found in file '$file' and passes implementation check." done if [ $errors -ne 0 ]; then exit 1 fi echo "All modules are properly implemented."Length of output: 4619
All exported modules are properly implemented.
suite-common/trading/src/reducers/tradingReducer.tsexists and contains export statements.suite-common/trading/src/middlewares/tradingMiddleware.tsexists and is correctly implemented.suite-common/trading/src/actions/tradingActions.tsexists and is properly implemented.suite-common/trading/src/actions/buyActions.tsexists and is correctly implemented.suite-common/trading/src/thunks/buyThunks.tsexists and contains export statements.suite-common/trading/src/thunks/tradingThunks.tsexists and is correctly implemented.suite-common/trading/src/reducers/__tests__/tradingReducer.test.ts (1)
1-28: LGTM! Consider verifying test coverage.The test implementation follows Redux testing best practices and uses fixtures for comprehensive test cases.
Run the following script to verify test coverage:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for trading reducer. # Test: Search for all possible action types in the reducer rg -t ts "case '.*':|createAction\(" suite-common/trading/src/actions/tradingActions.ts suite-common/trading/src/reducers/tradingReducer.ts # Test: Compare with test fixtures to ensure all actions are covered rg -t ts "description.*:.*" suite-common/trading/src/reducers/__fixtures__/tradingReducer.tsLength of output: 2152
Test coverage verification complete:
The shell script outputs confirm that each trading action declared in the reducer (viacreateActionintradingActions.ts) has a corresponding fixture test in the test suite. The test fixture descriptions cover actions related to loading symbols info, saving trade details, setting modal and crypto parameters, and more. This indicates comprehensive test coverage.suite-common/trading/src/reducers/__tests__/buyReducer.test.ts (1)
1-36: LGTM! Consider verifying test coverage.The test implementation follows Redux testing best practices and uses fixtures for comprehensive test cases.
Run the following script to verify test coverage:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for buy trading reducer. # Test: Search for all possible action types in the reducer rg -t ts "case '.*':|createAction\(" suite-common/trading/src/actions/buyActions.ts suite-common/trading/src/reducers/buyReducer.ts # Test: Compare with test fixtures to ensure all actions are covered rg -t ts "description.*:.*" suite-common/trading/src/reducers/__fixtures__/buyTradingReducer.tsLength of output: 1866
Test coverage verification confirmed
- All action types in
suite-common/trading/src/actions/buyActions.tsare present in the test fixtures.- The fixture file (
suite-common/trading/src/reducers/__fixtures__/buyTradingReducer.ts) includes tests for saving buy info, saving quote request, saving transaction detail id, saving quotes, clearing quotes, verifying address, disposing verifying address, saving selected quote, and setting redirect status.packages/suite/src/hooks/wallet/trading/form/common/useTradingInitializer.ts (1)
3-3: LGTM! Verify the constant value after the move.The import path change aligns with the PR objectives of moving trading functionality to suite-common.
Run the following script to verify that the constant value remains the same after the move:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Compare constant values between old and new locations. # Test: Search for the constant value in both locations echo "New location:" rg -t ts "INVITY_API_RELOAD_QUOTES_AFTER_SECONDS.*=.*" suite-common/trading/src/ echo "Old location:" rg -t ts "INVITY_API_RELOAD_QUOTES_AFTER_SECONDS.*=.*" packages/suite/src/constants/wallet/trading/metadataLength of output: 455
Constant preservation verified.
The constant is now defined insuite-common/trading/src/constants.tswith the value of 30, and the old location is no longer present, confirming the proper move as intended.suite-common/trading/src/actions/buyActions.ts (1)
1-18: Well-structured action types with consistent naming pattern.The action types follow Redux best practices with:
- Common prefix for related actions
- Clear, descriptive names
- Proper string literal type definitions
suite-common/trading/src/thunks/buyThunks.ts (1)
14-25: Robust error handling with proper fallback values.Good defensive programming by handling null/undefined cases with sensible defaults.
suite-common/trading/src/thunks/__tests__/tradingThunk.test.ts (1)
29-71: 🛠️ Refactor suggestionAdd test cases for error scenarios.
While the happy path is tested, consider adding tests for:
- Device not connected
- Address verification failure
- Invalid account/address
packages/suite/src/views/wallet/trading/common/TradingHeader/TradingHeader.tsx (1)
3-3: LGTM! Import path updated correctly.The import path change aligns with the PR objective of moving trading functionality to suite-common.
suite-common/trading/src/thunks/__tests__/buyThunk.test.ts (2)
38-67: LGTM! Test case for successful response is well structured.The test properly validates the transformation of API response data, especially the conversion of defaultAmountsOfFiatCurrencies from object to Map.
69-84: LGTM! Test case for unsuccessful response handles edge case appropriately.The test ensures proper fallback behavior when the API returns undefined.
suite-common/trading/src/actions/tradingActions.ts (3)
7-9: Address TODO comments about prefix unification.The TODO comments suggest technical debt regarding prefix unification. Consider addressing this before the code becomes more established.
Would you like me to help create an issue to track this task?
23-82: LGTM! Action creators are well-structured.The action creators are properly typed and follow Redux Toolkit best practices.
14-14: Clarify the status of SET_LOADING action.The comment suggests this action might be deprecated. Either remove if unused or document why it's still needed.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for usages of SET_LOADING action rg "SET_LOADING|setLoading" -A 3Length of output: 27814
Action is actively used within trading state management—do not remove the SET_LOADING constant.
The grep search confirms that the action is used in multiple places (middleware, reducers, and tests) in the trading module. Instead of deprecating it, the code should update the comment to document its role clearly.suite-common/trading/src/reducers/__fixtures__/buyTradingReducer.ts (1)
6-196: LGTM! Comprehensive test fixtures with realistic data.The fixtures cover all major actions with clear descriptions and realistic mock data. The test cases are well-structured and maintainable.
packages/suite/src/middlewares/wallet/tradingMiddleware.ts (1)
3-7: LGTM! Imports are correctly organized.The imports are well-organized, with related trading utilities grouped together.
suite-common/trading/src/reducers/__fixtures__/account.ts (1)
184-424: Consider adding validation for transaction amounts.The transaction fixtures contain amounts that should be validated to ensure they're within reasonable ranges.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if transaction amounts are validated in the codebase. # Test: Search for amount validation logic rg -A 5 'amount.*validate|validate.*amount' --type tsLength of output: 2963
Validation for transaction amounts is already handled in the relevant processing logic.
The fixture file is used to supply static sample data for tests. We’ve confirmed that validation of transaction amounts is performed in other parts of the codebase—for example, in the API logic in
packages/connect/src/api/bitcoin/outputs.tsas well as in the UI components like in CoinControl (packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx). Since the fixtures are well-defined test inputs and the runtime validation occurs during transaction processing, there is no need to add additional validation logic directly within these fixtures.suite-common/trading/tsconfig.json (1)
5-12: LGTM! Dependencies are properly organized.The references are correctly ordered and include all necessary dependencies for the trading module.
| // TODO: refactor Map and Set https://redux.js.org/style-guide/#do-not-put-non-serializable-values-in-state-or-actions | ||
| export interface BuyInfo { | ||
| buyInfo: Omit<BuyListResponse, 'defaultAmountsOfFiatCurrencies'> & { | ||
| defaultAmountsOfFiatCurrencies: TradingFiatCurrenciesProps; | ||
| }; | ||
| providerInfos: { [name: string]: BuyProviderInfo }; | ||
| supportedFiatCurrencies: Set<string>; | ||
| supportedCryptoCurrencies: Set<CryptoId>; | ||
| } |
There was a problem hiding this comment.
Address TODO: Replace Set with serializable alternatives.
The use of Set for currency collections violates Redux's serialization requirements. This could cause issues with state persistence and time-travel debugging.
Replace Set with arrays and consider using a selector for Set-like operations:
export interface BuyInfo {
buyInfo: Omit<BuyListResponse, 'defaultAmountsOfFiatCurrencies'> & {
defaultAmountsOfFiatCurrencies: TradingFiatCurrenciesProps;
};
providerInfos: { [name: string]: BuyProviderInfo };
- supportedFiatCurrencies: Set<string>;
- supportedCryptoCurrencies: Set<CryptoId>;
+ supportedFiatCurrencies: string[];
+ supportedCryptoCurrencies: CryptoId[];
}Add a selector for Set-like operations:
// selectors.ts
export const selectSupportedFiatCurrencies = (state: RootState) =>
new Set(state.trading.buy.buyInfo?.supportedFiatCurrencies);| }); | ||
| } else { | ||
| // special case: device no-backup permissions not granted | ||
| if (response.payload.code === 'Method_PermissionsNotGranted') return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Safely access response.payload.code to prevent undefined errors
To avoid potential runtime errors when response.payload is undefined, safely access response.payload.code using optional chaining.
Apply this diff to ensure safe access:
- if (response.payload.code === 'Method_PermissionsNotGranted') return;
+ if (response.payload?.code === 'Method_PermissionsNotGranted') return;📝 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.
| if (response.payload.code === 'Method_PermissionsNotGranted') return; | |
| if (response.payload?.code === 'Method_PermissionsNotGranted') return; |
| address = address ?? accountAddress.address; | ||
| path = path ?? accountAddress.path; | ||
| if (!path || !address) return; |
There was a problem hiding this comment.
Check for undefined accountAddress before accessing its properties
If getUnusedAddressFromAccount(account) returns undefined, accessing accountAddress.address or accountAddress.path may cause a runtime error. Ensure that accountAddress is defined before accessing its properties.
Apply this diff to prevent potential runtime errors:
const accountAddress = getUnusedAddressFromAccount(account);
+if (!accountAddress) return;
address = address ?? accountAddress.address;
path = path ?? accountAddress.path;📝 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.
| address = address ?? accountAddress.address; | |
| path = path ?? accountAddress.path; | |
| if (!path || !address) return; | |
| const accountAddress = getUnusedAddressFromAccount(account); | |
| if (!accountAddress) return; | |
| address = address ?? accountAddress.address; | |
| path = path ?? accountAddress.path; | |
| if (!path || !address) return; |
| const routeName = newState.router.route?.name; | ||
| const isBuy = routeName === 'wallet-trading-buy'; | ||
| const isSell = routeName === 'wallet-trading-sell'; | ||
| const isExchange = routeName === 'wallet-trading-exchange'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure that route and route.name are defined before accessing them
Accessing newState.router.route?.name and state.router.route?.name without checking if router.route is defined may lead to runtime errors if route is undefined. Add checks to handle cases where route may not be available.
Apply this diff to ensure safe access:
const routeName = newState.router.route?.name;
+if (!routeName) return;
const isBuy = routeName === 'wallet-trading-buy';
const isSell = routeName === 'wallet-trading-sell';
const isExchange = routeName === 'wallet-trading-exchange';📝 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.
| const routeName = newState.router.route?.name; | |
| const isBuy = routeName === 'wallet-trading-buy'; | |
| const isSell = routeName === 'wallet-trading-sell'; | |
| const isExchange = routeName === 'wallet-trading-exchange'; | |
| const routeName = newState.router.route?.name; | |
| if (!routeName) return; | |
| const isBuy = routeName === 'wallet-trading-buy'; | |
| const isSell = routeName === 'wallet-trading-sell'; | |
| const isExchange = routeName === 'wallet-trading-exchange'; |
| export const tradingMiddleware = createMiddleware(async (action, { dispatch, next, getState }) => { | ||
| const state = selectState(getState()); | ||
| const { isLoading, lastLoadedTimestamp } = state.wallet.trading; |
There was a problem hiding this comment.
Correct the import or usage of tradingActions to avoid confusion
The usage of tradingActions.tradingActions can be confusing and may lead to errors. Ensure that the import and usage of tradingActions are consistent.
Apply this diff to correct the import and usage:
- import * as tradingActions from '../actions/tradingActions';
+ import { tradingActions } from '../actions/tradingActions';Now, you can use tradingActions.setLoading(true) directly without redundancy.
Committable suggestion skipped: line range outside the PR's diff.
| const paymentMethods: TradingPaymentMethodListProps[] = [ | ||
| { | ||
| value: '', | ||
| label: '', | ||
| }, | ||
| { | ||
| value: 'creditCard', | ||
| label: 'Credit Card', | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add validation for payment method values.
The payment methods array includes an empty value/label pair which could cause issues in the UI.
🏁 Script executed:
#!/bin/bash
# Description: Check if empty payment methods are handled correctly in the UI.
# Test: Search for payment method handling in the UI components
rg -A 5 'paymentMethods.*map' --type tsLength of output: 41
I'll generate a new shell script to search for all references to paymentMethods across the repository (with context) so we can better understand how and where these values (including the empty ones) are used.
🏁 Script executed:
#!/bin/bash
# Searching for all occurrences of paymentMethods with context to see its usage across the codebase:
rg -i -C 5 'paymentMethods'Length of output: 64889
Action: Remove or Validate Empty Payment Method Entry
Our investigation confirms that the fixture file
suite-common/trading/src/reducers/__fixtures__/tradingReducer.ts
includes an entry with an empty value/label pair. When this fixture is used, no additional validation prevents an empty payment method from propagating into the UI. Although some UI logic (e.g., in the dropdown’s label formatter) conditionally skips rendering components for empty values, there is no explicit check preventing empty values from ever being set or used, which could lead to unexpected UI behavior.
Recommendations:
- Filter or Remove Empty Entries: Update the fixture (and any similar logic producing payment method lists) to filter out entries with empty strings for both
valueandlabel. - Add Validation Logic: Enhance the business logic (for example, in the hook or the component that builds the payment methods list) to validate that each payment method is a non-empty, valid option before it reaches the UI.
|
🚀 Expo preview is ready!
|
adderpositive
force-pushed
the
chore/move-buy-trading-state-in-common
branch
from
fffe3d1 to
d69d585
Compare
Description
Duplicated the functionality of the buy trading section.
Info
reduxjs-toolkitRelated Issue
Resolve #16518