Solana tx timer #17045
Solana tx timer #17045
Conversation
...e/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx
Outdated
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
izmy
force-pushed
the
feat/solana-tx-modal-timer
branch
3 times, most recently
from
d3b3b62 to
de18dbb
Compare
|
🚀 Expo preview is ready!
|
izmy
requested review from
MiroslavProchazka,
marekrjpolak,
a team and
mroz22
as code owners
WalkthroughThe changes enhance Solana transaction error handling by introducing new error constants and refining the conditions within the transaction push process. New error constants have been added for specific transaction failures, and the error handling logic has been reorganized to improve granularity. A new "warning" variant has been added to the Badge component, expanding its configurability. The Transaction Review Modal has been updated to include retry functionality and countdown timers for transaction validity. Enhancements to error handling in transaction signing thunks now allow differentiation between timeout and cancellation errors. The stake reducer has been modified to include a creation timestamp for transactions. New support messages related to transaction handling have been introduced, along with a help center URL for Solana transactions. Minor refactors include using a named selector for account state retrieval and adjusting UI elements in stake confirmation modals. Overall, the changes address both backend error management and front-end user interface updates related to transaction processing workflows. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (2)
✨ 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: 1
🧹 Nitpick comments (5)
packages/suite/src/hooks/useTransactionTimeout.tsx (1)
9-37: Consider adding input validation and performance optimizations.While the hook implementation is generally good, there are a few improvements that could be made:
- Input validation for negative values
- Performance optimization by using
useReffor the interval IDexport const useTransactionTimeout = ( timestamp: number, timeoutSeconds: number, onTimeout?: () => void, ) => { + if (timestamp < 0 || timeoutSeconds < 0) { + throw new Error('timestamp and timeoutSeconds must be non-negative'); + } + + const intervalRef = useRef<NodeJS.Timeout>(); const [remainingTime, setRemainingTime] = useState(() => getRemainingTime(timestamp, timeoutSeconds), ); const handleTimeout = useCallback(() => { onTimeout?.(); }, [onTimeout]); useEffect(() => { - const intervalId = setInterval(() => { + intervalRef.current = setInterval(() => { const newRemainingTime = getRemainingTime(timestamp, timeoutSeconds); setRemainingTime(newRemainingTime); if (newRemainingTime === 0) { - clearInterval(intervalId); + clearInterval(intervalRef.current); handleTimeout(); } }, 1000); - return () => clearInterval(intervalId); + return () => clearInterval(intervalRef.current); }, [timestamp, timeoutSeconds, handleTimeout]); return remainingTime; };packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (2)
23-27: Consider adding validation for remainingTime prop.While the props are well-typed, consider adding runtime validation for the remainingTime prop to ensure it's non-negative.
type TransactionReviewOutputTimerProps = { - remainingTime: number; + remainingTime: number & { __brand: 'NonNegative' }; isMinimal?: boolean; onTryAgain?: (close: boolean) => void; }; + +// Utility type guard +const isNonNegative = (n: number): n is number & { __brand: 'NonNegative' } => n >= 0;
29-67: Consider adding aria-label for better accessibility.The component is well-implemented but could benefit from improved accessibility.
export const TransactionReviewOutputTimer = ({ isMinimal, remainingTime, onTryAgain, }: TransactionReviewOutputTimerProps) => { if (isMinimal) { return ( - <Badge variant="warning"> + <Badge variant="warning" aria-label={`${remainingTime} seconds remaining`}> <TimerBox> <Translation id="TR_SOLANA_TX_CONFIRMATION_TIMER_SHORT" values={{ remainingTime, strong: chunks => <strong>{chunks}</strong>, }} /> </TimerBox> </Badge> ); } return ( <Banner variant="warning" icon="hourglass"> <TimerBox> <Text typographyStyle="callout" as="div"> <Translation id="TR_SOLANA_TX_CONFIRMATION_TIMER" values={{ remainingTime }} /> </Text> <Translation id="TR_SOLANA_TX_CONFIRMATION_TIMER_DESCRIPTION" /> {onTryAgain && ( <Row margin={{ top: spacings.xs }}> - <RetryLink onClick={() => onTryAgain(true)}> + <RetryLink + onClick={() => onTryAgain(true)} + role="button" + aria-label="Try transaction again" + > <Translation id="TR_SOLANA_TX_CONFIRMATION_TIMER_AGAIN" /> </RetryLink> </Row> )} </TimerBox> </Banner> ); };packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (2)
45-45: Document the significance of the 58-second timeout.The magic number
58seconds should be documented to explain why this specific duration was chosen for Solana transactions.Add a comment explaining that this value is slightly less than Solana's block validity period (60 seconds) to ensure transactions are processed before expiration.
211-217: Improve function naming and parameter clarity.The function name and parameter could be more descriptive:
-const handleTryAgain = (cancel: boolean) => { +const handleRetryTransaction = (shouldCancelPrevious: boolean) => { - if (cancel) { + if (shouldCancelPrevious) { TrezorConnect.cancel('tx-timeout'); } tryAgainSignTx(); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/blockchain-link/src/workers/solana/index.ts(2 hunks)packages/components/src/components/Badge/Badge.stories.tsx(1 hunks)packages/components/src/components/Badge/Badge.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(9 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx(1 hunks)packages/suite/src/hooks/useTransactionTimeout.tsx(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/wallet-core/src/send/sendFormActions.ts(1 hunks)suite-common/wallet-core/src/send/sendFormReducer.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(2 hunks)
⏰ 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-web
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: EAS Update
🔇 Additional comments (21)
packages/components/src/components/Badge/Badge.stories.tsx (1)
30-30: LGTM! The new warning variant is properly added to the Storybook configuration.The addition of the 'warning' variant is well-typed and consistent with the component's API.
packages/components/src/components/Badge/Badge.tsx (1)
24-24: LGTM! The warning variant is consistently implemented across all color mappings.The implementation:
- Properly extends the type system
- Uses semantic color tokens
- Maintains consistency with existing variants
Let's verify the color contrast ratios for the warning variant using Python:
#!/usr/bin/env python3 import colorsys import math def hex_to_rgb(hex_color): hex_color = hex_color.lstrip('#') return tuple(int(hex_color[i:i+2], 16) for i in (0, 2, 4)) def relative_luminance(r, g, b): rs = r/255 gs = g/255 bs = b/255 rs = rs/12.92 if rs <= 0.03928 else math.pow((rs+0.055)/1.055, 2.4) gs = gs/12.92 if gs <= 0.03928 else math.pow((gs+0.055)/1.055, 2.4) bs = bs/12.92 if bs <= 0.03928 else math.pow((bs+0.055)/1.055, 2.4) return 0.2126 * rs + 0.7152 * gs + 0.0722 * bs def contrast_ratio(l1, l2): return (l1 + 0.05)/(l2 + 0.05) if l1 > l2 else (l2 + 0.05)/(l1 + 0.05) # Example warning colors (replace with actual theme colors) warning_bg = "#FFF5D6" # backgroundAlertYellowSubtleOnElevation0 warning_text = "#936200" # textAlertYellow bg_rgb = hex_to_rgb(warning_bg) text_rgb = hex_to_rgb(warning_text) bg_luminance = relative_luminance(*bg_rgb) text_luminance = relative_luminance(*text_rgb) ratio = contrast_ratio(bg_luminance, text_luminance) print(f"Contrast ratio: {ratio:.2f}") print(f"Meets WCAG AA for normal text (>= 4.5:1): {ratio >= 4.5}") print(f"Meets WCAG AA for large text (>= 3:1): {ratio >= 3}")[skip_cloning]
Also applies to: 57-57, 68-68, 79-79
packages/suite/src/hooks/useTransactionTimeout.tsx (1)
3-7: LGTM! The time calculation logic is accurate and handles edge cases.The
getRemainingTimefunction correctly:
- Converts milliseconds to seconds using floor division
- Ensures non-negative values using Math.max
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx (1)
1-34: LGTM! The error UI component is well-implemented.The component:
- Has proper type definitions
- Uses semantic UI components
- Provides clear error messaging with help resources
- Follows accessibility best practices with proper icon usage
suite-common/wallet-core/src/send/sendFormActions.ts (1)
28-37: LGTM! The timestamp addition to the action payload is well-typed.The change properly integrates the timestamp into the action creator while maintaining type safety.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (1)
10-21: LGTM! The styled components are well-implemented.Good use of:
tabular-numsfor consistent number display- Proper hover states for the retry link
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (3)
4-4: LGTM: New import for transaction retry functionality.The import of
signAndPushSendFormTransactionThunkis correctly added to support the retry mechanism.
23-23: LGTM: Well-implemented retry functionality.The
handleTryAgainSignTxfunction is properly implemented with:
- Null checks for both
precomposedFormandprecomposedTx- Correct usage of the selected account from state
Also applies to: 35-45
49-50: LGTM: Props correctly passed to child component.The timestamp and retry function are properly passed to TransactionReviewModalContent.
Also applies to: 52-52
suite-common/wallet-core/src/send/sendFormReducer.ts (3)
36-36: LGTM: Type definition properly updated.The SendState type is correctly extended with an optional timestamp property.
44-44: LGTM: State properly initialized.The timestamp is correctly initialized as undefined in the initial state.
72-72: LGTM: Action handler correctly updated.The storePrecomposedTransaction action handler properly updates the timestamp in state.
Also applies to: 79-79
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx (2)
30-31: LGTM: Props properly typed.The remainingTime and onTryAgain props are correctly added to TransactionReviewOutputListProps.
116-127: LGTM: Well-structured timer integration.The timer component is:
- Properly nested within a Column component
- Correctly conditional on Solana network type
- Well-organized with appropriate spacing
packages/urls/src/urls.ts (1)
129-131: Track TODO for updating help center link.The URL currently points to a general Solana article. Ensure to update it when the specific article about Solana transactions is ready.
Please track this TODO and update the URL when the specific article becomes available.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (2)
84-86: LGTM: Timer implementation with automatic cancellation.The hook correctly manages the countdown timer and automatically cancels the transaction when time expires.
343-345: Clarify the buttonRequestsCount condition.The condition
buttonRequestsCount != 1is not immediately clear. Consider adding a comment explaining its significance or using a named constant.Could you explain why this specific condition is used? This will help determine if the logic is correct.
suite-common/wallet-core/src/send/sendFormThunks.ts (2)
460-462: LGTM: Proper error handling for transaction timeout.The early return on timeout prevents showing unnecessary error toasts since the UI handles the timeout state.
585-585: LGTM: Timestamp addition for transaction tracking.The timestamp is correctly added to track transaction lifetime and support the timer functionality.
packages/blockchain-link/src/workers/solana/index.ts (1)
201-211: LGTM: Improved error handling for expired transactions.The error handling correctly identifies expired transactions by checking both the preflight failure and blockhash not found conditions. The error message is clear and guides users to retry.
packages/suite/src/support/messages.ts (1)
9131-9156: LGTM! Well structured message additions for Solana transaction handling.The new messages provide clear user feedback about transaction timeouts, remaining confirmation time, and retry options. The messages are well organized with descriptive IDs and make good use of dynamic value interpolation.
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
izmy
force-pushed
the
feat/solana-tx-modal-timer
branch
from
de18dbb to
ca838db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
35-45: Consider adding error handling for null checks.The function correctly implements the retry logic, but could benefit from error feedback when precomposedForm or precomposedTx are null.
const handleTryAgainSignTx = () => { if (send.precomposedForm != null && send.precomposedTx != null) { dispatch( signAndPushSendFormTransactionThunk({ formState: send.precomposedForm, precomposedTransaction: send.precomposedTx, selectedAccount: selectedAccount.account, }), ); + } else { + console.warn('Cannot retry signing: transaction data is missing'); } };suite-common/wallet-core/src/send/sendFormThunks.ts (1)
460-469: Consider adding logging for timeout events.The timeout handling is correctly implemented, but adding logging would help with debugging and monitoring.
if (message === 'tx-timeout') { + console.info('Transaction signing timed out'); return; } if (message === 'tx-cancelled') { + console.info('Transaction signing cancelled by user'); return rejectWithValue({ error: 'sign-transaction-failed', message: 'User canceled the signing process.', }); }packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
70-70: Consider a more robust default value for timestamp.The default value of 0 for the timestamp prop could cause unexpected behavior if not handled properly in the
useTransactionTimeouthook, as it represents the Unix epoch (January 1, 1970). Consider usingDate.now()orundefinedas the default value to better represent the absence of a timestamp.- timestamp = 0, + timestamp = Date.now(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/blockchain-link/src/workers/solana/index.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(9 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx(1 hunks)packages/suite/src/hooks/useTransactionTimeout.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)suite-common/wallet-core/src/send/sendFormActions.ts(1 hunks)suite-common/wallet-core/src/send/sendFormReducer.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx
- packages/suite/src/hooks/useTransactionTimeout.tsx
- suite-common/wallet-core/src/send/sendFormActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
- suite-common/wallet-core/src/send/sendFormReducer.ts
- packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: build-web
- 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: Setup and Cache Dependencies
- GitHub Check: test
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
🔇 Additional comments (8)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (2)
4-4: LGTM!The new import and selector are correctly added to support the retry functionality.
Also applies to: 23-23
49-49: LGTM!The timestamp and tryAgainSignTx props are correctly passed to support the countdown timer and retry functionality.
Also applies to: 52-52
suite-common/wallet-core/src/send/sendFormThunks.ts (1)
585-585: LGTM!The timestamp is correctly added to support the countdown timer feature.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (4)
84-86: LGTM!The
useTransactionTimeouthook is correctly implemented with proper parameters and a cancel callback.
211-217: LGTM!The
handleTryAgainfunction is well-implemented with proper error handling and flexibility through the cancel parameter.
354-354: Use Translation component for button text.The button text should be internationalized for consistency.
- <Translation id="TR_AGAIN" /> + <Translation id="TR_RETRY" />
343-345: LGTM!The conditional rendering logic correctly handles the timer reset issue by checking
buttonRequestsCount.packages/blockchain-link/src/workers/solana/index.ts (1)
201-211: LGTM!The error handling for Solana transaction preflight failure is well-implemented with:
- Proper error constant usage
- Clear error message for users
- Correct nesting of error conditions as suggested in past review comments
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
izmy
force-pushed
the
feat/solana-tx-modal-timer
branch
from
ca838db to
e2ca37d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
45-45:⚠️ Potential issueAdjust the Solana transaction timeout value.
The
SOLANA_TX_TIMEOUT_SECONDSis set to 58 seconds, which is slightly below Solana's minimum blockhash expiration time of 60 seconds. This tight margin could lead to premature timeouts.-const SOLANA_TX_TIMEOUT_SECONDS = 58; +const SOLANA_TX_TIMEOUT_SECONDS = 60;
🧹 Nitpick comments (7)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
48-55: Consider adding error handling for tryAgainSignTx.While the implementation is correct, consider adding error handling to provide feedback when the retry action fails.
<TransactionReviewModalContent timestamp={send.timestamp} decision={decision} txInfoState={txInfoState} - tryAgainSignTx={handleTryAgainSignTx} + tryAgainSignTx={async () => { + try { + await handleTryAgainSignTx(); + } catch (error) { + dispatch(notificationsActions.addToast({ + type: 'error', + error: 'Failed to retry transaction' + })); + } + }} cancelSignTx={handleCancelSignTx} isRbfConfirmedError={type === 'review-transaction-rbf-previous-transaction-mined-error'} />packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (6)
84-86: Consider adding error handling for the timeout callback.The timeout callback directly calls
TrezorConnect.cancel()without any error handling. If the call fails, the error would be uncaught.const remainingTime = useTransactionTimeout(timestamp, SOLANA_TX_TIMEOUT_SECONDS, () => { - TrezorConnect.cancel('tx-timeout'); + try { + TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel transaction:', error); + } });
211-217: Add error handling tohandleTryAgain.The
handleTryAgainfunction should handle potential errors fromTrezorConnect.cancel()andtryAgainSignTx().const handleTryAgain = (cancel: boolean) => { - if (cancel) { - TrezorConnect.cancel('tx-timeout'); - } - - tryAgainSignTx(); + try { + if (cancel) { + TrezorConnect.cancel('tx-timeout'); + } + tryAgainSignTx(); + } catch (error) { + console.error('Failed to retry transaction:', error); + dispatch( + notificationsActions.addToast({ + type: 'error', + error: 'Failed to retry transaction', + }) + ); + } };
343-361: Improve the timer visibility condition.The timer visibility condition
buttonRequestsCount != 1is unclear and could be made more explicit. Consider using a more descriptive condition or extracting it into a named variable.-{networkType === 'solana' && - !isSolanaExpired && - buttonRequestsCount != 1 && ( +const shouldShowTimer = networkType === 'solana' && + !isSolanaExpired && + buttonRequestsCount > 1; + +{shouldShowTimer && (
354-354: Use consistent translation key.The translation key
TR_AGAINis used for the retry button whileTR_TRY_AGAINis used elsewhere. This inconsistency could lead to confusion.-<Translation id="TR_AGAIN" /> +<Translation id="TR_TRY_AGAIN" />
178-190: Consider adding loading state management for non-Solana networks.The
handleSendfunction only sets the loading state for Solana networks. This could lead to inconsistent UI feedback for other networks.const handleSend = () => { - if (networkType === 'solana') { - setIsSending(true); - } + setIsSending(true); if (decision) { decision.resolve(true); reportTransactionCreatedEvent( isRbfTransaction(precomposedTx) ? mapRbfTypeToReporting[precomposedTx.rbfType] : 'sent', ); } };
132-141: Add error handling toonCancel.The
onCancelfunction should handle potential errors fromcancelSignTx()anddecision?.resolve().const onCancel = () => { - if (isRbfConfirmedError || networkType === 'solana') { - dispatch(modalActions.onCancel()); - } - - if (isActionAbortable || serializedTx) { - cancelSignTx(); - decision?.resolve(false); - } + try { + if (isRbfConfirmedError || networkType === 'solana') { + dispatch(modalActions.onCancel()); + } + + if (isActionAbortable || serializedTx) { + cancelSignTx(); + decision?.resolve(false); + } + } catch (error) { + console.error('Failed to cancel transaction:', error); + dispatch( + notificationsActions.addToast({ + type: 'error', + error: 'Failed to cancel transaction', + }) + ); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/blockchain-link/src/workers/solana/index.ts(2 hunks)packages/components/src/components/Badge/Badge.stories.tsx(1 hunks)packages/components/src/components/Badge/Badge.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(9 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx(1 hunks)packages/suite/src/hooks/useTransactionTimeout.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/wallet-core/src/send/sendFormActions.ts(1 hunks)suite-common/wallet-core/src/send/sendFormReducer.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/urls/src/urls.ts
- packages/components/src/components/Badge/Badge.stories.tsx
- suite-common/wallet-core/src/send/sendFormActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
- suite-common/wallet-core/src/send/sendFormReducer.ts
- packages/suite/src/hooks/useTransactionTimeout.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx
- packages/blockchain-link/src/workers/solana/index.ts
- packages/components/src/components/Badge/Badge.tsx
- packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
🔇 Additional comments (4)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (2)
4-4: LGTM! Import added for retry functionality.The import of
signAndPushSendFormTransactionThunkaligns with the PR's objective of adding retry capability.
35-45: LGTM! Well-implemented retry handler.The
handleTryAgainSignTxfunction includes proper null checks and correctly passes the required parameters to retry the transaction.suite-common/wallet-core/src/send/sendFormThunks.ts (2)
460-469: LGTM! Proper handling of transaction timeout.The code correctly handles the 'tx-timeout' case by returning early, preventing unnecessary error notifications while maintaining the existing cancellation logic.
585-585: LGTM! Timestamp added for transaction tracking.The timestamp is correctly added using
Date.getTime()to track transaction timing, which supports the new countdown timer feature.
izmy
force-pushed
the
feat/solana-tx-modal-timer
branch
from
e2ca37d to
f2c6dd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (2)
24-28: Consider improving type clarity for theonTryAgaincallback.The boolean parameter in
onTryAgaincould be more descriptive to indicate its purpose.type TransactionReviewOutputTimerProps = { deadline: number; isMinimal?: boolean; - onTryAgain?: (close: boolean) => void; + onTryAgain?: (shouldCloseModal: boolean) => void; };
64-66: Enhance accessibility and performance of the retry link.Consider adding ARIA attributes and memoizing the click handler.
+const handleTryAgain = React.useCallback(() => onTryAgain(true), [onTryAgain]); -<RetryLink onClick={() => onTryAgain(true)}> +<RetryLink + onClick={handleTryAgain} + role="button" + aria-label={<Translation id="TR_SOLANA_TX_CONFIRMATION_TIMER_AGAIN" />} +> <Translation id="TR_SOLANA_TX_CONFIRMATION_TIMER_AGAIN" /> </RetryLink>packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
39-50: Verify null checks in retry logic.The function checks for null on both
precomposedFormandprecomposedTx, but consider using optional chaining for more concise code.-if (send.precomposedForm != null && send.precomposedTx != null) { +if (send?.precomposedForm && send?.precomposedTx) {packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
57-77: Add JSDoc comments to explain the complex timer visibility logic.The
shouldShowSolanaTimerfunction contains complex business logic for determining when to show the timer. Adding documentation would help future maintainers understand the conditions.+/** + * Determines whether to show the Solana transaction timer. + * @param networkType - The type of network (e.g., 'solana') + * @param deadline - The transaction deadline timestamp + * @param outputs - List of transaction outputs + * @param symbol - The network symbol + * @param accounts - The accounts state + * @param buttonRequestsCount - Number of button requests + * @returns {boolean} True if the timer should be shown + */ const shouldShowSolanaTimer = ( networkType: NetworkType, deadline: number, outputs: ReviewOutput[], symbol: NetworkSymbol, accounts: AccountsState, buttonRequestsCount: number, ) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/blockchain-link/src/workers/solana/index.ts(2 hunks)packages/components/src/components/Badge/Badge.stories.tsx(1 hunks)packages/components/src/components/Badge/Badge.tsx(4 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(1 hunks)packages/suite/src/components/suite/CountdownTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(10 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/wallet-core/src/send/sendFormActions.ts(1 hunks)suite-common/wallet-core/src/send/sendFormReducer.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(4 hunks)suite-common/wallet-core/src/send/sendFormTypes.ts(1 hunks)suite-native/module-send/src/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/components/src/components/Badge/Badge.stories.tsx
- suite-common/wallet-core/src/send/sendFormActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx
- suite-common/wallet-core/src/send/sendFormReducer.ts
- packages/components/src/components/Badge/Badge.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
- packages/urls/src/urls.ts
- packages/blockchain-link/src/workers/solana/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
🔇 Additional comments (14)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (1)
1-22: LGTM! Well-structured imports and styled components.The imports are well-organized, and the styled components are appropriately defined. The use of
tabular-numsfor the timer display ensures consistent number width and alignment.packages/suite/src/support/messages.ts (2)
9135-9143: LGTM! Clear error messaging for Solana transaction timeouts.The error messages clearly explain to users that Solana transactions have a limited signing window and can timeout, becoming invalid.
9144-9160: LGTM! Well-structured timer messages for Solana transactions.The timer-related messages provide good user feedback about the remaining time to confirm transactions, with both short and long format options. The warning about the ~1 minute constraint helps set proper user expectations.
suite-common/wallet-core/src/send/sendFormTypes.ts (1)
60-63: LGTM! Well-structured error type addition.The new
SignTransactionTimeoutErrortype follows the established pattern and is properly integrated with the existing error handling system.packages/suite/src/components/suite/CountdownTimer.tsx (1)
88-88: LGTM! Clean addition of text formatting capability.The
strongproperty addition provides a clean way to emphasize specific parts of the translated text.suite-native/module-send/src/sendFormThunks.ts (1)
43-43: LGTM! Comprehensive error handling enhancement.The addition of
SignTransactionTimeoutErrorto the reject value type properly extends the error handling capabilities of the thunk.packages/suite/src/actions/wallet/send/sendFormThunks.ts (1)
235-239: LGTM! Well-placed timeout handling logic.The addition of the timeout check prevents premature modal closure, improving the user experience during transaction timeout scenarios.
suite-common/wallet-core/src/send/sendFormThunks.ts (3)
67-67: LGTM: New error type import.The addition of
SignTransactionTimeoutErroraligns with the PR's objective of handling transaction timeouts.
403-403: Improved error handling for transaction timeouts.The changes enhance error handling by:
- Adding timeout error to the reject value type
- Distinguishing between timeout and cancellation errors
- Providing clear error messages for both cases
This improvement aligns with the PR's objective of managing transaction timeouts effectively.
Also applies to: 461-473
589-589:❓ Verification inconclusive
Verify timestamp usage in the UI.
The timestamp addition will help track transaction age, which is crucial for the countdown timer feature. However, ensure that the UI correctly interprets this Unix timestamp.
🏁 Script executed:
#!/bin/bash # Search for timestamp usage in UI components rg -A 5 "timestamp.*getTime\(\)" --type ts --type tsxLength of output: 83
Review: Verify Correct UI Handling of Timestamp Data
The automated search did not return expected results due to an unrecognized file type error with TSX files. Please confirm that the UI components responsible for displaying transaction details—especially the countdown timer—are correctly interpreting the Unix timestamp added via
new Date().getTime(). You might need to verify this manually within the appropriate TSX files.
- Ensure TSX files are included in your searches or verify their usage manually.
- Double-check that the countdown timer and any related UI logic properly convert and handle the Unix timestamp.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (4)
52-52: LGTM! The timeout value aligns with Solana's blockhash expiration.The
SOLANA_TX_TIMEOUT_MSconstant is set to 60 seconds, which matches Solana's minimum blockhash expiration time. This addresses previous concerns about timer accuracy.
102-102: Verify the default timestamp behavior.Setting the default timestamp to 0 could cause immediate expiration of the transaction. Consider using
Date.now()as the default value to prevent this edge case.- timestamp = 0, + timestamp = Date.now(),
117-129: LGTM! Clean timer implementation with proper cleanup.The timer implementation is well-structured:
- Calculates remaining time correctly
- Uses cleanup function to prevent memory leaks
- Has stable dependency on deadline
404-404: Use consistent translation key for retry action.The button text uses "TR_AGAIN" while similar retry actions in the codebase use "TR_RETRY". Consider using the consistent translation key.
- <Translation id="TR_AGAIN" /> + <Translation id="TR_RETRY" />
izmy
force-pushed
the
feat/solana-tx-modal-timer
branch
from
f2c6dd9 to
4d5954f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (2)
11-22: Consider using theme tokens for text decoration properties.For better maintainability and consistency with the design system, consider using theme tokens for text decoration properties.
const RetryLink = styled.a` - text-decoration: underline; - text-decoration-style: dotted; + text-decoration: ${({ theme }) => theme.TEXT_DECORATION.dotted}; &:hover { - text-decoration: none; + text-decoration: ${({ theme }) => theme.TEXT_DECORATION.none}; } `;
64-66: Add ARIA label and role to the retry link.For better accessibility, add ARIA attributes to the retry link.
- <RetryLink onClick={() => onTryAgain(true)}> + <RetryLink + onClick={() => onTryAgain(true)} + role="button" + aria-label={<Translation id="TR_SOLANA_TX_CONFIRMATION_TIMER_AGAIN" />} + >packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (3)
57-77: Add JSDoc comments and break down complex conditions.The function's logic is complex and would benefit from documentation and clearer structure.
+/** + * Determines whether to show the Solana transaction timer. + * @param networkType - The type of network + * @param deadline - The transaction deadline timestamp + * @param outputs - Array of transaction outputs + * @param symbol - Network symbol + * @param accounts - Account state + * @param buttonRequestsCount - Number of button requests + * @returns boolean indicating whether to show the timer + */ const shouldShowSolanaTimer = ( networkType: NetworkType, deadline: number, outputs: ReviewOutput[], symbol: NetworkSymbol, accounts: AccountsState, buttonRequestsCount: number, ) => { + // Early return if not Solana or already expired if (networkType !== 'solana' || isSolanaExpired(networkType, deadline)) { return false; } const firstOutput = outputs[0]; + // Check if transaction is internal (between own accounts) const isInternalTransfer = firstOutput?.type === 'address' && findAccountsByAddress(symbol, firstOutput.value, accounts).length > 0; const isFirstStep = buttonRequestsCount === 1; return isInternalTransfer || !isFirstStep; };
120-129: Enhance error handling and timeout management.The timeout handling could be more robust with proper error handling and cleanup.
useEffect(() => { const now = Date.now(); const timeLeft = Math.max(deadline - now, 0); + // Skip setting timeout if already expired + if (timeLeft === 0) { + TrezorConnect.cancel('tx-timeout'); + return; + } const timeoutId = setTimeout(() => { + try { TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel transaction:', error); + } }, timeLeft); return () => clearTimeout(timeoutId); }, [deadline]);
263-269: Add error handling to transaction retry logic.The retry functionality should include error handling for robustness.
const handleTryAgain = (cancel: boolean) => { if (cancel) { + try { TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel transaction:', error); + } } + try { tryAgainSignTx(); + } catch (error) { + console.error('Failed to retry transaction:', error); + dispatch( + notificationsActions.addToast({ + type: 'error', + error: 'Failed to retry transaction', + }) + ); + } };packages/suite/src/support/messages.ts (1)
9135-9160: LGTM! Clear and informative transaction messages.The new messages for Solana transaction handling provide good user feedback about timing constraints and failure cases. The messages are well-structured and follow the existing pattern.
One minor suggestion - consider adding a period at the end of
TR_SOLANA_TX_CONFIRMATION_TIMER_DESCRIPTIONfor consistency with other similar messages:- 'Due to Solana constraints, you have ~1 minute to check everything before your transaction times out.', + 'Due to Solana constraints, you have ~1 minute to check everything before your transaction times out.'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/suite/src/actions/wallet/send/sendFormThunks.ts(1 hunks)packages/suite/src/components/suite/CountdownTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(10 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(4 hunks)suite-common/wallet-core/src/send/sendFormTypes.ts(1 hunks)suite-native/module-send/src/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- suite-native/module-send/src/sendFormThunks.ts
- suite-common/wallet-core/src/send/sendFormTypes.ts
- packages/suite/src/components/suite/CountdownTimer.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx
- suite-common/wallet-core/src/send/sendFormThunks.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: test
🔇 Additional comments (2)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (1)
24-28: LGTM!The props type definition is clear and well-structured.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
52-52: Verify the Solana transaction timeout value.Based on past review comments and Solana's documentation, blockhashes typically expire after 60-90 seconds. The current timeout of 60 seconds might be cutting it close, especially considering network latency.
Consider increasing the timeout to provide a buffer:
-const SOLANA_TX_TIMEOUT_MS = 60 * 1000; +const SOLANA_TX_TIMEOUT_MS = 75 * 1000; // 75 seconds to account for network conditions
izmy
force-pushed
the
feat/solana-tx-modal-timer
branch
from
4d5954f to
c07a945
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
54-54: 💡 Verification agent🛠️ Refactor suggestion
🧩 Analysis chain
Update Solana transaction timeout value.
The constant
SOLANA_TX_TIMEOUT_MSis set to 40 seconds (40,000 ms), which is significantly shorter than Solana's typical blockhash expiration window of 60-90 seconds. This could lead to premature expiration warnings and unnecessary retries.Increase the timeout value to at least 60,000 ms (60 seconds) to align with Solana's minimum blockhash expiration time:
-const SOLANA_TX_TIMEOUT_MS = 40 * 1000; +const SOLANA_TX_TIMEOUT_MS = 60 * 1000;
🌐 Web query:
What is the expiration time for a Solana blockhash?💡 Result:
A Solana blockhash expires 60–90 seconds after its generation due to network dynamics tied to slot durations and validation rules. Here's a breakdown of how this works:
Blockhash Expiration Mechanism
- Slot-Based Expiration: Each blockhash is valid for 151 slots (a slot being the period when a validator can propose a block). Slots typically last 400–600ms, leading to an expiration window of roughly 60–90 seconds[1][3][5].
- Block Height Comparison: Validators reject transactions if their referenced blockhash’s slot is more than 151 slots older than the current slot being processed[1][3]. Some sources describe this as 150 blocks, aligning with similar timing (~80–90 seconds)[6][7].
Key Factors Affecting Expiration
Slot Duration Variability:
- Slots fluctuate between 400ms and 600ms due to network conditions[1][5].
- Example: At 400ms/slot, expiration occurs after ~60 seconds (151 × 0.4s). At 600ms/slot, this extends to ~90 seconds[1][3].
RPC Node Behavior:
- Transactions using expired blockhashes return errors like
Blockhash not found[5].- Developers should fetch fresh blockhashes immediately before signing transactions and set matching commitment levels for preflight checks[1][2].
Practical Implications
- Applications must handle retries or prompt users if transactions fail due to expiration[2][5].
- Tools like
getLatestBlockhash(withlastValidBlockHeight) help track expiration programmatically[2][6].In summary, while exact terminology varies between "slots" and "blocks," transactions referencing a blockhash older than ~60–90 seconds risk rejection. Proactive management of blockhash freshness is critical for reliability[1][3][5].
Citations:
- 1: https://omniatech.io/pages/solana-transaction-confirmation/
- 2: https://www.quicknode.com/guides/solana-development/transactions/solana-transaction-propagation-handling-dropped-transactions
- 3: https://substack.com/home/post/p-142004310
- 4: https://blog.fordefi.com/warping-time-for-solana-defi-reconciling-expiring-nonces-and-institutional-policy-controls
- 5: https://www.helius.dev/blog/how-to-deal-with-blockhash-errors-on-solana
- 6: https://solana.com/docs/core/transactions
- 7: https://docs.chainstack.com/docs/solana-how-to-handle-the-transaction-expiry-error
Suggested Update: Align Solana Transaction Timeout with Blockhash Expiration
The current constant
SOLANA_TX_TIMEOUT_MSis set to 40,000 ms, which is below the minimum blockhash expiration threshold of approximately 60 seconds. Given that a Solana blockhash typically expires after 60–90 seconds, this shorter timeout might trigger premature expiration warnings and cause unnecessary transaction retries.
- Location:
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(Line 54)- Action: Increase the timeout value to at least 60,000 ms.
-const SOLANA_TX_TIMEOUT_MS = 40 * 1000; +const SOLANA_TX_TIMEOUT_MS = 60 * 1000;
🧹 Nitpick comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx (1)
30-31: Consider makingonTryAgainprop optional.The
onTryAgainprop is marked as required, but it might only be used whendeadlineis provided for Solana transactions. Making it optional (or conditionally required) would be more flexible, especially for non-Solana network types.- onTryAgain: (close: boolean) => void; + onTryAgain?: (close: boolean) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/blockchain-link/src/workers/solana/index.ts(2 hunks)packages/components/src/components/Badge/Badge.stories.tsx(1 hunks)packages/components/src/components/Badge/Badge.tsx(4 hunks)packages/suite-data/files/translations/en.json(1 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts(2 hunks)packages/suite/src/actions/wallet/stakeActions.ts(3 hunks)packages/suite/src/components/suite/CountdownTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(9 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(5 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/wallet-core/src/send/sendFormReducer.ts(1 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(3 hunks)suite-common/wallet-core/src/send/sendFormTypes.ts(1 hunks)suite-common/wallet-core/src/stake/stakeReducer.ts(1 hunks)suite-common/wallet-types/src/sendForm.ts(1 hunks)suite-native/module-send/src/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/urls/src/urls.ts
- suite-common/wallet-core/src/send/sendFormReducer.ts
- suite-common/wallet-types/src/sendForm.ts
- packages/components/src/components/Badge/Badge.stories.tsx
- packages/suite/src/actions/wallet/stakeActions.ts
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- suite-common/wallet-core/src/stake/stakeReducer.ts
- suite-native/module-send/src/sendFormThunks.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx
- packages/components/src/components/Badge/Badge.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx
- packages/blockchain-link/src/workers/solana/index.ts
- packages/suite-data/files/translations/en.json
- packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts
- packages/suite/src/components/suite/CountdownTimer.tsx
- suite-common/wallet-core/src/send/sendFormTypes.ts
- suite-common/wallet-core/src/send/sendFormThunks.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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-web
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: test
🔇 Additional comments (13)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx (4)
117-129: LGTM: Timer component implementation is well-structured.The implementation of the timer component for Solana transactions looks good. The code correctly:
- Shows the timer only for Solana network type and when a deadline is provided
- Uses a nested column layout for better organization
- Passes the necessary props to the timer component
This implementation aligns well with the PR objective of adding a countdown timer to Solana transaction modals.
126-127:✅ Verification successful
Ensure
isSendingis properly passed to the timer.The
isSendingprop is passed to the timer component but wasn't explicitly added to the component's props type. Make sure this prop is expected and handled properly in theTransactionReviewOutputTimercomponent.
🏁 Script executed:
#!/bin/bash # Check the TransactionReviewOutputTimer component implementation cat packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsxLength of output: 2601
Proper Integration of
isSendingin TransactionReviewOutputTimerThe
isSendingprop is now explicitly defined in theTransactionReviewOutputTimerPropstype and is used appropriately within the component. In the minimal layout, it correctly disables the retry button and conditionally determines theCountdownTimer'spastDeadlineMessage. In the non-minimal branch,isSendingis used to disable the Banner's retry button, which is consistent with the intended functionality. No further changes are required.
112-114:❓ Verification inconclusive
The condition logic was updated - verify implications.
The condition for showing the address confirmation view now includes additional checks for
!isInternalTransferand!signedTx. Please verify this doesn't unintentionally hide the address confirmation in cases where it should be shown.
🏁 Script executed:
#!/bin/bash # Find where this component is used to understand the impact rg "TransactionReviewOutputList" --type typescriptLength of output: 85
Conditional Check Update on TransactionReviewOutputList
The update now requires both
!isInternalTransferand!signedTxto render the address confirmation view. The initial search for usages using--type typescriptreturned an error, so please re-run this query with a corrected file type (for example, using--type tsx) or manually verify the component’s usage. Ensure that no valid scenarios—especially those involving internal transfers or signed transactions—are inadvertently causing the address confirmation to be skipped.
88-88:✅ Verification successful
Verify the change to
isFirstStepcondition.The condition for
isFirstStephas been changed from checking ifbuttonRequestsCount === 1tobuttonRequestsCount <= 1. This broadens the condition to include cases wherebuttonRequestsCountis 0, which could affect the rendering logic. Please ensure this change is intentional and won't cause unexpected behavior.
🏁 Script executed:
#!/bin/bash # Search for other uses of buttonRequestsCount in the codebase to understand the semantics rg "buttonRequestsCount" --type typescriptLength of output: 77
I'll re-run the search without specifying a file type to ensure we capture all usages of
buttonRequestsCountin the repository:Once we have the output from this command, we can better assess whether the change from
buttonRequestsCount === 1tobuttonRequestsCount <= 1(which now includes the 0 case) is intentional and safe. Please run this script and share the results for further evaluation.
🏁 Script executed:
#!/bin/bash rg "buttonRequestsCount"Length of output: 3688
Clarify the modification to the "isFirstStep" logic
The change from strictly checking
buttonRequestsCount === 1to usingbuttonRequestsCount <= 1now treats a count of 0 as a "first step" as well. Our search shows that this condition is applied consistently across related components (e.g., inTransactionReviewModalContent.tsx). If your intent is to handle scenarios where there might be no button requests (i.e., a count of 0), then this broader condition is appropriate. Otherwise, please double-check that including 0 won’t inadvertently affect the rendering logic.
- Review Locations:
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx– uses the same<= 1condition.packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx– the condition in question.If this handling of 0 is expected based on your design, then no further changes are needed.
packages/suite/src/support/messages.ts (2)
8913-8914: Quotation mark correction.The single quote character has been changed to a proper apostrophe in the message about ETH being blocked during a period.
9151-9176: New Solana transaction timeout messages look good.These newly added messages implement the timer functionality for Solana transactions as described in the PR objectives. The messages appropriately cover:
- Error handling for failed transactions due to timeout
- Visual feedback for the remaining time
- Clear information about time constraints (less than 1 minute)
- A recovery mechanism with the "Start over" button
These messages follow the project's naming conventions and formatting patterns.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (5)
125-140: Good implementation of the useEffect cleanup.The implementation properly handles component unmounting with the
mountedflag and timeout cleanup, preventing potential race conditions or memory leaks.
279-285: Add error handling for TrezorConnect.cancel.The
handleTryAgainfunction should handle potential errors fromTrezorConnect.cancel().const handleTryAgain = (cancel: boolean) => { if (cancel) { - TrezorConnect.cancel('tx-timeout'); + try { + TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel TrezorConnect:', error); + } } tryAgainSignTx(); };
296-307: Good implementation of retry functionality for expired blockhash.The retry mechanism for expired Solana transactions is well implemented, providing users with clear options to either try again or close the modal.
360-362: Good user feedback for expired transactions.Displaying the
ExpiredBlockhashcomponent when a Solana blockhash has expired provides clear feedback to users about why the transaction failed.
411-420: Well-implemented timer display logic.The conditional rendering of the timer based on transaction state and network type is well implemented, providing users with real-time feedback on transaction status.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (2)
47-66: Add null check for selectedAccount.The
handleTryAgainSignTxfunction assumesselectedAccount.accountis always defined, but this might not be true. Add a null check to prevent potential runtime errors.const handleTryAgainSignTx = () => { - if (send.precomposedForm && send.precomposedTx) { + if (send.precomposedForm && send.precomposedTx && selectedAccount.account) { dispatch(sendFormActions.discardTransaction()); dispatch( signAndPushSendFormTransactionThunk({ formState: send.precomposedForm, precomposedTransaction: send.precomposedTx, selectedAccount: selectedAccount.account, }), ); } else if (stake.precomposedForm && stake.precomposedTx) { dispatch(stakeActions.dispose()); dispatch( signTransaction( stake.precomposedForm, stake.precomposedTx as PrecomposedTransactionFinal, ), ); } };
69-74: Good passing of transaction timestamp and retry handler.The component correctly passes the transaction timestamp and retry handler to the modal content, enabling the timeout and retry functionality.
tomasklim
force-pushed
the
feat/solana-tx-modal-timer
branch
from
e409a3b to
39ada5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
54-54:⚠️ Potential issueUpdate the Solana transaction timeout value.
The constant
SOLANA_TX_TIMEOUT_MSis currently set to 40 seconds (40,000 ms), which is below Solana's typical blockhash expiration window of 60–90 seconds. This could lead to premature timeout notifications.Increase the timeout value to at least 60,000 ms to align with Solana's minimum blockhash expiration time:
-const SOLANA_TX_TIMEOUT_MS = 40 * 1000; +const SOLANA_TX_TIMEOUT_MS = 60 * 1000;
🧹 Nitpick comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
59-82: Consider adding documentation for timer display logic.The function
shouldShowSolanaTimercontains complex logic to determine when to display the timer. Adding JSDoc comments would improve maintainability and help future developers understand the conditions.+/** + * Determines whether to display the Solana transaction timer based on transaction state. + * + * Shows timer when: + * - Network is Solana and blockhash hasn't expired yet + * - AND (transaction is internal transfer OR not first step OR serialized transaction exists OR is staking) + * + * @returns boolean indicating whether timer should be displayed + */ const shouldShowSolanaTimer = ( networkType: NetworkType, deadline: number, outputs: ReviewOutput[], symbol: NetworkSymbol, accounts: AccountsState, buttonRequestsCount: number, serializedTx: SerializedTx | undefined, stakeType: StakeType | null, ) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/blockchain-link/src/workers/solana/index.ts(2 hunks)packages/components/src/components/Badge/Badge.stories.tsx(1 hunks)packages/components/src/components/Badge/Badge.tsx(4 hunks)packages/suite-data/files/translations/en.json(1 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts(2 hunks)packages/suite/src/actions/wallet/stakeActions.ts(3 hunks)packages/suite/src/components/suite/CountdownTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(9 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(5 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/wallet-core/src/send/sendFormReducer.ts(1 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(3 hunks)suite-common/wallet-core/src/send/sendFormTypes.ts(1 hunks)suite-common/wallet-core/src/stake/stakeReducer.ts(1 hunks)suite-common/wallet-types/src/transaction.ts(2 hunks)suite-native/module-send/src/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- suite-common/wallet-core/src/stake/stakeReducer.ts
- packages/suite/src/components/suite/CountdownTimer.tsx
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- suite-common/wallet-core/src/send/sendFormReducer.ts
- packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx
- packages/urls/src/urls.ts
- packages/components/src/components/Badge/Badge.stories.tsx
- suite-common/wallet-core/src/send/sendFormTypes.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx
- suite-native/module-send/src/sendFormThunks.ts
- suite-common/wallet-core/src/send/sendFormThunks.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx
- packages/suite-data/files/translations/en.json
- packages/blockchain-link/src/workers/solana/index.ts
- packages/suite/src/actions/wallet/stakeActions.ts
- packages/components/src/components/Badge/Badge.tsx
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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: EAS Update
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
🔇 Additional comments (17)
suite-common/wallet-types/src/transaction.ts (1)
112-112: LGTM: Adding timestamp tracking for transaction expiry.The addition of the optional
createdTimestampproperty to both transaction types is a solid enhancement that will enable the countdown timer functionality for the Solana transaction modal. This property will track when transactions are created, which is essential for calculating the remaining time until blockhash expiration.Also applies to: 122-122
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx (7)
8-8: New component imports added properlyThe imports are correctly updated to include the new
TransactionReviewOutputTimercomponent and reorganized the components import from@trezor/components.Also applies to: 17-17
30-31: New props added for Solana transaction timer functionalityThe props are well-defined:
deadline?: number- Optional timestamp for transaction expirationonTryAgain: (close: boolean) => void- Callback for retry functionalityThe
onTryAgainprop is required which could create issues if the component is used without providing this handler.Consider making
onTryAgainoptional if it's only needed for Solana transactions.
78-80: Props properly implemented in component parametersThe new props are correctly added to the function parameter list and
isSendingis appropriately moved to maintain consistency.
88-88: Changed condition forisFirstStepThe condition has been modified from exact equality (
buttonRequestsCount === 1) to less than or equal (buttonRequestsCount <= 1), which now includes cases wherebuttonRequestsCountis 0.This change modifies when the component considers itself to be in the "first step". Verify that this behavior change is intentional and doesn't cause unexpected UI states. Test with both buttonRequestsCount=0 and buttonRequestsCount=1 scenarios.
112-113: Modified conditional rendering checksThe conditions for rendering the Card component have been updated to include checks for
!isInternalTransferand!signedTx.
117-129: UI structure changes to accommodate timer componentThe layout has been restructured to:
- Reduce the gap in the main Column from
xxxltoxxl- Add a nested Column with the heading and timer with a gap of
md- Conditionally render the
TransactionReviewOutputTimerfor Solana transactions when a deadline is providedThis is a clean implementation that maintains UI consistency while adding new functionality.
122-128: Timer component integration for Solana transactionsThe timer component is properly integrated with:
- Appropriate conditional rendering (only for Solana transactions with a deadline)
- Required props passed down including
deadline,onTryAgain, andisSendingThis implementation aligns well with the PR objective of adding a countdown timer for Solana transactions.
packages/suite/src/support/messages.ts (2)
8913-8913: Minor syntax adjustment.Changed apostrophe to single quote in the text string.
9151-9176: Good implementation of Solana transaction timer messages.These new messages effectively support the countdown timer feature for Solana transaction confirmation, providing clear UI text for:
- Error states when transactions time out
- Countdown display in multiple formats
- Clear explanation of the time constraint
- Retry functionality text
This implementation aligns well with the PR objective of adding a timer to show remaining confirmation time and providing a retry option when the blockhash expires.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (5)
125-140: Add error handling for TrezorConnect.cancel.The useEffect hook should include error handling for the
TrezorConnect.cancelcall to prevent unhandled exceptions.useEffect(() => { const now = Date.now(); const timeLeft = Math.max(deadline - now, 0); let mounted = true; const timeoutId = setTimeout(() => { if (mounted && !isSending) { - TrezorConnect.cancel('tx-timeout'); + try { + TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel TrezorConnect:', error); + } } }, timeLeft); return () => { mounted = false; clearTimeout(timeoutId); }; }, [deadline, isSending]);
279-285: Add error handling for TrezorConnect.cancel.The
handleTryAgainfunction should handle potential errors fromTrezorConnect.cancel().const handleTryAgain = (cancel: boolean) => { if (cancel) { - TrezorConnect.cancel('tx-timeout'); + try { + TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel TrezorConnect:', error); + } } tryAgainSignTx(); };
296-307: Use Translation component for button text.The "Try Again" button text is already using a Translation component, which is good for internationalization.
The implementation of the retry button for expired blockhash looks clean and follows the UI pattern of the application.
364-380: Reusable column component improves layout structure.Wrapping the
TransactionReviewOutputListin aColumncomponent with consistent spacing improves UI structure and maintainability.The additional props for deadline and onTryAgain properly integrate the new timer functionality with the existing components.
401-421: Well-structured timer integration with the modal header.The timer is elegantly placed in the header next to the transaction summary, providing good visibility while maintaining a clean UI.
The conditional rendering based on
showSolanaTimerensures the timer only appears when needed, and the minimal variant keeps the UI clean.packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (2)
47-66: Add null check for selectedAccount.The
handleTryAgainSignTxfunction assumesselectedAccount.accountis always defined, but this might not be true. Add a null check to prevent potential runtime errors.const handleTryAgainSignTx = () => { - if (send.precomposedForm && send.precomposedTx) { + if (send.precomposedForm && send.precomposedTx && selectedAccount.account) { dispatch(sendFormActions.discardTransaction()); dispatch( signAndPushSendFormTransactionThunk({ formState: send.precomposedForm, precomposedTransaction: send.precomposedTx, selectedAccount: selectedAccount.account, }), ); } else if (stake.precomposedForm && stake.precomposedTx) { dispatch(stakeActions.dispose()); dispatch( signTransaction( stake.precomposedForm, stake.precomposedTx as PrecomposedTransactionFinal, ), ); } };
69-76: Good implementation of timestamp and tryAgainSignTx props.Passing the transaction timestamp and retry function to the content component properly implements the feature requirements.
The timestamp is correctly sourced from the precomposed transaction's
createdTimestamp, which will be used to calculate the deadline in the content component.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (2)
54-54:⚠️ Potential issueUpdate the Solana transaction timeout value.
The constant
SOLANA_TX_TIMEOUT_MSis set to 40 seconds (40,000 ms), which is significantly shorter than Solana's typical blockhash expiration time of 60-90 seconds. This could cause premature timeout of valid transactions.-const SOLANA_TX_TIMEOUT_MS = 40 * 1000; +const SOLANA_TX_TIMEOUT_MS = 60 * 1000; // Solana blockhashes typically expire after 60-90 seconds
282-288: 🛠️ Refactor suggestionAdd error handling for TrezorConnect.cancel.
The
handleTryAgainfunction doesn't properly handle potential errors that might occur when callingTrezorConnect.cancel().const handleTryAgain = (cancel: boolean) => { if (cancel) { - TrezorConnect.cancel('tx-timeout'); + try { + TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel TrezorConnect:', error); + } } tryAgainSignTx(); };
🧹 Nitpick comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
59-84: Simplify the shouldShowSolanaTimer function.The function has many conditions that make it hard to follow and maintain. Consider breaking it down into smaller, more focused helper functions.
const shouldShowSolanaTimer = ( networkType: NetworkType, deadline: number, outputs: ReviewOutput[], symbol: NetworkSymbol, accounts: AccountsState, buttonRequestsCount: number, serializedTx: SerializedTx | undefined, stakeType: StakeType | null, isTradingAction: boolean, ) => { if (networkType !== 'solana' || isSolanaBlockhashExpired(networkType, deadline)) { return false; } - const firstOutput = outputs[0]; - const isInternalTransfer = - firstOutput?.type === 'address' && - findAccountsByAddress(symbol, firstOutput.value, accounts).length > 0; - - const isFirstStep = buttonRequestsCount <= 1; - const isStaking = stakeType && !serializedTx; - const isTrading = isTradingAction && !serializedTx; - - return isInternalTransfer || !isFirstStep || serializedTx || isStaking || isTrading; + // Check if this is an internal transfer + const isInternalTransfer = isOutputInternalTransfer(outputs[0], symbol, accounts); + + // Check if we're past the first step of the transaction + const isPastFirstStep = buttonRequestsCount > 1; + + // Check for special transaction types + const isStaking = stakeType && !serializedTx; + const isTrading = isTradingAction && !serializedTx; + + // Show timer if any of these conditions are true + return isInternalTransfer || isPastFirstStep || serializedTx || isStaking || isTrading; }; // Helper function to determine if an output is an internal transfer const isOutputInternalTransfer = ( output: ReviewOutput | undefined, symbol: NetworkSymbol, accounts: AccountsState ): boolean => { return ( output?.type === 'address' && findAccountsByAddress(symbol, output.value, accounts).length > 0 ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- 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: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (4)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (4)
127-142: Improve race condition handling in useEffect cleanup.The cleanup function prevents the
TrezorConnect.cancel()call if the component is unmounted, but there's still a potential race condition during re-renders.useEffect(() => { const now = Date.now(); const timeLeft = Math.max(deadline - now, 0); let mounted = true; const timeoutId = setTimeout(() => { if (mounted && !isSending) { - TrezorConnect.cancel('tx-timeout'); + try { + TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel TrezorConnect:', error); + } } }, timeLeft); return () => { mounted = false; clearTimeout(timeoutId); }; }, [deadline, isSending]);
299-310: Good implementation of the retry flow for expired transactions.The BottomContent component now provides clear options for users when their Solana transaction has expired, allowing them to try again or close the modal.
363-365: Good implementation of the expired blockhash UI indicator.When a Solana blockhash expires, the component now properly displays the ExpiredBlockhash component to inform the user.
414-423: Good implementation of the timer display.The timer is well-integrated into the UI, displayed alongside the transaction summary when appropriate. The conditional rendering based on showSolanaTimer ensures it only appears when needed.
tomasklim
force-pushed
the
feat/solana-tx-modal-timer
branch
2 times, most recently
from
164a2fc to
c379a58
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
54-54: 🛠️ Refactor suggestionIncrease the Solana transaction timeout value.
The constant
SOLANA_TX_TIMEOUT_MSis set to 40 seconds (40,000 ms), which is below Solana's typical blockhash expiration window of 60-90 seconds. This may lead to premature expiration notifications.-const SOLANA_TX_TIMEOUT_MS = 40 * 1000; +const SOLANA_TX_TIMEOUT_MS = 60 * 1000;packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
47-66: 🛠️ Refactor suggestionAdd null check for selectedAccount.account.
The
handleTryAgainSignTxfunction assumesselectedAccount.accountis always defined, but this might not be true. Add a null check to prevent potential runtime errors.const handleTryAgainSignTx = () => { - if (send.precomposedForm && send.precomposedTx) { + if (send.precomposedForm && send.precomposedTx && selectedAccount.account) { dispatch(sendFormActions.discardTransaction()); dispatch( signAndPushSendFormTransactionThunk({ formState: send.precomposedForm, precomposedTransaction: send.precomposedTx, selectedAccount: selectedAccount.account, }), ); } else if (stake.precomposedForm && stake.precomposedTx) { dispatch(stakeActions.dispose()); dispatch( signTransaction( stake.precomposedForm, stake.precomposedTx as PrecomposedTransactionFinal, ), ); } };
🧹 Nitpick comments (2)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (2)
59-83: Simplify the shouldShowSolanaTimer function.The function has 9 parameters which makes it difficult to maintain. Consider refactoring it to accept an object parameter or break it down into smaller, more focused functions.
-const shouldShowSolanaTimer = ( - networkType: NetworkType, - deadline: number, - outputs: ReviewOutput[], - symbol: NetworkSymbol, - accounts: AccountsState, - buttonRequestsCount: number, - serializedTx: SerializedTx | undefined, - stakeType: StakeType | null, - isTrading: boolean, -) => { +type ShowTimerParams = { + networkType: NetworkType; + deadline: number; + outputs: ReviewOutput[]; + symbol: NetworkSymbol; + accounts: AccountsState; + buttonRequestsCount: number; + serializedTx: SerializedTx | undefined; + stakeType: StakeType | null; + isTrading: boolean; +}; + +const shouldShowSolanaTimer = ({ + networkType, + deadline, + outputs, + symbol, + accounts, + buttonRequestsCount, + serializedTx, + stakeType, + isTrading, +}: ShowTimerParams) => {
208-218: Update the shouldShowSolanaTimer function call.If you implement the suggested refactoring to use an object parameter, update the function call accordingly.
- const showSolanaTimer = shouldShowSolanaTimer( - networkType, - deadline, - outputs, - symbol, - accounts, - buttonRequestsCount, - serializedTx, - stakeType, - isTradingAction, - ); + const showSolanaTimer = shouldShowSolanaTimer({ + networkType, + deadline, + outputs, + symbol, + accounts, + buttonRequestsCount, + serializedTx, + stakeType, + isTrading: isTradingAction, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/blockchain-link/src/workers/solana/index.ts(2 hunks)packages/components/src/components/Badge/Badge.stories.tsx(1 hunks)packages/components/src/components/Badge/Badge.tsx(4 hunks)packages/suite-data/files/translations/en.json(1 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts(2 hunks)packages/suite/src/actions/wallet/stakeActions.ts(3 hunks)packages/suite/src/components/suite/CountdownTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(10 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(5 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/wallet-core/src/send/sendFormReducer.ts(1 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(3 hunks)suite-common/wallet-core/src/send/sendFormTypes.ts(1 hunks)suite-common/wallet-core/src/stake/stakeReducer.ts(1 hunks)suite-common/wallet-types/src/transaction.ts(2 hunks)suite-native/module-send/src/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- suite-common/wallet-core/src/stake/stakeReducer.ts
- packages/suite/src/components/suite/CountdownTimer.tsx
- suite-common/wallet-types/src/transaction.ts
- packages/urls/src/urls.ts
- suite-common/wallet-core/src/send/sendFormTypes.ts
- suite-native/module-send/src/sendFormThunks.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx
- packages/suite/src/actions/wallet/stakeActions.ts
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- suite-common/wallet-core/src/send/sendFormThunks.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx
- suite-common/wallet-core/src/send/sendFormReducer.ts
- packages/blockchain-link/src/workers/solana/index.ts
- packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx
- packages/components/src/components/Badge/Badge.stories.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
- packages/suite-data/files/translations/en.json
- packages/components/src/components/Badge/Badge.tsx
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: test
🔇 Additional comments (7)
packages/suite/src/support/messages.ts (2)
8912-8914: Looks good! Link reference removed.The link referring to "Learn more" has been correctly removed from the default message for
TR_STAKE_ETH_WILL_BE_BLOCKED. This makes the error message cleaner while still conveying the essential information.
9151-9176: New Solana transaction timeout messages added correctly.These new messages effectively support the timer feature for Solana transactions as described in the PR objectives. The messages provide:
- Clear error messaging when transactions fail due to timeout
- Timer display formats (both standard and short versions)
- An informative description about the time constraints
- A retry button label
The implementation properly handles the UI needs for the countdown timer and retry functionality when Solana transactions expire.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (4)
136-154: Approved: Good error handling in the useEffect timer.The implementation properly handles component unmounting with a
mountedflag to prevent callingTrezorConnect.cancelafter the component is unmounted, and also checks if the transaction is already being sent.
284-290: Add error handling for TrezorConnect.cancel.The
handleTryAgainfunction should handle potential errors fromTrezorConnect.cancel().const handleTryAgain = (cancel: boolean) => { if (cancel) { - TrezorConnect.cancel('tx-timeout'); + try { + TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel TrezorConnect:', error); + } } tryAgainSignTx(); };
301-312: Approved: Good UI for expired blockhash.The UI now clearly shows appropriate actions when the blockhash has expired, including a retry button and options to close the modal. This provides a good user experience.
416-425: Approved: Good implementation of timer in the modal description.The timer is conditionally displayed based on the transaction state, and is correctly positioned to be visible to the user. The minimal flag ensures it doesn't take up too much space.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
70-74: Approved: Good update to TransactionReviewModalContent props.The necessary props
timestampandtryAgainSignTxare now correctly passed to the TransactionReviewModalContent component, enabling the timer and retry functionality.
tomasklim
force-pushed
the
feat/solana-tx-modal-timer
branch
from
c379a58 to
e68433f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
54-54: 🛠️ Refactor suggestionUpdate Solana Transaction Timeout Value.
The constant
SOLANA_TX_TIMEOUT_MSis currently set to 40,000 ms (40 seconds), which is below Solana's typical blockhash expiration window of 60–90 seconds. This could cause premature expiration of transactions.Consider increasing this value to at least 60,000 ms (60 seconds) to align with Solana's minimum blockhash validity period.
-const SOLANA_TX_TIMEOUT_MS = 40 * 1000; +const SOLANA_TX_TIMEOUT_MS = 60 * 1000;packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
47-66:⚠️ Potential issueAdd null check for selectedAccount in retry function.
The
handleTryAgainSignTxfunction assumesselectedAccount.accountis always defined when dispatching the send form transaction. This could cause runtime errors if the account is not available.const handleTryAgainSignTx = () => { - if (send.precomposedForm && send.precomposedTx) { + if (send.precomposedForm && send.precomposedTx && selectedAccount.account) { dispatch(sendFormActions.discardTransaction()); dispatch( signAndPushSendFormTransactionThunk({ formState: send.precomposedForm, precomposedTransaction: send.precomposedTx, selectedAccount: selectedAccount.account, }), ); } else if (stake.precomposedForm && stake.precomposedTx) { dispatch(stakeActions.dispose()); dispatch( signTransaction( stake.precomposedForm, stake.precomposedTx as PrecomposedTransactionFinal, ), ); } };
🧹 Nitpick comments (2)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx (1)
30-31: New props for Solana transaction timeout handling.You've added two new props to support the Solana transaction timer functionality:
deadline(optional): Represents the expiration timestamp for the transactiononTryAgain(required): Callback for retrying when a transaction expiresThe
onTryAgainprop should probably be optional since it's only needed for Solana transactions, butdeadlineis already properly marked as optional.export type TransactionReviewOutputListProps = { account: Account; precomposedTx: GeneralPrecomposedTransactionFinal; signedTx?: { tx: string }; outputs: ReviewOutput[]; buttonRequestsCount: number; isRbfAction: boolean; isTradingAction: boolean; isSending?: boolean; stakeType?: StakeType; deadline?: number; - onTryAgain: (close: boolean) => void; + onTryAgain?: (close: boolean) => void; };packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
59-83:❓ Verification inconclusive
Complex condition for showing Solana timer.
The
shouldShowSolanaTimerfunction contains complex conditional logic that determines when to show the timer. While comprehensive, the complexity may make it difficult to maintain.Consider simplifying this logic or adding comments to explain the reasoning behind each condition. Also, the TODO comment about whether to use stake type or safe address has been addressed by checking both conditions.
🏁 Script executed:
#!/bin/bash # Look for similar implementations that might have clearer patterns rg -A 3 -B 3 "showTimer|shouldShow" --type tsx --type ts packages/suite/src/componentsLength of output: 116
Simplify and Document Complex Conditional in
shouldShowSolanaTimerThe conditional logic here is quite intricate, which can make it hard to understand and maintain. Since similar patterns couldn’t be readily discovered in the codebase (and the initial regex search encountered issues with the TSX file type), please verify manually that this logic is unique and necessary. In the meantime, consider the following enhancements:
- Refactor or Comment: Break down the condition into smaller helper functions or add inline comments for clarity on why each condition (e.g.,
isTrading,isInternalTransfer, andisStaking) is required.- Safety Check: Confirm that checking both
stakeTypeandserializedTxaccurately reflects the intended behavior, as the previous TODO has been resolved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/blockchain-link/src/workers/solana/index.ts(2 hunks)packages/components/src/components/Badge/Badge.stories.tsx(1 hunks)packages/components/src/components/Badge/Badge.tsx(4 hunks)packages/suite-data/files/translations/en.json(1 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts(2 hunks)packages/suite/src/actions/wallet/stakeActions.ts(3 hunks)packages/suite/src/components/suite/CountdownTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(10 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(5 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(4 hunks)suite-common/wallet-core/src/send/sendFormTypes.ts(1 hunks)suite-common/wallet-core/src/stake/stakeReducer.ts(1 hunks)suite-common/wallet-types/src/transaction.ts(2 hunks)suite-native/module-send/src/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- suite-native/module-send/src/sendFormThunks.ts
- packages/urls/src/urls.ts
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- packages/components/src/components/Badge/Badge.stories.tsx
- suite-common/wallet-core/src/stake/stakeReducer.ts
- packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts
- packages/suite/src/components/suite/CountdownTimer.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredBlockhash.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx
- packages/suite/src/actions/wallet/stakeActions.ts
- packages/blockchain-link/src/workers/solana/index.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx
- packages/suite-data/files/translations/en.json
- suite-common/wallet-core/src/send/sendFormTypes.ts
- suite-common/wallet-types/src/transaction.ts
- packages/components/src/components/Badge/Badge.tsx
- packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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: EAS Update
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: test
🔇 Additional comments (11)
suite-common/wallet-core/src/send/sendFormThunks.ts (4)
67-67: Good addition of a specialized error type.Adding a dedicated error type for transaction timeouts creates a clearer distinction between different transaction failure scenarios.
403-403: Proper type signature update.The function's reject value type has been correctly updated to include the new timeout error type, maintaining type safety throughout the application.
461-467: Good error handling enhancement for timeouts.Adding specific handling for the 'tx-timeout' case allows the UI to differentiate between user-initiated cancellations and timeout situations. This is essential for displaying appropriate error messages and enabling the retry functionality mentioned in the PR objectives.
588-591: Good addition of timestamp for expiration tracking.Adding a
createdTimestampto the transaction object enables the countdown timer functionality described in the PR objectives. This timestamp will allow the UI to calculate and display the remaining time before a transaction expires.packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx (2)
117-129: Solana timer integration.The timer component is now conditionally rendered for Solana transactions when a deadline is provided. The layout has been modified to accommodate this new component with appropriate spacing.
The nested Column structure provides good separation of concerns and maintains proper spacing between elements.
88-88:❓ Verification inconclusive
Logic change in isFirstStep condition.
The condition for determining
isFirstStephas been changed from checking ifbuttonRequestsCountequals 1 to checking if it's less than or equal to 1. This means that abuttonRequestsCountof 0 will now also be considered a first step.Make sure this change is intentional and won't affect other features that depend on this logic.
🏁 Script executed:
#!/bin/bash # Find other usages of isFirstStep to ensure this change doesn't break existing functionality rg -A 2 -B 2 "isFirstStep" --type tsx --type ts packages/suiteLength of output: 90
I'll perform a re-verification using a revised search command that correctly includes TSX files. Please run the following script:
Once you confirm that this search shows no unexpected usages or side effects from the change (i.e. treating a
buttonRequestsCountof 0 as a first step), then the updated logic is acceptable. If any dependency or assumption is found that relies strictly onbuttonRequestsCount === 1, please review and adjust accordingly.Attention: Validate the new isFirstStep logic change
- The updated condition now treats both 0 and 1 as indicating the first step.
- Confirm that no feature relies on
buttonRequestsCountbeing exactly 1.- If potential side effects are discovered, ensure that the change is intentional and safe.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run the search for all occurrences of "isFirstStep" in TS & TSX files using iglob. rg -A 2 -B 2 "isFirstStep" packages/suite --iglob "*.tsx" --iglob "*.ts"Length of output: 3101
Attention: Revisit the isFirstStep logic change
We verified that both occurrences of the
isFirstStepflag—in
•packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsxand
•packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsxnow use the condition
buttonRequestsCount <= 1. This means that abuttonRequestsCountof 0 will be treated as a first step. As the search revealed no additional usages that might depend strictly onbuttonRequestsCount === 1, this change appears consistent across the codebase.Please confirm that treating a value of 0 as a first step is intentional and acceptable for your business logic. If any feature relies on the previous strict condition, additional adjustments might be needed.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (4)
136-154: Improved transaction expiration handling with proper cleanup.The implementation properly handles Solana transaction timeouts with several good practices:
- Calculates the remaining time correctly
- Tracks component mount state to prevent calling functions after unmount
- Properly cleans up the timeout on unmount
- Skips timer setup for trading actions
This prevents memory leaks and ensures the timeout behaves correctly in all component lifecycle scenarios.
284-290: Add error handling for TrezorConnect.cancel.The
handleTryAgainfunction should handle potential errors fromTrezorConnect.cancel().const handleTryAgain = (cancel: boolean) => { if (cancel) { - TrezorConnect.cancel('tx-timeout'); + try { + TrezorConnect.cancel('tx-timeout'); + } catch (error) { + console.error('Failed to cancel TrezorConnect:', error); + } } tryAgainSignTx(); };
301-312: Good UX for expired transactions.The added retry flow for expired Solana transactions provides a clear path forward for users when a transaction times out. This implementation:
- Shows appropriate action buttons
- Provides both retry and close options
- Maintains consistency with the rest of the modal patterns
This enhances the user experience by reducing frustration when transactions expire.
416-425: Clean implementation of minimal timer in header.The timer is conditionally rendered in the header area when appropriate, using the minimal variant to save space. The Row component with proper spacing ensures the timer is visually separated from other header elements.
This provides users with time awareness without cluttering the interface.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
69-76: Clean integration with TransactionReviewModalContent.The component now properly passes:
- The transaction timestamp for deadline calculation
- The newly created retry function
This provides all necessary data for the timer functionality while maintaining clean component interfaces.
...ts/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx
Show resolved
Hide resolved
...ite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
48-57:⚠️ Potential issueAdd null check for
selectedAccount.account.The function assumes
selectedAccount.accountis always defined, but this might not be true. Add a null check to prevent potential runtime errors.const handleTryAgainSignTx = () => { - if (send.precomposedForm && send.precomposedTx) { + if (send.precomposedForm && send.precomposedTx && selectedAccount.account) { dispatch(sendFormActions.discardTransaction()); dispatch( signAndPushSendFormTransactionThunk({ formState: send.precomposedForm, precomposedTransaction: send.precomposedTx, selectedAccount: selectedAccount.account, }), ); } else if (stake.precomposedForm && stake.precomposedTx) {
🧹 Nitpick comments (3)
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredTxValidity.tsx (2)
16-32: UI component structure looks good, but help center link is hardcoded for Solana.The component currently uses
HELP_CENTER_SOL_SENDfor all network types, but the component accepts anyNetworkSymbol. This might lead to incorrect help resources being linked for non-Solana networks.Consider making the help center link dynamic based on the network symbol:
- <TrezorLink typographyStyle="hint" href={HELP_CENTER_SOL_SEND} icon="arrowUpRight"> + <TrezorLink + typographyStyle="hint" + href={symbol === 'sol' ? HELP_CENTER_SOL_SEND : getHelpCenterLinkByNetwork(symbol)} + icon="arrowUpRight">You would need to implement a
getHelpCenterLinkByNetworkfunction that returns the appropriate help center link for each supported network.
28-30: Missing aria-label for accessibility.The link to the help center would benefit from an accessible label that describes its purpose more explicitly than just "Learn more".
- <TrezorLink typographyStyle="hint" href={HELP_CENTER_SOL_SEND} icon="arrowUpRight"> + <TrezorLink + typographyStyle="hint" + href={HELP_CENTER_SOL_SEND} + icon="arrowUpRight" + aria-label={`Learn more about transaction failures on ${networkName}`}> <Translation id="TR_LEARN_MORE" /> </TrezorLink>packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
62-62: Use a type guard instead of type casting.Type casting
stake.precomposedTx as PrecomposedTransactionFinalcould be risky if the actual type doesn't match. Consider adding a type guard for better type safety.- stake.precomposedTx as PrecomposedTransactionFinal, + stake.precomposedTx && 'fee' in stake.precomposedTx ? stake.precomposedTx : null,Add appropriate error handling if the transaction is not of the expected type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(10 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredTxValidity.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: test
🔇 Additional comments (12)
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredTxValidity.tsx (4)
1-8: Clean imports with good organization.The imports are well-organized, separating external library imports from internal component imports. I appreciate the use of named imports for clarity.
9-11: Type definition is clear and concise.The props interface is well-defined with appropriate typing for the network symbol.
13-15: Consider adding null checking for network retrieval.When retrieving the network name, there's no validation to ensure the network exists. If
getNetworkdoesn't find a match for the provided symbol, accessing.namemight cause runtime errors.- const networkName = getNetwork(symbol).name; + const network = getNetwork(symbol); + const networkName = network ? network.name : symbol;
1-34: Consider adding unit tests for this component.This component plays an important role in the error handling flow. Adding unit tests would help ensure it renders correctly and handles different network symbols appropriately.
Do you have plans to add tests for this component? I'd be happy to assist with generating test cases if needed.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx (1)
6-6: Good refactoring to use a shared selector.Replacing the inline selector with the imported
selectAccountsselector improves code organization and maintainability. Using shared selectors across the codebase ensures consistent access to Redux state and makes the component more resilient to changes in the state structure.Also applies to: 249-249
packages/suite/src/support/messages.ts (4)
8911-8914: LGTM: Message wording updated to correctly include formatting.The change in this message ("Your {networkDisplaySymbol} will be blocked...") corrects the format to use tilde (~) which is needed for the localization system to properly handle the message.
9151-9159: Informative error message for transaction timeouts.These new messages clearly explain what happens when a transaction fails due to timeout. The error message correctly includes the network name as a variable.
9160-9167: Countdown timer messages properly implemented.The timer messages are well designed to show the remaining time in both standard and short formats. The short format includes HTML formatting for emphasis.
9168-9172: Informative description of Solana's transaction time constraints.This message correctly explains the time limit constraint for Solana transactions, informing users they have less than 1 minute to confirm and send transactions before timeout.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (3)
32-32: Consider using theselectSelectedAccountselector for consistency.Based on the previous comment, it would be better to use the dedicated
selectSelectedAccountselector instead of directly accessing the state withuseSelector. This would maintain consistency in how selectors are used throughout the codebase.- const selectedAccount = useSelector(state => state.wallet.selectedAccount); + const selectedAccount = useSelector(selectSelectedAccount);
72-72: The implementation aligns with the PR objectives.Adding the
tryAgainSignTxprop to theTransactionReviewModalContentis consistent with the PR's objective of adding retry functionality for expired Solana transactions.
47-66:✅ Verification successful
Implement timeout handling for Solana transactions.
Based on the PR objectives, this modal needs to handle Solana transaction timeouts and allow retrying when the blockhash expires. The current implementation is missing specific handling for Solana network transactions.
🏁 Script executed:
#!/bin/bash # Check if there are Solana-specific conditions or network checks in the dependent components # Look for Solana network checks in related files rg -i "solana" -A 5 -B 5 "packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx" "packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx" "packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx"Length of output: 10723
Solana Timeout Handling is Properly Implemented
The Solana-specific timeout logic is present in the modal’s content layer. We now see that:
- The
getTxValidityTimeoutInMsfunction inTransactionReviewModalContent.tsxreturns a 40-second timeout for Solana transactions.- The
<TransactionReviewOutputTimer>component is conditionally rendered for Solana transactions, using translations (e.g.,TR_SOLANA_TX_CONFIRMATION_TIMER_DESCRIPTION) to inform the user.- The retry logic in
handleTryAgainSignTxis generic and appropriately re-dispatches actions for both send and stake flows.No additional modifications are needed in
TransactionReviewModal.tsxregarding Solana timeout handling. Please verify in manual testing that the end-to-end behavior (display of the timer and the retry flow when a blockhash expires) meets all requirements.
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
| import { ReplaceByFeeFailedOriginalTxConfirmed } from '../UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed'; | ||
|
|
||
| const getTxValidityTimeoutInMs = (networkType?: NetworkType) => { | ||
| if (networkType === 'solana') { | ||
| return 40 * 1000; // 40 seconds |
There was a problem hiding this comment.
Please add the reason why 40s for the future
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Show resolved
Hide resolved
tomasklim
force-pushed
the
feat/solana-tx-modal-timer
branch
from
b5eca1d to
cfaefd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
47-57:⚠️ Potential issueAdd null check for selectedAccount.
The
handleTryAgainSignTxfunction doesn't verify thatselectedAccount.accountexists before using it. This could lead to runtime errors if the selected account is not defined.const handleTryAgainSignTx = () => { - if (send.precomposedForm && send.precomposedTx) { + if (send.precomposedForm && send.precomposedTx && selectedAccount.account) { dispatch(sendFormActions.discardTransaction()); dispatch( signAndPushSendFormTransactionThunk({ formState: send.precomposedForm, precomposedTransaction: send.precomposedTx, selectedAccount: selectedAccount.account, }), );
🧹 Nitpick comments (3)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (2)
27-56: Consider adding aria attributes for improved accessibility.The minimal view implementation looks good, with appropriate conditional rendering based on the sending state. For better accessibility, consider adding aria attributes to the timer display.
<TimerBox> + <span aria-live="polite" role="status"> {isSending ? ( <Translation id="TR_CONFIRMING_TX" /> ) : ( <CountdownTimer deadline={deadline} unitDisplay="narrow" message="TR_TX_CONFIRMATION_TIMER_SHORT" pastDeadlineMessage="TR_TX_SEND_FAILED_TITLE" /> )} + </span> </TimerBox>
58-79: Consider adding context for retry action.The standard view implementation works well with the banner component. Consider providing additional context about what happens when the user clicks retry to help with user understanding.
<Banner icon="hourglass" rightContent={ - <Banner.Button isDisabled={isSending} onClick={() => onTryAgain(true)}> + <Banner.Button + isDisabled={isSending} + onClick={() => onTryAgain(true)} + aria-label={isSending ? "Retry not available while sending" : "Retry transaction"} + > <Translation id="TR_RETRY" /> </Banner.Button> } >packages/suite/src/support/messages.ts (1)
9168-9172: Consider being more specific about timer duration.The PR objectives mention a 40-second timer, but this message states "less than 1 minute". For better accuracy, consider updating to specify "about 40 seconds" to match the actual implementation.
- 'Due to Solana network constraints you have <1 minute to confirm and send the transaction before it times out.', + 'Due to Solana network constraints you have about 40 seconds to confirm and send the transaction before it times out.',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/blockchain-link/src/workers/solana/index.ts(2 hunks)packages/components/src/components/Badge/Badge.stories.tsx(1 hunks)packages/components/src/components/Badge/Badge.tsx(4 hunks)packages/suite-data/files/translations/en.json(1 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts(2 hunks)packages/suite/src/actions/wallet/stakeActions.ts(3 hunks)packages/suite/src/components/suite/CountdownTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(10 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(5 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredTxValidity.tsx(1 hunks)packages/suite/src/support/messages.ts(2 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(4 hunks)suite-common/wallet-core/src/send/sendFormTypes.ts(1 hunks)suite-common/wallet-core/src/stake/stakeReducer.ts(1 hunks)suite-common/wallet-types/src/transaction.ts(2 hunks)suite-native/module-send/src/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/components/src/components/Badge/Badge.stories.tsx
- packages/urls/src/urls.ts
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- packages/suite/src/components/suite/CountdownTimer.tsx
- suite-native/module-send/src/sendFormThunks.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredTxValidity.tsx
- packages/suite/src/actions/wallet/stakeActions.ts
- suite-common/wallet-core/src/stake/stakeReducer.ts
- suite-common/wallet-core/src/send/sendFormTypes.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
- packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx
- packages/blockchain-link/src/workers/solana/index.ts
- packages/suite-data/files/translations/en.json
- packages/components/src/components/Badge/Badge.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
- suite-common/wallet-types/src/transaction.ts
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- 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-web
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (13)
suite-common/wallet-core/src/send/sendFormThunks.ts (4)
67-67: Added new error type import for handling transaction timeouts.The addition of
SignTransactionTimeoutErrorimport is necessary to support the new timeout handling capability.
403-403: Enhanced type definition to include timeout errors.The reject value type is properly updated to include the new
SignTransactionTimeoutErrortype, ensuring type safety when handling timeout scenarios.
461-473: Improved error handling with specific timeout detection.The new conditional block properly handles the 'tx-timeout' case separately from 'tx-cancelled', allowing for more granular error handling and user feedback. This enables the implementation of the retry functionality when blockhash expires.
588-591: Added timestamp tracking for transaction expiration.The addition of
createdTimestampto the precomposed transaction is essential for the countdown timer feature, allowing the UI to calculate and display the remaining time until transaction expiration.packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (3)
10-12: Looks good, nice use of tabular numbers for consistent width.Using
font-variant-numeric: tabular-numsis a good practice for displaying timer digits to prevent layout shifts as numbers change.
14-19: Props are well structured and documented.The props interface is clear with appropriate required/optional designations. The
onTryAgaincallback with a boolean parameter provides flexibility for handling retry actions.
67-77:❓ Verification inconclusive
Consider handling boundary cases for the timer.
The implementation looks good. Consider adding handling for edge cases where the deadline might be in the past when the component first renders.
🏁 Script executed:
#!/bin/bash # Search for CountdownTimer implementation to understand how it handles deadlines in the past rg -A 10 'export const CountdownTimer' --type=tsxLength of output: 79
I'll run an updated search command without relying on the
--type=tsxflag to verify theCountdownTimerimplementation. Please execute the following script:
🏁 Script executed:
#!/bin/bash # Search for CountdownTimer implementation in all TSX files using a glob pattern rg -A 10 'export const CountdownTimer' --iglob '*.tsx'Length of output: 952
Verify Timer's Handling of Past Deadlines
The current implementation of the
CountdownTimercomponent inpackages/suite/src/components/suite/CountdownTimer.tsxrelies on theuseCountdownTimerhook, which returns anisPastDeadlineflag based on the provided deadline. However, it's not clear from the code whether the component immediately displays thepastDeadlineMessagewhen the deadline is already in the past on initial render. Please verify that this boundary case is properly handled, and if needed, update the component logic to account for an already expired deadline.packages/suite/src/support/messages.ts (3)
8913-8913: The "Learn more" link has been removed as suggested.This line now removes the hyperlink from the message, which was discussed in previous comments.
9151-9159: New error messages for transaction failure added successfully.These new messages clearly explain to users why a transaction failed to send due to timeout constraints, improving the error handling experience.
9160-9167: Timer display messages implemented correctly.These messages effectively show the remaining time for transaction confirmation, with both standard and compact versions available for different UI contexts.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (3)
32-32: Consider using a dedicated selector for selectedAccount.Instead of directly accessing state with
state => state.wallet.selectedAccount, consider using theselectSelectedAccountselector for better maintainability and consistency with the codebase patterns.
57-65: Add type assertion check for precomposedTx.The
stake.precomposedTxis directly cast toPrecomposedTransactionFinalwithout verification, which could lead to runtime errors if the actual type doesn't match.- } else if (stake.precomposedForm && stake.precomposedTx) { + } else if (stake.precomposedForm && stake.precomposedTx && 'fee' in stake.precomposedTx) { dispatch(stakeActions.dispose()); dispatch( signTransaction( stake.precomposedForm, stake.precomposedTx as PrecomposedTransactionFinal, ), ); }
72-72: Excellent addition of the retry functionality.Adding the
tryAgainSignTxprop to the TransactionReviewModalContent component properly implements the retry functionality for transaction signing, which is particularly useful for Solana transactions where blockhash can expire.
tomasklim
force-pushed
the
feat/solana-tx-modal-timer
branch
from
cfaefd0 to
4997254
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
47-66:⚠️ Potential issueAdd null check for selectedAccount.
The
handleTryAgainSignTxfunction assumesselectedAccount.accountis always defined when accessing it on line 54, but this might not be true. Add a null check to prevent potential runtime errors.const handleTryAgainSignTx = () => { - if (send.precomposedForm && send.precomposedTx) { + if (send.precomposedForm && send.precomposedTx && selectedAccount.account) { dispatch(sendFormActions.discardTransaction()); dispatch( signAndPushSendFormTransactionThunk({ formState: send.precomposedForm, precomposedTransaction: send.precomposedTx, selectedAccount: selectedAccount.account, }), ); } else if (stake.precomposedForm && stake.precomposedTx) { dispatch(stakeActions.dispose()); dispatch( signTransaction( stake.precomposedForm, stake.precomposedTx as PrecomposedTransactionFinal, ), ); } };
🧹 Nitpick comments (6)
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredTxValidity.tsx (1)
16-33: Consider adding an aria-label for better accessibility.The component UI structure looks good with proper spacing and visual hierarchy. For better accessibility, consider adding an aria-label to the IconCircle or wrapping it in a semantic element with appropriate aria attributes to ensure screen readers properly announce the warning state.
- <IconCircle name="warning" size={110} variant="destructive" /> + <IconCircle + name="warning" + size={110} + variant="destructive" + aria-label={`${networkName} transaction failed warning`} + />packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (3)
14-19: Props interface is well-defined but could be enhanced.The
TransactionReviewOutputTimerPropsinterface is clear and properly typed. However, the purpose of thecloseparameter inonTryAgaincould be more explicit in the type definition with a descriptive name likeshouldCloseModal.type TransactionReviewOutputTimerProps = { deadline: number; isMinimal?: boolean; - onTryAgain: (close: boolean) => void; + onTryAgain: (shouldCloseModal: boolean) => void; isSending?: boolean; };
27-56: Consider memoizing callback functions to prevent unnecessary renders.The onClick handler creates a new function on each render. Consider using useCallback to memoize this function.
import React from 'react'; +import React, { useCallback } from 'react'; // ... export const TransactionReviewOutputTimer = ({ deadline, isMinimal, onTryAgain, isSending, }: TransactionReviewOutputTimerProps) => { + const handleRetry = useCallback(() => onTryAgain(true), [onTryAgain]); if (isMinimal) { return ( <> <Button icon="arrowClockwise" variant="tertiary" type="button" size="tiny" isDisabled={isSending} - onClick={() => onTryAgain(true)} + onClick={handleRetry} >
1-80: Consider adding React.memo to optimize rendering performance.This component might re-render frequently as the timer updates. Wrapping it with React.memo could help optimize performance by preventing unnecessary re-renders when props haven't changed.
import React from 'react'; // ...other imports export const TransactionReviewOutputTimer = ({ deadline, isMinimal, onTryAgain, isSending, }: TransactionReviewOutputTimerProps) => { // Component implementation }; +export const MemoizedTransactionReviewOutputTimer = React.memo(TransactionReviewOutputTimer);Then export and use the memoized version in parent components.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (2)
32-32: Use the dedicated selector instead of direct state access.Consider using the
selectSelectedAccountselector that was mentioned in the previous review comment instead of directly accessing the state withstate.wallet.selectedAccount.- const selectedAccount = useSelector(state => state.wallet.selectedAccount); + const selectedAccount = useSelector(selectSelectedAccount);Note: You'll need to import the
selectSelectedAccountselector.
57-65: Validate stake.precomposedTx type before casting.Instead of directly casting
stake.precomposedTxtoPrecomposedTransactionFinal, consider adding a validation check or using a type guard to ensure the object is of the expected type. This prevents potential runtime issues if the object structure doesn't match.} else if (stake.precomposedForm && stake.precomposedTx) { dispatch(stakeActions.dispose()); + // Ensure stake.precomposedTx is of the expected type + const precomposedTx = stake.precomposedTx as PrecomposedTransactionFinal; dispatch( signTransaction( stake.precomposedForm, - stake.precomposedTx as PrecomposedTransactionFinal, + precomposedTx, ), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/blockchain-link/src/workers/solana/index.ts(3 hunks)packages/components/src/components/Badge/Badge.stories.tsx(1 hunks)packages/components/src/components/Badge/Badge.tsx(4 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts(2 hunks)packages/suite/src/actions/wallet/stakeActions.ts(3 hunks)packages/suite/src/components/suite/CountdownTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(10 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(5 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredTxValidity.tsx(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/urls/src/urls.ts(1 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(4 hunks)suite-common/wallet-core/src/send/sendFormTypes.ts(1 hunks)suite-common/wallet-core/src/stake/stakeReducer.ts(1 hunks)suite-common/wallet-types/src/transaction.ts(2 hunks)suite-native/module-send/src/sendFormThunks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/urls/src/urls.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx
- packages/suite/src/components/suite/CountdownTimer.tsx
- suite-common/wallet-core/src/stake/stakeReducer.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/StakeModal/StakeEthForm/ConfirmStakeEthModal.tsx
- suite-common/wallet-types/src/transaction.ts
- suite-native/module-send/src/sendFormThunks.ts
- packages/components/src/components/Badge/Badge.stories.tsx
- packages/suite/src/actions/wallet/stakeActions.ts
- suite-common/wallet-core/src/send/sendFormTypes.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- packages/components/src/components/Badge/Badge.tsx
- packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts
- packages/blockchain-link/src/workers/solana/index.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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: EAS Update
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (15)
suite-common/wallet-core/src/send/sendFormThunks.ts (4)
67-67: Added new error type for transaction timeoutsThis import brings in the
SignTransactionTimeoutErrortype which is used for handling transaction timeout scenarios. Good addition that aligns with the countdown timer feature being implemented.
403-403: Updated reject value type to include timeout errorsThe function signature now properly includes the timeout error type in its reject value types, ensuring type safety when handling timeout scenarios.
461-473: Enhanced error handling for transaction timeoutsThe new conditional check for 'tx-timeout' adds proper error handling for transaction timeouts, allowing the UI to show a retry button when a transaction times out instead of treating it as a general failure.
This implementation properly distinguishes between user-initiated cancellations and timeout failures, providing a better user experience by enabling specific error handling flows for each case.
588-591: Added timestamp to track transaction creation timeAdding a creation timestamp to the precomposed transaction enables tracking how long the transaction has been pending, which is essential for the countdown timer functionality.
This timestamp will allow you to calculate the remaining time until the transaction expires (40 seconds as mentioned in the PR description), providing a better user experience.
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ExpiredTxValidity.tsx (5)
1-8: Good use of modular imports.The imports are well organized with clear separation between external dependencies and internal components. The dedicated import for the Solana help center URL suggests good organization of constants.
9-11: Type definition looks good.The props interface is concise and properly typed with NetworkSymbol, which helps ensure type safety throughout the component.
13-15: Well-structured component with proper network name retrieval.The component correctly uses the getNetwork utility to dynamically retrieve the network name based on the symbol prop.
28-31: Good integration with help resources.The component provides a clear path for users to learn more about the issue, using the TrezorLink component with appropriate styling and icon.
1-34:❓ Verification inconclusive
Verify the dynamic text with different network names.
The component correctly uses the networkName in the description, but it would be good to verify that the translation strings handle different network names gracefully, especially for networks with longer names that might affect layout.
🏁 Script executed:
#!/bin/bash # Look for the translation keys to verify their content rg "TR_TX_SEND_FAILED_TITLE|TR_TX_SEND_FAILED_DESCRIPTION" --type jsonLength of output: 72
I'll run another script that searches for occurrences of both keys across all file types (not just JSON) to locate the corresponding translation strings. This should help us verify if the dynamic text (network names) is provided with sufficient flexibility in the translations.
Once we have the output of this search, we can evaluate whether the translations are set up to gracefully handle different network names, including longer ones.
🏁 Script executed:
#!/bin/bash # Search for translation keys in the entire repository rg "TR_TX_SEND_FAILED_TITLE|TR_TX_SEND_FAILED_DESCRIPTION"Length of output: 1163
Action Required: Manually Verify Layout with Long Network Names
The dynamic text is correctly injected through the translation key (TR_TX_SEND_FAILED_DESCRIPTION) using the providednetworkNamevalue. We located these keys (in both the component and the messages file) and confirmed that they are referenced consistently. However, since we rely on the translation strings to handle variable substitutions (especially for networks with longer names), please manually verify in the UI that the layout gracefully accommodates these variations without breaking the design.packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputTimer.tsx (2)
40-53: Good use of conditional rendering in the Badge component.The Badge component with the warning variant effectively communicates the time-sensitive nature of the transaction. The conditional rendering based on
isSendingstate provides clear feedback to the user.
59-79:❓ Verification inconclusive
Banner implementation provides clear user guidance.
The Banner component effectively communicates the transaction status with appropriate icon and action button. The use of Typography styles and longer descriptions in this view provides better context for users. The translation ID "TR_SOLANA_TX_CONFIRMATION_TIMER_DESCRIPTION" should have a matching entry in the translation files.
🏁 Script executed:
#!/bin/bash # Check if the translation key exists in the translation files rg "TR_SOLANA_TX_CONFIRMATION_TIMER_DESCRIPTION" --type jsonLength of output: 60
Action Required: Verify Translation Key Presence in Translation Files
The Banner component is implemented correctly and clearly communicates the transaction status. However, our automated search did not confirm the existence of the translation key "TR_SOLANA_TX_CONFIRMATION_TIMER_DESCRIPTION" in the JSON translation files. Please manually verify that this key is present or add it if missing to ensure proper translations in production.
- Confirm that "TR_SOLANA_TX_CONFIRMATION_TIMER_DESCRIPTION" exists in your translation JSON files.
- If missing, add the appropriate translation entry.
packages/suite/src/support/messages.ts (3)
9152-9160: Well-structured error message for failed transactions.The added message definitions provide clear explanations about transaction timeouts, which is critical for Solana transactions. The error message properly informs users that their transaction failed due to time constraints, with appropriate placeholders for the network name.
9161-9168: Good implementation of countdown timer messages.These messages provide both standard and emphasized variants for displaying the remaining time to confirm a transaction. The short version with HTML emphasis is particularly helpful for drawing user attention to the time constraint.
9169-9173: Clear explanation of Solana network constraints.This message effectively communicates the specific time limitations imposed by the Solana network, setting appropriate user expectations. This directly addresses the PR objective of adding a countdown timer for Solana transaction confirmation.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)
72-72: LGTM - The new function is properly passed as a prop.The
handleTryAgainSignTxfunction is correctly passed to theTransactionReviewModalContentcomponent as thetryAgainSignTxprop.
tomasklim
force-pushed
the
feat/solana-tx-modal-timer
branch
from
4997254 to
b3e479e
Compare
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13567675397 |
trezor-ci
force-pushed
the
feat/solana-tx-modal-timer
branch
from
a85d461 to
6158765
Compare
Description
A countdown timer has been added to show the remaining time for transaction confirmation. If the blockhash expires, a retry button will be displayed.
Related Issue
Resolve #10906
Screenshots: