refactor(trading): minor improvements #17097
Conversation
9cfe792 to
535f675
Compare
|
🚀 Expo preview is ready!
|
5021916 to
701f7a5
Compare
535f675 to
2262fce
Compare
701f7a5 to
8aa6308
Compare
2262fce to
4bbbd8e
Compare
4f95dde to
2e9a7c8
Compare
8aa6308 to
c5820cb
Compare
2e9a7c8 to
476426c
Compare
c5820cb to
d9c6be9
Compare
suite-common/trading/src/middlewares/__tests__/tradingMiddleware.test.ts
Show resolved
Hide resolved
suite-common/trading/src/middlewares/__tests__/tradingMiddleware.test.ts
Show resolved
Hide resolved
a3910c3 to
f64d3d8
Compare
f64d3d8 to
ec3c1b2
Compare
WalkthroughThe changes introduce two new selectors to the extraDependencies module to retrieve the selected account and trading environment from the application state. Corresponding updates were made to the type definitions and mock implementations to support these selectors. In addition, multiple test suites across trading reducers, thunks, and utilities were updated to improve the clarity of their descriptions without altering functionality. Key modifications in the trading middleware include renaming and restructuring to incorporate extra dependencies, while some legacy trading selectors were removed. Minor refactoring in trading thunks improved variable naming for better clarity. Finally, configuration adjustments in native modules involved adding or removing dependency references and updating TypeScript configuration paths related to the trading module. ✨ Finishing Touches
🪧 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 (1)
suite-common/trading/src/middlewares/tradingMiddleware.ts (1)
21-40: Refactored middleware to use dependency injection pattern.The middleware has been updated to use
createMiddlewareWithExtraDepsand access selectors through the injectedextraparameter. This architectural change:
- Improves testability by making dependencies explicit and injectable
- Makes the code more modular and flexible
- Aligns with modern Redux patterns for dependency management
The selectors
selectSelectedAccountandselectTradingEnvironmentare now accessed through the extra dependencies rather than directly imported.
📜 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 (23)
packages/suite/src/support/extraDependencies.ts(1 hunks)suite-common/redux-utils/src/extraDependenciesType.ts(1 hunks)suite-common/test-utils/src/extraDependenciesMock.ts(2 hunks)suite-common/trading/src/__tests__/utils.test.ts(9 hunks)suite-common/trading/src/middlewares/__tests__/tradingMiddleware.test.ts(5 hunks)suite-common/trading/src/middlewares/tradingMiddleware.ts(2 hunks)suite-common/trading/src/reducers/__fixtures__/buyTradingReducer.ts(11 hunks)suite-common/trading/src/reducers/__fixtures__/tradingReducer.ts(13 hunks)suite-common/trading/src/reducers/__tests__/buyReducer.test.ts(1 hunks)suite-common/trading/src/selectors/tradingSelectors.ts(0 hunks)suite-common/trading/src/thunks/__tests__/tradingThunks.test.ts(6 hunks)suite-common/trading/src/thunks/buy/__tests__/confirmTradeThunk.test.ts(1 hunks)suite-common/trading/src/thunks/buy/__tests__/handleRequestThunk.test.ts(5 hunks)suite-common/trading/src/thunks/buy/__tests__/loadInfoThunk.test.ts(3 hunks)suite-common/trading/src/thunks/buy/__tests__/selectQuoteThunk.test.ts(9 hunks)suite-common/trading/src/thunks/buy/loadInfoThunk.ts(1 hunks)suite-common/trading/src/thunks/tradingThunks.ts(1 hunks)suite-common/trading/src/utils/buy/__tests__/buyUtils.test.ts(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/tsconfig.json(1 hunks)suite-native/state/package.json(0 hunks)suite-native/state/src/store.ts(0 hunks)suite-native/state/tsconfig.json(0 hunks)
💤 Files with no reviewable changes (4)
- suite-native/state/package.json
- suite-native/state/src/store.ts
- suite-native/state/tsconfig.json
- suite-common/trading/src/selectors/tradingSelectors.ts
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- 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: EAS Update
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
🔇 Additional comments (53)
suite-native/module-trading/package.json (1)
16-16: New Dependency Added: "@suite-common/trading"The addition of the dependency
"@suite-common/trading": "workspace:*"is implemented correctly and aligns with the refactoring objectives. This dependency is now explicitly referenced in the trading module, and it’s important to ensure that its integration does not conflict with the recent removal of the same dependency from the@suite-native/statemodule. Please verify that any usage or bridging logic between these modules is updated accordingly.suite-native/module-trading/tsconfig.json (2)
5-7: Clarify the New Trading Dependency ReferenceThe new reference
{ "path": "../../suite-common/trading" }was added to integrate the trading functionalities through the common trading package. Please verify that the relative path is correct in your project structure and that the types or modules from the trading package are correctly imported elsewhere in the code.
8-12: Review Remaining References for ConsistencyThe current list of references (to navigation, test-utils, blockchain-link-types, and utils) appears consistent with the previous configuration. Ensure that adding the trading dependency does not introduce any conflicts with these. Overall, the JSON structure adheres to best practices for TypeScript configuration.
suite-common/test-utils/src/extraDependenciesMock.ts (3)
6-6: Good update to the import statement.Adding
SelectedAccountLoadedto the imports aligns with the new selectors being added below.
101-104: Good addition of the selectSelectedAccount selector.This new mock selector implementation properly provides a default selected account for tests, which will help with testing components that depend on this selector.
110-110: Good addition of the selectTradingEnvironment selector.This mock implementation provides a consistent 'localhost' environment for testing trading functionality.
suite-common/trading/src/reducers/__tests__/buyReducer.test.ts (1)
8-8: Improved test suite description.Renaming from the generic "Testing buy trading reducer" to the more specific "tradingBuyReducer" improves clarity and follows a more consistent naming convention.
suite-common/trading/src/thunks/buy/__tests__/confirmTradeThunk.test.ts (1)
237-237: Enhanced test description clarity.Adding "successful" to the test description makes it more explicit that this test is verifying the happy path scenario when the API response succeeds. This clarification is valuable for developers reading the test.
suite-common/trading/src/utils/buy/__tests__/buyUtils.test.ts (1)
4-5: Improved test naming conventions.Changing from the generic "testing buy utils" to the specific function name "getAmountLimits" and improving the test description to "should test all scenarios" makes the test suite purpose much clearer. This follows best practices for test naming.
suite-common/trading/src/thunks/buy/loadInfoThunk.ts (2)
30-30: Variable naming improvement.Renaming
etoproviderenhances code readability by using a more descriptive variable name that clearly communicates the purpose and content of the variable.
36-37: Variable naming improvement.Renaming
ctocurrencyimproves code clarity by using a descriptive name that properly identifies what the variable represents.suite-common/trading/src/reducers/__fixtures__/buyTradingReducer.ts (1)
79-235: Test description improvements throughout the file.The updated test description format that uses "should" statements (e.g., "should save buy info" instead of "test saving buy info") follows best practices for test descriptions. This makes the test intentions clearer and more consistent.
suite-common/trading/src/thunks/buy/__tests__/loadInfoThunk.test.ts (2)
12-12: Simplified describe block title.The describe block title has been simplified to just "loadInfoThunk" which follows clean testing conventions by avoiding redundant words like "Testing".
36-36: Improved test case descriptions.The test descriptions now clearly state the expected behavior using "should" statements, which is more informative and follows modern testing practices.
Also applies to: 63-63
suite-common/trading/src/thunks/tradingThunks.ts (1)
35-35: Updated selector reference.The change from
selectTradingSettingAddressDisplayTypetoextra.selectors.selectAddressDisplayTypereflects an architectural shift in how selectors are accessed. This modification is part of the broader refactoring mentioned in the PR objectives and maintains the same functionality while improving code organization.packages/suite/src/support/extraDependencies.ts (2)
94-94: LGTM - Implementation looks correct.Adding a selector for the selected account provides a clean way to access this data through the extraDependencies interface. This is consistent with the pattern used for other wallet-related selectors in this file.
98-99: LGTM - Trading environment selector.This selector for the trading environment provides a clear way to access the Invity server configuration from debug settings. As noted in previous discussions, keeping this in
settings.debugmakes sense for the current architecture.suite-common/redux-utils/src/extraDependenciesType.ts (2)
87-87: LGTM - Type definition for selectSelectedAccount.The type definition appropriately uses
SuiteCompatibleSelectorwith theSelectedAccountStatustype, ensuring type safety when using this selector.
92-94: LGTM - Well-defined type for trading environment.Good work using a precise union type for the trading environment values, which clearly documents all possible values and improves type safety.
suite-common/trading/src/reducers/__fixtures__/tradingReducer.ts (1)
127-127: Improved test descriptions with "should" pattern.The test description updates throughout the file provide clearer expectations for each test case by following the "should" pattern. This is a good practice for making tests more readable and self-documenting.
Also applies to: 137-137, 150-150, 164-164, 181-181, 195-195, 212-212, 226-226, 244-244, 258-258, 276-276, 293-293, 307-307
suite-common/trading/src/__tests__/utils.test.ts (1)
24-36: Enhanced test organization and descriptions.The refactoring of test descriptions and organization follows best practices:
- Individual
describeblocks for each function being tested- "should" phrasing for test cases that clearly states expectations
- Consistent structure throughout the test file
This improves readability and maintainability without changing test behavior.
Also applies to: 38-45, 47-78, 80-93, 95-108, 110-129, 131-150, 152-167, 169-181, 183-215
suite-common/trading/src/thunks/buy/__tests__/selectQuoteThunk.test.ts (10)
135-135: Improved test description for better clarity.The test description has been updated to follow a more conventional "should..." format, which enhances readability and clearly communicates the test's purpose.
159-160: Enhanced test suite and case descriptions.Both the describe block and test case have been updated to follow a clearer naming convention using "should" and "when" prefixes, making the test intentions more explicit.
188-188: Improved test description clarity.Test description now clearly states the condition being tested with the "when" prefix.
216-216: Better test case naming.Updated test description to clearly indicate the scenario being tested.
245-245: Enhanced test description.Test description now more clearly articulates the condition being tested.
274-297: Improved test description and formatting.The test case has been updated to follow the "when" convention, improving clarity of the test purpose.
301-302: Better describe block and test case naming.The describe block and test case descriptions have been updated to better reflect their purpose following the "should"/"when" convention.
346-346: Improved test description.Test description now clearly indicates the specific condition being tested.
381-381: Enhanced test case description.Updated to follow the "when" convention for better test suite readability.
417-417: Clearer test case naming.Test description now follows the consistent pattern with other tests in the suite.
suite-common/trading/src/thunks/__tests__/tradingThunks.test.ts (7)
28-28: Renamed test suite for better specificity.The test suite name has been changed to focus specifically on the "verifyAddressThunk" functionality rather than using a generic name, which improves clarity about what's being tested.
33-33: Improved test description.Test description now clearly communicates the expected outcome using the "should" convention.
77-77: Enhanced test case description.Updated description to follow the "should" convention, making the test's purpose more explicit.
111-111: More descriptive test case name.The test description has been updated to clearly indicate the specific condition being tested.
154-154: Improved test description clarity.Updated to provide more context about what the test is verifying, including both the expected behavior and the condition.
202-202: Better test case naming.Test description now clearly communicates both the condition and expected behavior.
250-250: Enhanced test case description.Updated to provide a more specific description of the failure condition being tested.
suite-common/trading/src/middlewares/tradingMiddleware.ts (3)
1-1: Updated imports to support dependency injection pattern.The import has been changed to use the dependency injection pattern with
createMiddlewareWithExtraDepsinstead of a regular middleware creation approach, improving testability and modularity.
15-16: Updated function signature to accept extra dependencies.The
getAccountAccordingToSectionfunction now accepts anextraparameter and uses the injected selectorselectSelectedAccountinstead of directly using a selector from the trading module. This change improves modularity and aligns with the dependency injection pattern.
44-64: Core functionality maintained while leveraging new architecture.While the middleware has been refactored to use the dependency injection pattern, the core functionality and logic flow remain intact. The middleware still handles loading Invity data based on account changes and time thresholds, maintaining the same business logic with an improved architecture.
suite-common/trading/src/thunks/buy/__tests__/handleRequestThunk.test.ts (5)
17-17: Simplified test suite name.The test suite description has been updated to be more concise while still clearly indicating what's being tested.
122-122: Improved test description clarity.Test description now follows the "should" convention and clearly states the expected outcome.
149-149: Enhanced test description and added explicit verification.
- The test description has been updated to clearly state the expected behavior
- A new assertion has been added to explicitly verify that no quotes are saved (quotes.length = 0) in this failure case, strengthening the test
This change improves both readability and test coverage.
Also applies to: 167-167
171-171: Improved test description and verification.
- The test description has been updated to follow the "should" convention
- A new assertion has been added to explicitly verify that quotes.length = 0 in this failure scenario
These changes make the test intent clearer and improve verification coverage.
Also applies to: 188-188
192-192: Enhanced test description.Updated test description to clearly indicate both the condition and expected behavior.
suite-common/trading/src/middlewares/__tests__/tradingMiddleware.test.ts (7)
1-1: Good choice using Redux Toolkit's standard createReducerImporting
createReducerdirectly from Redux Toolkit aligns with recommended patterns and simplifies the reducer creation approach.
29-35: Nice simplification of test reducerReplacing
createReducerWithExtraDepswith the standard Redux ToolkitcreateReducersimplifies the test code.The previous review noted "if we are not using reducer with extra deps here, we probably dont need extra deps?" - this change addresses that comment by using the standard reducer creation approach.
149-149: Improved test suite descriptionThe simplified test suite description enhances readability.
154-156: Clear test descriptionThe updated test description "should update when account is changed" is more descriptive and clearly communicates the test's purpose.
158-160: Improved test clarityThe updated description for outdated data checks is clearer than the previous version.
162-184: Clear test description for unchanged dataThe updated test description about keeping the same data version without updates clearly describes the test's purpose.
48-54:❓ Verification inconclusive
Consider whether all extra dependencies are necessary
The middleware is now prepared with a configuration that includes all extra dependencies plus specific selectors.
Previous review comments questioned if all these extra dependencies are necessary. Consider if you could further optimize by only including the dependencies actually used by the middleware.
🏁 Script executed:
#!/bin/bash # Search for usages of extraDependenciesMock in the trading middleware implementation echo "Checking where extraDependenciesMock is used in the trading middleware..." rg "extraDependenciesMock" ./suite-common/trading/src/middlewares/ -A 2 -B 2 --no-filenameLength of output: 855
Dependency Optimization Check for Trading Middleware
After verifying the usage ofextraDependenciesMockin the trading middleware, it’s clear that it’s being spread into both the middleware and the reducer. Please review whether all properties withinextraDependenciesMockare actually consumed byprepareTradingMiddleware. If certain dependencies aren’t used, consider refactoring to pass only the necessary ones. This could help simplify the configuration and reduce potential coupling.
- Confirm which properties from
extraDependenciesMockare essential for the middleware.- If possible, narrow down the dependencies to only those used in the middleware logic.
ec3c1b2 to
1e06636
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-common/trading/src/middlewares/tradingMiddleware.ts (1)
15-16: Function signature updated to accept external dependenciesThe function now accepts an
extraparameter containing selectors, which is a more flexible approach than directly importing selectors in the file.Consider adding JSDoc comments to clarify the purpose of this function and its parameters, especially explaining what the
ExtraDependenciescontain.
📜 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 (23)
packages/suite/src/support/extraDependencies.ts(1 hunks)suite-common/redux-utils/src/extraDependenciesType.ts(1 hunks)suite-common/test-utils/src/extraDependenciesMock.ts(2 hunks)suite-common/trading/src/__tests__/utils.test.ts(9 hunks)suite-common/trading/src/middlewares/__tests__/tradingMiddleware.test.ts(5 hunks)suite-common/trading/src/middlewares/tradingMiddleware.ts(2 hunks)suite-common/trading/src/reducers/__fixtures__/buyTradingReducer.ts(11 hunks)suite-common/trading/src/reducers/__fixtures__/tradingReducer.ts(13 hunks)suite-common/trading/src/reducers/__tests__/buyReducer.test.ts(1 hunks)suite-common/trading/src/selectors/tradingSelectors.ts(0 hunks)suite-common/trading/src/thunks/__tests__/tradingThunks.test.ts(6 hunks)suite-common/trading/src/thunks/buy/__tests__/confirmTradeThunk.test.ts(1 hunks)suite-common/trading/src/thunks/buy/__tests__/handleRequestThunk.test.ts(5 hunks)suite-common/trading/src/thunks/buy/__tests__/loadInfoThunk.test.ts(3 hunks)suite-common/trading/src/thunks/buy/__tests__/selectQuoteThunk.test.ts(9 hunks)suite-common/trading/src/thunks/buy/loadInfoThunk.ts(1 hunks)suite-common/trading/src/thunks/tradingThunks.ts(1 hunks)suite-common/trading/src/utils/buy/__tests__/buyUtils.test.ts(1 hunks)suite-native/module-trading/package.json(1 hunks)suite-native/module-trading/tsconfig.json(1 hunks)suite-native/state/package.json(0 hunks)suite-native/state/src/store.ts(0 hunks)suite-native/state/tsconfig.json(0 hunks)
💤 Files with no reviewable changes (4)
- suite-native/state/package.json
- suite-native/state/tsconfig.json
- suite-native/state/src/store.ts
- suite-common/trading/src/selectors/tradingSelectors.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- suite-common/trading/src/reducers/tests/buyReducer.test.ts
- suite-native/module-trading/tsconfig.json
- suite-common/trading/src/thunks/buy/tests/confirmTradeThunk.test.ts
- suite-common/trading/src/thunks/tradingThunks.ts
- suite-common/redux-utils/src/extraDependenciesType.ts
- suite-common/trading/src/thunks/buy/tests/loadInfoThunk.test.ts
- suite-native/module-trading/package.json
- suite-common/trading/src/thunks/buy/loadInfoThunk.ts
- suite-common/trading/src/utils/buy/tests/buyUtils.test.ts
- packages/suite/src/support/extraDependencies.ts
- suite-common/trading/src/reducers/fixtures/tradingReducer.ts
- suite-common/trading/src/thunks/buy/tests/selectQuoteThunk.test.ts
- suite-common/test-utils/src/extraDependenciesMock.ts
- suite-common/trading/src/reducers/fixtures/buyTradingReducer.ts
- suite-common/trading/src/tests/utils.test.ts
- suite-common/trading/src/thunks/buy/tests/handleRequestThunk.test.ts
- suite-common/trading/src/thunks/tests/tradingThunks.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (14)
- 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-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (15)
suite-common/trading/src/middlewares/tradingMiddleware.ts (5)
1-1: Import dependency update to useExtraDependenciespatternThe middleware now uses
createMiddlewareWithExtraDepsinstead of the regular middleware creation pattern, enabling injection of external dependencies and selectors.
21-22: Middleware function renamed and restructuredThe middleware is now prepared using
createMiddlewareWithExtraDeps, which supports dependency injection through theextraparameter, making the module more testable and maintainable.
25-27: Updated account retrieval patternThe middleware now gets the account through the external dependencies selector rather than directly importing the selector function, aligning with the dependency injection pattern.
38-42: Updated environment selection using external dependenciesThe trading environment is now retrieved from the injected selectors rather than imported directly, consistent with the new dependency injection pattern.
67-69: Updated middleware export patternThe middleware is now exported as a factory function that returns the configured middleware with dependencies, rather than as a direct middleware instance.
suite-common/trading/src/middlewares/__tests__/tradingMiddleware.test.ts (10)
1-1: Updated imports to use regular Redux reducersThe code now uses standard Redux
createReducerinstead of the extra dependencies version, which is appropriate for test mocks where dependency injection isn't needed.
17-17: Updated middleware import to match new implementationThe import has been updated to use the renamed middleware factory function
prepareTradingMiddleware, aligning with the changes in the implementation file.
29-35: Simplified reducer mock implementationThe mock reducer now uses standard Redux
createReducerrather than the extra dependencies version, simplifying the test setup.
37-46: Updated suite reducer implementationSimilar to the account reducer, the suite reducer now uses the standard Redux
createReducer, maintaining consistency in the test approach.
48-54: Middleware initialization with mock dependenciesThe middleware is now initialized with mock selectors, providing a clean way to test it without complex state setup.
63-63: Updated reducer references in store configurationThe test store configuration has been updated to match the changed reducer structure, maintaining the test's ability to verify middleware behavior.
Also applies to: 65-65
149-149: More concise test suite descriptionThe test suite description has been simplified to "tradingMiddleware", which is clearer and more direct.
154-156: Improved test case descriptionTest description is now more descriptive and focuses on the expected behavior rather than implementation details.
158-160: Improved outdated data test descriptionThe test description now clearly states that it tests data updates when data is outdated, improving test readability.
162-184: Improved unchanged data test descriptionThe test now has a clearer description that states it verifies that data isn't updated unnecessarily, making the test purpose more obvious.
Description
Related Issue
Partially resolve #17049