chore(trading): move useTradingInfo hook to @suite-common/trading #17450
Conversation
jbazant
added
the
no-project
WalkthroughThe changes refactor the source of the ✨ 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 (
|
![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
🧹 Nitpick comments (1)
suite-common/trading/src/types.ts (1)
119-133: Well-designed interface for the relocated hookThis comprehensive interface aligns perfectly with the PR objective of moving the
useTradingInfohook to@suite-common/trading. The methods provide all the necessary transformations for working with crypto IDs and building selection options.One suggestion would be to rename this to
TradingInfoHookPropsor similar to clearly indicate it represents a hook's return value.
📜 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 (4)
packages/suite/src/types/trading/trading.ts(3 hunks)suite-common/trading/package.json(1 hunks)suite-common/trading/src/hooks/useTradingInfo.ts(3 hunks)suite-common/trading/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- suite-common/trading/src/hooks/useTradingInfo.ts
- packages/suite/src/types/trading/trading.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: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (5)
suite-common/trading/package.json (2)
19-19: Addition of@trezor/address-validatorDependencyThe inclusion of
"@trezor/address-validator": "workspace:*"in the dependencies is appropriate for consolidating trading-related validations within the module. Ensure that its usage is documented and covered by tests in the module where it's utilized.
26-26: Addition ofreact-reduxDependencyAdding
"react-redux": "8.0.7"supports the Redux integrations needed by hooks such asuseTradingInfo. Verify that this version is compatible with the other parts of your codebase and that no conflicts arise within the larger monorepo.suite-common/trading/src/types.ts (3)
103-105: Good addition of discriminator property and contextual fieldsAdding the
type: 'currency'discriminator property is a good practice for union types. The optionalbalanceandnetworkNameproperties will improve the UI display capabilities of crypto assets.
108-113: Well-structured group interface with appropriate discriminatorThis interface provides a clear way to represent asset groups in selection components with all the necessary metadata. The discriminator pattern with
type: 'group'works well with the existingTradingCryptoSelectItemProps.
115-117: Good union type implementationCreating this union type allows for proper typing of components that need to handle both individual crypto assets and group headers, improving type safety when working with hierarchical data structures.
|
🚀 Expo preview is ready!
|
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13703131956 |
trezor-ci
force-pushed
the
refactor-useTradingInfo-to-suite-common
branch
from
4beb965 to
4501684
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-common/trading/src/types.ts (1)
119-133: Consider adding JSDoc comments to the interface methodsThe
TradingInfoPropsinterface is well-structured and comprehensive, defining all the functionality required for theuseTradingInfohook that's being moved in this PR. However, adding JSDoc comments to document the purpose and behavior of each method would improve maintainability.Example:
export interface TradingInfoProps { + /** + * Returns the platform name for a given crypto ID + * @param cryptoId - The crypto ID to look up + * @returns The platform name or undefined if not found + */ cryptoIdToPlatformName: (cryptoId: CryptoId) => string | undefined; // Add similar comments for other methods... }
📜 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 (34)
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TradingTermsModal.tsx(1 hunks)packages/suite/src/hooks/wallet/trading/form/common/useTradingFormActions.ts(1 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingBuyForm.tsx(1 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingBuyFormDefaultValues.tsx(1 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts(1 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingExchangeFormDefaultValues.ts(1 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingSellForm.ts(1 hunks)packages/suite/src/types/trading/trading.ts(3 hunks)packages/suite/src/types/trading/tradingForm.ts(1 hunks)packages/suite/src/utils/wallet/trading/__tests__/buyUtils.test.ts(1 hunks)packages/suite/src/utils/wallet/trading/__tests__/exchangeUtils.test.ts(2 hunks)packages/suite/src/utils/wallet/trading/__tests__/sellUtils.test.ts(2 hunks)packages/suite/src/views/wallet/trading/common/TradingAddressOptions.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingCryptoAmount.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingFeaturedOffers/TradingFeaturedOffersAmounts.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInput/TradingFormInputCryptoSelect.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInput/TradingFormInputFiatCrypto/TradingFormInputCryptoAmount.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInput/TradingFormInputFiatCrypto/TradingFormInputFiatCrypto.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOffer.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferCryptoAmount.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingInfo/TradingInfoHeader.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendSwap.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingVerify/TradingVerify.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingVerify/TradingVerifyOptions.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingVerify/TradingVerifyOptionsItem.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingTransactions/TradingTransaction/TradingTransactionAmounts.tsx(1 hunks)suite-common/trading/package.json(1 hunks)suite-common/trading/redux.d.ts(1 hunks)suite-common/trading/src/hooks/useTradingInfo.ts(3 hunks)suite-common/trading/src/index.ts(1 hunks)suite-common/trading/src/thunks/buy/__tests__/handleRequestThunk.test.ts(1 hunks)suite-common/trading/src/types.ts(1 hunks)suite-common/trading/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (33)
- suite-common/trading/tsconfig.json
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TradingTermsModal.tsx
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingVerify/TradingVerifyOptionsItem.tsx
- suite-common/trading/src/thunks/buy/tests/handleRequestThunk.test.ts
- packages/suite/src/hooks/wallet/trading/form/useTradingExchangeFormDefaultValues.ts
- suite-common/trading/src/index.ts
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferCryptoAmount.tsx
- packages/suite/src/views/wallet/trading/common/TradingCryptoAmount.tsx
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInput/TradingFormInputCryptoSelect.tsx
- suite-common/trading/redux.d.ts
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInput/TradingFormInputFiatCrypto/TradingFormInputCryptoAmount.tsx
- packages/suite/src/hooks/wallet/trading/form/common/useTradingFormActions.ts
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOffer.tsx
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx
- suite-common/trading/package.json
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingVerify/TradingVerifyOptions.tsx
- suite-common/trading/src/hooks/useTradingInfo.ts
- packages/suite/src/hooks/wallet/trading/form/useTradingBuyFormDefaultValues.tsx
- packages/suite/src/types/trading/tradingForm.ts
- packages/suite/src/utils/wallet/trading/tests/exchangeUtils.test.ts
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInput/TradingFormInputFiatCrypto/TradingFormInputFiatCrypto.tsx
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingVerify/TradingVerify.tsx
- packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts
- packages/suite/src/views/wallet/trading/common/TradingTransactions/TradingTransaction/TradingTransactionAmounts.tsx
- packages/suite/src/views/wallet/trading/common/TradingFeaturedOffers/TradingFeaturedOffersAmounts.tsx
- packages/suite/src/hooks/wallet/trading/form/useTradingSellForm.ts
- packages/suite/src/hooks/wallet/trading/form/useTradingBuyForm.tsx
- packages/suite/src/utils/wallet/trading/tests/sellUtils.test.ts
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingInfo/TradingInfoHeader.tsx
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendSwap.tsx
- packages/suite/src/views/wallet/trading/common/TradingAddressOptions.tsx
- packages/suite/src/types/trading/trading.ts
- packages/suite/src/utils/wallet/trading/tests/buyUtils.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-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: build-deploy
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
suite-common/trading/src/types.ts (3)
103-105: Type discriminator addition looks goodAdding the
type: 'currency'discriminator field toTradingCryptoSelectItemPropsenables proper type narrowing when used in the union type withTradingSelectAssetOptionGroupProps. The optionalbalanceandnetworkNameproperties enhance the type with useful information for UI rendering.
108-113: Clear interface definition for option groupsThe
TradingSelectAssetOptionGroupPropsinterface is well-defined with a clear discriminator (type: 'group') to distinguish it from individual crypto items in the union type.
115-117: Good use of discriminated unionThis union type properly combines the two option types (items and groups) with discriminating
typefields, making it easy to use type narrowing in components that consume these types.
Description
Refactor
useTradingInfoto suite-common.