feat(trading): trading sign message flow #17005
Conversation
packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts
Outdated
Show resolved
Hide resolved
packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts
Outdated
Show resolved
Hide resolved
...rading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx
Outdated
Show resolved
Hide resolved
...rading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx
Show resolved
Hide resolved
FreeWall
force-pushed
the
feat/trading-sign-message-flow
branch
from
ce372cb to
473bfe2
Compare
|
🚀 Expo preview is ready!
|
WalkthroughThe changes involve updates to dependency versions and enhancements to the trading exchange functionality. The version of the 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (9)
✨ 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
🧹 Nitpick comments (2)
packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts (1)
603-639:❓ Verification inconclusive
Consider future extensibility for other networks.
The current implementation only supports Ethereum network and EIP-712 typed data. While this meets the current requirements, consider structuring the code to easily accommodate future network types.
Let's check if there are any other network types that might need support:
🏁 Script executed:
#!/bin/bash # Search for network types in the codebase rg -A 2 "networkType.*=.*"Length of output: 66988
Refactor signature handling to support additional networks
While the current implementation correctly handles Ethereum signatures via EIP-712, our codebase clearly uses various network types (e.g., Bitcoin, Cardano, Solana, Ripple). To make the trading exchange form more future-proof, consider abstracting the network-specific signing logic—perhaps using an adapter or factory approach. This would allow you to easily add or modify support for other networks without overly complicating the core conditional logic.
- Current behavior: Only Ethereum EIP-712 typed data is supported.
- Suggestion: Isolate the signing functionality into network-specific modules so new networks can be integrated seamlessly.
- Benefit: Consistent extensibility with the rest of the codebase that handles multiple network types.
packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSignData.tsx (1)
9-15: Consider using TypeScript type guards for validation.While the current validation works, using TypeScript type guards would make the validation more explicit and type-safe.
Consider this refactor:
- if (!selectedQuote) return null; - - const { exchange, dexTx, receive, send } = selectedQuote; - if (!exchange || !dexTx || !receive || !send) return null; + if (!selectedQuote || !isValidQuote(selectedQuote)) return null; + + const { exchange, dexTx, receive, send } = selectedQuote; +// Type guard function +function isValidQuote(quote: typeof selectedQuote): quote is NonNullable<typeof selectedQuote> & { + exchange: NonNullable<typeof selectedQuote.exchange>; + dexTx: NonNullable<typeof selectedQuote.dexTx>; + receive: NonNullable<typeof selectedQuote.receive>; + send: NonNullable<typeof selectedQuote.send>; +} { + return !!quote && !!quote.exchange && !!quote.dexTx && !!quote.receive && !!quote.send; +}
📜 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 (7)
packages/suite/package.json(1 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts(4 hunks)packages/suite/src/types/trading/tradingForm.ts(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchange.tsx(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSignData.tsx(1 hunks)suite-common/trading/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- suite-common/trading/package.json
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Type Checking
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: Unit Tests
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (9)
packages/suite/package.json (1)
129-129:✅ Verification successful
Verify the version update of @types/invity-api.
The version bump from
^1.1.2to^1.1.3might be related to new type definitions needed for the sign message flow. Let's verify the changes in this version.
🏁 Script executed:
#!/bin/bash # Description: Check the changelog or release notes for @types/invity-api version 1.1.3 # Check npm for package details curl -s https://registry.npmjs.org/@types/invity-api | jq '.versions["1.1.3"]' # Check for any security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "@types/invity-api") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 2316
@types/invity-api Version Update Verified
- The npm registry confirms that version 1.1.3 is published with the correct package details.
- No security advisories were reported for this version.
- The update aligns with the expected changes (e.g., new type definitions for the sign message flow).
packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchange.tsx (1)
13-13:✅ Verification successful
LGTM! The changes correctly integrate the new sign data flow.
The implementation properly handles the new
SIGN_DATAstep by:
- Adding the new component import
- Updating the
isActivecondition- Conditionally rendering the appropriate component based on the exchange step
Let's verify the component integration:
Also applies to: 54-54, 58-64
🏁 Script executed:
#!/bin/bash # Description: Verify the TradingOfferExchangeSignData component implementation # Check if the component exists and exports correctly ast-grep --pattern 'export const TradingOfferExchangeSignData' # Check if the component is used correctly ast-grep --pattern 'TradingOfferExchangeSignData'Length of output: 7304
Verified Component Integration: TradingOfferExchangeSignData is Correct
I’ve confirmed through the shell script checks that the new sign data flow is correctly integrated. Specifically:
- Export Verification: The component is properly exported from
TradingOfferExchangeSignData.tsx(starting at line 8).- Import & Usage: It’s correctly imported in
TradingOfferExchange.tsx(line 13) and is used appropriately (e.g., at line 60, and similarly in sections 54 and 58–64).LGTM! The changes correctly implement the
SIGN_DATAstep.packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx (1)
104-114: LGTM! The refactoring improves code clarity.The
proceedToSwapfunction has been simplified while maintaining the same functionality:
- Creates a new quote object without mutation
- Clears the approval type if present
- Dispatches the updated quote
- Proceeds with trade confirmation
packages/suite/src/types/trading/tradingForm.ts (1)
143-144: LGTM! The type definitions properly support the new sign data flow.The changes correctly add:
SIGN_DATAas a valid exchange step typesignDataAndConfirmmethod to the form context props interfaceAlso applies to: 272-272
packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts (4)
590-599: LGTM! Error handling is properly implemented.The function correctly validates the presence of required data before proceeding with the signing process.
429-433: LGTM! Status handling is consistent.The new 'SIGN_DATA' status is handled consistently with other status types in the function.
807-807: LGTM! Function export is properly added.The signDataAndConfirm function is correctly exposed in the hook's return object.
641-657:❓ Verification inconclusive
Verify trade status update logic.
The function updates the trade status and triggers confirmation. However, there's a potential edge case where
receiveAddressis missing.Let's check how
receiveAddressis set:
🏁 Script executed:
#!/bin/bash # Search for assignments to receiveAddress rg "receiveAddress.*="Length of output: 670
Action: Confirm Robustness of Trade Status Handling in the Absence of receiveAddress
The current implementation updates the trade status to
'SIGN_DATA'and proceeds to dispatch the trade confirmation only iftrade.receiveAddressis present. Our code search found that assignments toreceiveAddressprimarily occur within test files and related utility functions, which suggests that in production this property might not always be set.Please verify the following:
- Ensure that
selectedQuotereliably provides a validreceiveAddressin all production flows.- Confirm that silently returning when
receiveAddressis missing (without logging or error feedback) is the desired behavior, or if additional error handling should be incorporated.packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSignData.tsx (1)
20-47: LGTM! UI implementation follows best practices.The component:
- Uses proper layout components
- Handles loading and disabled states
- Provides clear user feedback
- Uses translations for internationalization
FreeWall
force-pushed
the
feat/trading-sign-message-flow
branch
from
473bfe2 to
719b896
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts (1)
589-657: Consider improving error handling and type safety.The signDataAndConfirm function has a few areas that could be improved:
- The error message "Cannot sign, missing data" is not using translations (as noted in past review comments).
- The type check
selectedQuote.signData.type === 'eip712-typed-data'could benefit from a type guard.- The early return on missing receiveAddress could show an error message to improve user experience.
Consider applying these improvements:
const signDataAndConfirm = async () => { if (!selectedQuote?.signData) { dispatch( notificationsActions.addToast({ type: 'error', - error: 'Cannot sign, missing data', + error: 'TR_CANNOT_SIGN_MISSING_DATA', }), ); return; } dispatch(tradingCommonActions.setTradingModalAccountKey(account.key)); let signature; if ( account.networkType === 'ethereum' && selectedQuote.signData.type === 'eip712-typed-data' ) { const typedData = selectedQuote?.signData .data as EthereumSignTypedDataMessage<EthereumSignTypedDataTypes>; const result = await TrezorConnect.ethereumSignTypedData({ device, path: account.path, metamask_v4_compat: false, data: typedData, }); if (!result.success) { dispatch( notificationsActions.addToast({ type: 'sign-message-error', error: result.payload.error, }), ); return; } signature = result.payload.signature; } else { dispatch( notificationsActions.addToast({ type: 'error', - error: 'Cannot sign data, unsupported network', + error: 'TR_CANNOT_SIGN_UNSUPPORTED_NETWORK', }), ); return; } const trade = { ...selectedQuote }; trade.signature = signature; trade.status = 'SIGN_DATA'; if (!trade.receiveAddress) { + dispatch( + notificationsActions.addToast({ + type: 'error', + error: 'TR_CANNOT_SIGN_MISSING_RECEIVE_ADDRESS', + }), + ); return; } dispatch( tradingExchangeActions.saveTrade( trade, selectedAccount.account, new Date().toISOString(), ), ); confirmTrade(trade.receiveAddress, undefined, trade); };
📜 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 (7)
packages/suite/package.json(1 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts(4 hunks)packages/suite/src/types/trading/tradingForm.ts(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchange.tsx(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSignData.tsx(1 hunks)suite-common/trading/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- suite-common/trading/package.json
- packages/suite/package.json
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSignData.tsx
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchange.tsx
- packages/suite/src/types/trading/tradingForm.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts (3)
26-29: LGTM!The new imports from @trezor/connect are correctly added to support Ethereum typed data signing functionality.
429-433: LGTM!The new case for 'SIGN_DATA' status is properly handled in the confirmTrade function, correctly updating the exchange step.
807-807: LGTM!The signDataAndConfirm function is correctly exported in the hook's return object.
FreeWall
force-pushed
the
feat/trading-sign-message-flow
branch
2 times, most recently
from
11e749b to
eed4134
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts (1)
589-657: New signDataAndConfirm function aligns with PR objectives.The implementation of the
signDataAndConfirmfunction looks solid. It correctly:
- Validates the presence of sign data
- Sets the trading modal account key
- Handles Ethereum EIP-712 typed data signing
- Properly processes the signing result
- Updates the trade with the signature before calling confirmTrade
A couple of suggestions:
- Consider adding a check for
trade.receiveAddressearlier in the function to avoid unnecessary operations if it's not available.- The error handling for an unsuccessful result from TrezorConnect is good, but consider adding more explicit error handling for unsupported sign data types.
const signDataAndConfirm = async () => { if (!selectedQuote?.signData) { dispatch( notificationsActions.addToast({ type: 'error', error: 'Cannot sign, missing data', }), ); return; } + if (!selectedQuote.receiveAddress) { + dispatch( + notificationsActions.addToast({ + type: 'error', + error: 'Missing receive address for signed data', + }), + ); + return; + } dispatch(tradingCommonActions.setTradingModalAccountKey(account.key)); let signature; if ( account.networkType === 'ethereum' && selectedQuote.signData.type === 'eip712-typed-data' ) { const typedData = selectedQuote?.signData .data as EthereumSignTypedDataMessage<EthereumSignTypedDataTypes>; const result = await TrezorConnect.ethereumSignTypedData({ device, path: account.path, metamask_v4_compat: false, data: typedData, }); if (!result.success) { dispatch( notificationsActions.addToast({ type: 'sign-message-error', error: result.payload.error, }), ); return; } signature = result.payload.signature; } else { + const unsupportedReason = account.networkType !== 'ethereum' + ? 'unsupported network' + : 'unsupported data type'; dispatch( notificationsActions.addToast({ type: 'error', - error: 'Cannot sign data, unsupported network', + error: `Cannot sign data, ${unsupportedReason}`, }), ); return; } const trade = { ...selectedQuote }; trade.signature = signature; trade.status = 'SIGN_DATA'; - if (!trade.receiveAddress) { - return; - } dispatch( tradingExchangeActions.saveTrade( trade, selectedAccount.account, new Date().toISOString(), ), ); confirmTrade(trade.receiveAddress, undefined, trade); };packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSignData.tsx (1)
1-48: Well-structured component for handling data signing UI.The component is cleanly implemented with proper:
- Type imports and usage
- Defensive checks for required data
- Clear UI structure with appropriate components
- Integration with the trading form context
A couple of observations:
- The component correctly checks for the presence of required data and returns null if missing.
- It uses the provider name from exchangeInfo with a fallback to the exchange name.
- The button is appropriately disabled when the device is not connected.
Consider adding a brief explanation text about what signing means to improve user understanding:
<Column gap={spacings.lg} flex="1"> + <InfoItem> + <Translation id="TR_EXCHANGE_SIGN_DATA_EXPLANATION" /> + </InfoItem> <InfoItem label={<Translation id="TR_EXCHANGE_SEND_FROM" />}> <AccountLabeling account={account} /> </InfoItem> ... </Column>This would require adding a new translation string that explains the data signing process.
📜 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 (7)
packages/suite/package.json(1 hunks)packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts(4 hunks)packages/suite/src/types/trading/tradingForm.ts(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchange.tsx(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSignData.tsx(1 hunks)suite-common/trading/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- suite-common/trading/package.json
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchange.tsx
- packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferExchange/TradingOfferExchangeSendApproval.tsx
- packages/suite/package.json
- packages/suite/src/types/trading/tradingForm.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: build-deploy
- GitHub Check: EAS Update
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build-web
🔇 Additional comments (3)
packages/suite/src/hooks/wallet/trading/form/useTradingExchangeForm.ts (3)
26-29: Good addition of specific Ethereum types for type safety.The addition of explicit
EthereumSignTypedDataMessageandEthereumSignTypedDataTypestypes from@trezor/connectimproves type safety when working with typed data signing.
429-433: Properly handling the new SIGN_DATA status.The conditional branch for handling the 'SIGN_DATA' status is correctly implemented, updating the exchange step and maintaining the flow consistency.
807-807: Good export of the new functionality.Adding
signDataAndConfirmto the returned object makes the function available to other components, maintaining a clean API.
FreeWall
force-pushed
the
feat/trading-sign-message-flow
branch
from
eed4134 to
9d432f6
Compare
tomasklim
added
the
release
| const providerName = | ||
| exchangeInfo?.providerInfos[exchange]?.companyName || selectedQuote.exchange; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
I like how clean this screen is.
However, a lot of users will see for the first time signing on the device.
I would love to have some banner here.
- sign data
- no tx fees
- swap can result into partial fill
Sign on Trezor and send- maybe we can use some extract from here https://help.1inch.io/en/articles/6800254-1inch-fusion-faq#h_3aa8adb5de
In case we do the above, I find this ok
| @@ -268,6 +269,7 @@ export interface TradingExchangeFormContextProps | |||
| setExchangeStep: (step: TradingExchangeStepType) => void; | |||
| confirmTrade: (address: string, extraField?: string, trade?: ExchangeTrade) => Promise<boolean>; | |||
| sendTransaction: () => void; | |||
| signDataAndConfirm: () => void; | |||
| new Date().toISOString(), | ||
| ), | ||
| ); | ||
| confirmTrade(trade.receiveAddress, undefined, trade); |
Description
For 1inch fusion/fusion+, user doesn't send a swap tx, only the signature of typed data (permit) is passed to the partner via API.
New sign flow is invoked by SIGN_DATA trade status.
Related Issue
Resolve #17118
Screenshots: