chore(trading): move handle change and common parts #16958
Conversation
adderpositive
requested review from
vytick,
jbazant,
izmy,
LukasRada,
FreeWall and
tomasklim
|
🚀 Expo preview is ready!
|
| @@ -57,11 +59,10 @@ export const useTradingBuyForm = ({ | |||
| const dispatch = useDispatch(); | |||
| const { addressVerified, buyInfo, isFromRedirect, quotes, quotesRequest, selectedQuote } = | |||
| useSelector(state => state.wallet.trading.buy); | |||
| const paymentMethods = useSelector(state => state.wallet.trading.info.paymentMethods); | |||
There was a problem hiding this comment.
issue (minor): Accdording to our style guide you should create named selector and do not access state directly. (multiple occurrences in multiple files)
There was a problem hiding this comment.
Will be replaced soon.
adderpositive
force-pushed
the
test/trading-common
branch
from
45c5aab to
5b0f0d5
Compare
adderpositive
force-pushed
the
chore/move-buy-handle-change
branch
from
a4ed15e to
16bcce8
Compare
| async (_, { fulfillWithValue }) => { | ||
| export const loadInfoThunk = createThunk<BuyInfo>( | ||
| `${BUY_THUNK_COMMON_PREFIX}/loadInfo`, | ||
| async () => { |
There was a problem hiding this comment.
Check this change: removing fulfillWithValue
There was a problem hiding this comment.
yes, please, this has changed in previous PR
| return request; | ||
| }; | ||
|
|
||
| export type HandleRequestThunk = { |
There was a problem hiding this comment.
| export type HandleRequestThunk = { | |
| export type HandleRequestThunkProps = { |
adderpositive
force-pushed
the
test/trading-common
branch
2 times, most recently
from
d38b715 to
8cafd93
Compare
adderpositive
force-pushed
the
chore/move-buy-handle-change
branch
2 times, most recently
from
f053e5b to
6b23943
Compare
adderpositive
force-pushed
the
chore/move-buy-handle-change
branch
from
6b23943 to
765fd1f
Compare
WalkthroughThe pull request refactors trading functionality across multiple modules. It replaces localized hooks with centralized utility functions for retrieving trading quotes and payment methods, shifting from the custom ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
suite-common/trading/src/__fixtures__/buyUtils.ts (1)
38-50:⚠️ Potential issueAdd missing payment method field.
The third quote object is missing the
paymentMethodfield, which could cause issues in the UI or payment processing logic.Apply this diff to add the missing field:
maxFiat: 17044, minCrypto: 0.00415525, maxCrypto: 1.66210137, + paymentMethod: 'creditCard',
🧹 Nitpick comments (15)
suite-common/trading/src/selectors/tradingSelectors.ts (1)
88-92: Add error handling for missing coin symbols.The function could return undefined if the coin is not found or the symbol is missing. Consider adding a default value or error handling to ensure consistent behavior.
export const cryptoIdToCoinSymbol = (state: TradingRootState, cryptoId: CryptoId) => { const { coins = {} } = state.wallet.trading.info; - return coins[cryptoId]?.symbol?.toUpperCase(); + const symbol = coins[cryptoId]?.symbol; + if (!symbol) { + return undefined; + } + return symbol.toUpperCase(); };packages/suite/src/hooks/wallet/trading/form/useTradingSellForm.ts (3)
94-94: Consider memoizing the selector.Direct Redux state access could trigger unnecessary re-renders if the payment methods array reference changes.
-const paymentMethods = useSelector(state => state.wallet.trading.info.paymentMethods); +const paymentMethods = useSelector( + useCallback( + state => state.wallet.trading.info.paymentMethods, + [] + ) +);
371-372: Replace console.log with proper error logging.Consider using a dedicated logging service or error tracking system for production environments.
-console.log(`[doSellTrade] ${response.trade.error}`); +logger.error('[doSellTrade]', { error: response.trade.error });
409-410: Replace console.log with proper error logging.Similar to the previous comment, use a dedicated logging service for consistency.
-console.log(`[doSellTrade] ${errorMessage}`); +logger.error('[doSellTrade]', { error: errorMessage });suite-common/trading/src/types.ts (3)
93-103: Add documentation for the shouldTryToFetch property.The type is well-structured, but the purpose of the
shouldTryToFetchproperty is not immediately clear. Consider adding a JSDoc comment to explain its usage.Add documentation like this:
export type TradingCryptoSelectItemProps = { badge?: ReactNode; symbol: NetworkSymbolExtended; cryptoName?: string; coingeckoId?: string; contractAddress: string | null; + /** Indicates whether the component should attempt to fetch additional data for this crypto item */ shouldTryToFetch?: boolean; value: CryptoId; label: string; ticker?: string; };
105-116: Add documentation for the TradingBuyFormProps type.The type structure is good, but adding JSDoc comments would improve clarity, especially for the
amountInCryptoproperty.Add documentation like this:
+/** + * Props for the trading buy form component + */ export type TradingBuyFormProps = { + /** The amount in fiat currency */ fiatInput?: string; + /** The amount in cryptocurrency */ cryptoInput?: string; currencySelect: TradingOption; cryptoSelect: TradingCryptoSelectItemProps; countrySelect: TradingOption; paymentMethod?: TradingPaymentMethodListProps; + /** Indicates whether the input amount is in cryptocurrency (true) or fiat currency (false) */ amountInCrypto: boolean; };
117-126: Consider using a numeric type for balance.The interface is well-structured, but consider using a more specific type for the
balanceproperty instead ofstring. This could help prevent potential type coercion issues and ensure numeric operations are handled correctly.Consider one of these alternatives:
- balance: string; + balance: number;Or if decimal precision is important:
- balance: string; + balance: bigint;Or if you need to handle both decimal strings and numbers:
- balance: string; + balance: string | number;suite-common/trading/src/utils/buy/buyUtils.ts (3)
24-26: Consider using named constants for magic numbers.The number
1e28used as an initial value forminAmountshould be defined as a named constant to improve code readability and maintainability.+const MAX_SAFE_INITIAL_MIN_AMOUNT = 1e28; +const MIN_SAFE_INITIAL_MAX_AMOUNT = 0; let minAmount: number | undefined; let maxAmount: number | undefined;
27-49: Consider simplifying nested conditions.The logic for handling crypto and fiat amounts follows similar patterns and could be simplified to reduce code duplication.
-for (const quote of quotes) { - if (!quote.error) { - return; - } - if (request.wantCrypto) { - const amount = Number(quote.receiveStringAmount); - if (amount && quote.minCrypto && amount < quote.minCrypto) { - minAmount = Math.min(minAmount || 1e28, quote.minCrypto); - } - if (amount && quote.maxCrypto && amount > quote.maxCrypto) { - maxAmount = Math.max(maxAmount || 0, quote.maxCrypto); - } - } else { - const amount = Number(quote.fiatStringAmount); - if (amount && quote.minFiat && amount < quote.minFiat) { - minAmount = Math.min(minAmount || 1e28, quote.minFiat); - } - if (amount && quote.maxFiat && amount > quote.maxFiat) { - maxAmount = Math.max(maxAmount || 0, quote.maxFiat); - } - } -} +const getQuoteValues = (quote: BuyTrade, wantCrypto: boolean) => ({ + amount: Number(wantCrypto ? quote.receiveStringAmount : quote.fiatStringAmount), + min: wantCrypto ? quote.minCrypto : quote.minFiat, + max: wantCrypto ? quote.maxCrypto : quote.maxFiat, +}); + +for (const quote of quotes) { + if (!quote.error) return; + + const { amount, min, max } = getQuoteValues(quote, request.wantCrypto); + + if (amount && min && amount < min) { + minAmount = Math.min(minAmount || MAX_SAFE_INITIAL_MIN_AMOUNT, min); + } + if (amount && max && amount > max) { + maxAmount = Math.max(maxAmount || MIN_SAFE_INITIAL_MAX_AMOUNT, max); + } +}
18-23: Add JSDoc comments to document edge cases.The function would benefit from documentation explaining the behavior when all quotes have errors and when limits are undefined.
+/** + * Calculates amount limits based on trade quotes. + * @returns undefined if at least one quote succeeds, + * otherwise returns the applicable min/max limits + * based on all failed quotes. + */ const getAmountLimits = ({ request, quotes, currency, }: GetAmountLimitsProps): AmountLimitProps | undefined => {suite-common/trading/src/utils/buy/__tests__/buyUtils.test.ts (1)
4-77: Add test cases for edge scenarios.While the current test coverage is good, consider adding the following test cases:
- Empty quotes array
- Mixed quotes (some successful, some with errors)
describe('testing buy utils', () => { it('testing getAmountLimits function', () => { + it('should handle empty quotes array', () => { + expect( + buyUtils.getAmountLimits({ + request: fixtures.QUOTE_REQUEST_FIAT, + quotes: [], + currency: 'bitcoin', + }), + ).toBe(undefined); + }); + + it('should handle mixed success/error quotes', () => { + const mixedQuotes = [ + { ...fixtures.MIN_MAX_QUOTES_OK[0], error: undefined }, + ...fixtures.MIN_MAX_QUOTES_LOW, + ]; + expect( + buyUtils.getAmountLimits({ + request: fixtures.QUOTE_REQUEST_FIAT, + quotes: mixedQuotes, + currency: 'bitcoin', + }), + ).toBe(undefined); + });suite-common/trading/src/utils/buy/__fixtures__/buyUtils.ts (2)
201-201: Replace hardcoded dates with dynamic values.The
validUntilfields contain hardcoded dates ('2020-08-04T13:57:57.757Z'). This could make tests brittle over time.Consider using a relative timestamp or a mock date utility:
- validUntil: '2020-08-04T13:57:57.757Z', + validUntil: new Date(Date.now() + 3600000).toISOString(), // 1 hour from nowAlso applies to: 217-217
29-29: Standardize error message formats.The error messages across different quotes use inconsistent formats:
- Some use 'Amount too low/high'
- Others use 'Transaction amount too low/high'
- Currency formats vary (EUR 25 vs 25 EUR)
Consider standardizing the error message format for consistency.
Also applies to: 58-58, 76-76, 91-91, 123-123, 138-138, 152-152, 240-240, 256-256
suite-common/trading/src/utils.ts (2)
167-176: Document the default decimals value.The function uses a default value of 8 decimals. Please add a comment explaining why this specific value was chosen as the default.
178-210: Consider memoizing the utility functions.Both
getTradingPaymentMethodsandgetTradingQuotesByPaymentMethodperform filtering operations that could benefit from memoization to avoid unnecessary recalculations when the inputs haven't changed.Example implementation using
useMemo:+import { useMemo } from 'react'; -export const getTradingPaymentMethods = <T extends TradingTradeBuySellType>( +export const useTradingPaymentMethods = <T extends TradingTradeBuySellType>( quotes: TradingTradeMapProps[T][], ) => { + return useMemo(() => { const newPaymentMethods: TradingPaymentMethodListProps[] = []; quotes.forEach(quote => { // ... existing implementation }); return newPaymentMethods; + }, [quotes]); }; -export const getTradingQuotesByPaymentMethod = <T extends TradingTradeBuySellType>( +export const useTradingQuotesByPaymentMethod = <T extends TradingTradeBuySellType>( quotes: TradingTradeMapProps[T][] | undefined, currentPaymentMethod: TradingPaymentMethodProps, ) => { + return useMemo(() => { if (!quotes) return; return quotes.filter( quote => quote.paymentMethod === currentPaymentMethod && quote.error === undefined, ); + }, [quotes, currentPaymentMethod]); };
📜 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 (26)
packages/suite/src/hooks/wallet/trading/form/useTradingBuyForm.tsx(5 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingPaymentMethod.tsx(0 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingSellForm.ts(4 hunks)packages/suite/src/utils/wallet/trading/__tests__/buyUtils.test.ts(1 hunks)packages/suite/src/utils/wallet/trading/buyUtils.ts(0 hunks)suite-common/trading/package.json(1 hunks)suite-common/trading/src/__fixtures__/buyUtils.ts(2 hunks)suite-common/trading/src/__tests__/utils.test.ts(2 hunks)suite-common/trading/src/actions/buyActions.ts(3 hunks)suite-common/trading/src/index.ts(1 hunks)suite-common/trading/src/middlewares/__tests__/tradingMiddleware.test.ts(1 hunks)suite-common/trading/src/middlewares/tradingMiddleware.ts(1 hunks)suite-common/trading/src/reducers/__fixtures__/buyTradingReducer.ts(4 hunks)suite-common/trading/src/reducers/buyReducer.ts(4 hunks)suite-common/trading/src/selectors/tradingSelectors.ts(3 hunks)suite-common/trading/src/thunks/buy/__tests__/handleRequestThunk.test.ts(1 hunks)suite-common/trading/src/thunks/buy/__tests__/loadInfoThunk.test.ts(3 hunks)suite-common/trading/src/thunks/buy/handleRequestThunk.ts(1 hunks)suite-common/trading/src/thunks/buy/index.ts(1 hunks)suite-common/trading/src/thunks/buy/loadInfoThunk.ts(1 hunks)suite-common/trading/src/types.ts(3 hunks)suite-common/trading/src/utils.ts(2 hunks)suite-common/trading/src/utils/buy/__fixtures__/buyUtils.ts(1 hunks)suite-common/trading/src/utils/buy/__tests__/buyUtils.test.ts(1 hunks)suite-common/trading/src/utils/buy/buyUtils.ts(1 hunks)suite-common/trading/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/suite/src/utils/wallet/trading/buyUtils.ts
- packages/suite/src/hooks/wallet/trading/form/useTradingPaymentMethod.tsx
✅ Files skipped from review due to trivial changes (2)
- suite-common/trading/src/middlewares/tests/tradingMiddleware.test.ts
- suite-common/trading/src/middlewares/tradingMiddleware.ts
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (39)
suite-common/trading/package.json (2)
21-21: Addition of@trezor/react-utilsDependency
The addition of"@trezor/react-utils": "workspace:*"supports the new trading functionalities by providing common utilities that can be leveraged across hooks and components. Please ensure that all relevant modules integrate and test this dependency consistently.
23-23: Update to React v18.2.0
Updating the React dependency to version18.2.0aligns with our move towards the latest React features and ensures compatibility with the refactored trading components. It is important to confirm that downstream components and hooks are fully compatible with the version bump.suite-common/trading/src/selectors/tradingSelectors.ts (2)
73-74: LGTM!The selector follows Redux best practices and maintains consistency with other selectors in the file.
85-86: LGTM!The selector follows Redux best practices and aligns with the PR's objective of centralizing payment method access.
suite-common/trading/src/reducers/__fixtures__/buyTradingReducer.ts (3)
4-4: LGTM! Initial state setup for loading and amount limits.The new imports and state properties align with the PR objectives to prepare the
isLoadingandamountLimitsstates in the Redux store.Also applies to: 15-16
71-78: Verify the amount limits test data.The test data shows
minCryptoequalsmaxCrypto(both 0.002). Please verify if this is intentional for testing edge cases.
209-236: LGTM! Comprehensive test coverage for new functionality.The new test cases for loading status and amount limits follow the established pattern and provide good coverage for the new functionality.
packages/suite/src/hooks/wallet/trading/form/useTradingSellForm.ts (4)
14-15: LGTM! Improved modularity with utility functions.The refactoring to use
getTradingPaymentMethodsandgetTradingQuotesByPaymentMethodfrom@suite-common/tradingimproves code organization and reusability.
169-172: LGTM! Improved quotes filtering.The use of
getTradingQuotesByPaymentMethodutility function provides a more maintainable way to filter quotes.
286-288: LGTM! Enhanced payment methods extraction.The use of
getTradingPaymentMethodsutility function provides a more consistent way to extract payment methods from quotes.
657-664: LGTM! Proper cleanup of AbortController.The cleanup effect ensures that pending requests are properly aborted when the component unmounts.
suite-common/trading/src/types.ts (1)
1-1: LGTM!The new imports are necessary for the type definitions and are correctly imported from their respective packages.
Also applies to: 19-19
suite-common/trading/src/index.ts (1)
5-5: LGTM! Export paths updated to reflect new structure.The new export paths align well with the PR's objective of reorganizing trading functionality. The changes maintain a clean separation of concerns by moving buy-related utilities and thunks to dedicated directories.
Also applies to: 11-11
packages/suite/src/utils/wallet/trading/__tests__/buyUtils.test.ts (1)
1-4:❓ Verification inconclusive
Verify test coverage for moved functionality.
The removal of
getAmountLimitstests is aligned with the code reorganization. However, let's verify that the test coverage is maintained in the new location.
🏁 Script executed:
#!/bin/bash # Description: Check if getAmountLimits tests exist in the new location # Search for getAmountLimits test coverage rg -l "getAmountLimits.*test" suite-common/trading/src/Length of output: 57
Manually verify test coverage for
getAmountLimitsfunctionalityThe recent reorganization removed the tests for
getAmountLimitsfrombuyUtils.test.ts. Our initial search insuite-common/trading/src/usingrg -l "getAmountLimits.*test"produced no results. Please confirm that tests covering thegetAmountLimitsfunctionality have indeed been relocated or added elsewhere. If they were renamed or moved to a different directory, update the test references accordingly. If not, ensure that appropriate tests are introduced to cover this functionality.suite-common/trading/src/__fixtures__/buyUtils.ts (2)
19-20: LGTM! Adding human-readable payment method names.The addition of
paymentMethodNameimproves clarity for UI display while maintaining the technicalpaymentMethodidentifier.
35-36:❓ Verification inconclusive
Verify Apple Pay integration.
The payment method has been changed to 'applePay'. Please ensure that:
- The Apple Pay integration is fully implemented
- The UI correctly handles this payment method
- The backend systems support Apple Pay transactions
🏁 Script executed:
#!/bin/bash # Search for Apple Pay related configurations and implementations rg -i "apple.?pay" -g "!{__fixtures__,__tests__}/*"Length of output: 6073
Action Required: Verify Complete Apple Pay Integration
The updated fixtures now reference
"applePay"and"Apple Pay"consistently. Searches indicate that this change is reflected across multiple fixtures, tests, and even some UI simulation files. However, please ensure that:
- The Apple Pay integration is fully implemented (end-to-end flow, error handling, etc.).
- UI components correctly display and handle the
"applePay"payment method.- Backend systems/processes support Apple Pay transactions as expected.
suite-common/trading/src/utils/buy/__fixtures__/buyUtils.ts (1)
21-259: LGTM! Comprehensive test fixtures.The test fixtures provide excellent coverage of various scenarios:
- Normal quotes (
MIN_MAX_QUOTES_OK)- Below minimum amount (
MIN_MAX_QUOTES_LOW)- Above maximum amount (
MIN_MAX_QUOTES_HIGH)- Alternative currency quotes (
ALTERNATIVE_QUOTES)- Empty amount handling (
EMPTY_AMOUNT_QUOTES)suite-common/trading/src/thunks/buy/handleRequestThunk.ts (4)
34-44: Consider potential concurrency implications when aborting requests.
Currently, the code aborts the previous request before initiating a new one, which may be desirable if only the latest request matters. However, if concurrent requests become necessary in the future, this logic could prematurely cancel valid requests.Do you plan to allow parallel requests at any point? If so, you might want to create separate controllers for each request instead of reusing one ref.
53-82: Returnundefinedearly if no amounts are provided.
Returningundefinedvends a clear signal that quotes shouldn’t be fetched when key fields are missing. This is a sound approach, but verify that other parts of the code handle anundefinedresult consistently to avoid potential NPEs or stale state.
84-91: Previous suggestion forHandleRequestThunkPropshas been adopted.
This matches the earlier feedback regarding the named export for the thunk props. Good job implementing this change.
93-162: Add error handling to manage potential request failures.
The call toinvityAPI.getBuyQuotescan throw an error if the API request fails or is interrupted. Consider wrapping the API call in a try/catch and dispatching an appropriate failure event, or using a global error handler, to prevent unhandled rejections.Do you have a centralized error boundary or global error handler for unexpected async errors in the thunk?
suite-common/trading/src/thunks/buy/index.ts (1)
1-9: Export structure looks good.
Aggregating the thunks underbuyThunkspromotes clean organization and discoverability.suite-common/trading/src/thunks/buy/loadInfoThunk.ts (3)
5-7: Import paths align well with the new folder structure.
This ensures consistency across your refactored trading code.
9-12: Exporting the thunk improves reusability.
MakingloadInfoThunkavailable for other modules is a sensible design choice, especially for testability and maintainability.
16-16: Safety check forbuyInfois a good defensive measure.
By ensuringbuyInfois not null or undefined, you avoid potential runtime errors when accessingproviders.suite-common/trading/src/actions/buyActions.ts (1)
52-61: LGTM! Well-structured action creators.The new action creators follow the established pattern and are properly typed. The implementation is clean and consistent with the existing codebase.
suite-common/trading/src/thunks/buy/__tests__/loadInfoThunk.test.ts (1)
6-10: LGTM! Improved test organization.The changes enhance readability through better test descriptions and maintain consistent import paths.
Also applies to: 14-15, 38-38, 65-65
suite-common/trading/src/reducers/buyReducer.ts (1)
28-29: LGTM! Well-integrated state management.The new state properties and reducer cases are properly integrated and maintain immutability through Redux Toolkit's builder pattern.
Also applies to: 42-43, 75-80
suite-common/trading/src/__tests__/utils.test.ts (3)
139-152: LGTM!The test case effectively verifies the deduplication of payment methods and validates the presence of specific payment methods.
154-164: LGTM!The test case thoroughly validates the quote filtering functionality and handles edge cases appropriately.
166-196: LGTM!The test case comprehensively validates network decimals retrieval, including default values and account options.
suite-common/trading/src/thunks/buy/__tests__/handleRequestThunk.test.ts (4)
121-146: LGTM!The test case effectively validates the successful response handling, including timer functions and state updates.
148-167: LGTM!The test case thoroughly validates error handling for incorrect inputs and state updates.
169-187: LGTM!The test case effectively validates error handling for incorrect crypto selection and state updates.
189-203: LGTM!The test case thoroughly validates error handling for empty API responses and state updates.
packages/suite/src/hooks/wallet/trading/form/useTradingBuyForm.tsx (3)
62-62: Create a named selector for accessing payment methods.Direct state access should be replaced with a named selector according to the style guide.
122-125: LGTM!The changes improve code organization by using centralized utility functions for payment method handling.
Also applies to: 229-230
236-240: LGTM!The changes improve code organization by using a centralized utility function for amount limits calculation.
suite-common/trading/tsconfig.json (1)
11-11: LGTM!The addition of react-utils reference is necessary to support the new utility functions.
| dispatch(tradingBuyActions.setAmountLimits(limits)); | ||
| dispatch(tradingBuyActions.saveQuotes(quotesSuccess)); | ||
| dispatch(tradingBuyActions.saveQuoteRequest(requestData)); | ||
| dispatch(tradingActions.savePaymentMethods(paymentMethodsFromQuotes)); | ||
| dispatch(tradingBuyActions.setIsLoading(false)); |
| import { buyUtils } from '../buyUtils'; | ||
|
|
||
| describe('testing buy utils', () => { | ||
| it('testing getAmountLimits function', () => { |
There was a problem hiding this comment.
nitpick (non-blocking): What do you think about restructuring this test to something like this?
it.each([
[fixtures.QUOTE_REQUEST_FIAT, fixtures.MIN_MAX_QUOTES_OK, undefined],
[fixtures.QUOTE_REQUEST_FIAT, fixtures.MIN_MAX_QUOTES_LOW],
// ...
])('testing getAmountLimits function case $#', (request, quotes, expectedResult) => {
expect(buyUtils.getAmountLimits({ request, quotes, currency }).toStrictEqual(expectedResult);
});
There was a problem hiding this comment.
updated here f59e124
| return sendCryptoSelect.decimals; | ||
| } | ||
|
|
||
| return network?.decimals ?? 8; |
There was a problem hiding this comment.
issue: Magic number. Introduce named constant or rebase on develop and use BASE_CRYPTO_MAX_DISPLAYED_DECIMALS please.
There was a problem hiding this comment.
I will use getNetwork('btc').decimals, would be better.
There was a problem hiding this comment.
updated here 88453d2
adderpositive
force-pushed
the
chore/move-buy-handle-change
branch
from
765fd1f to
d8a4124
Compare
adderpositive
force-pushed
the
chore/move-buy-handle-change
branch
from
d8a4124 to
30a32b7
Compare
| if (!quote.error) { | ||
| return; | ||
| } | ||
| if (request.wantCrypto) { |
There was a problem hiding this comment.
nit:
really weird name 😀 why it is not txType fiat | crypto
Description
isLoadingandamountLimitsin redux stategetAmountLimitsfunction for buy (also replaced)paymentMethodsto utils (also replaced)getTradingNetworkDecimals(not replaced for now)handleChangeand renamedRelated Issue
Resolve #16518