Feat/cancel pending transaction #5520
Conversation
WalkthroughThe recent updates introduce a transaction cancellation feature for the Stacks blockchain. This includes new components, hooks, and utilities to support transaction cancellation, as well as modifications to existing components to integrate this feature. Additionally, some props and enums have been renamed for clarity and consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant TransactionItem
participant CancelTransactionButton
participant CancelStxTransactionDialog
participant StacksNetwork
User ->> TransactionItem: Clicks on Cancel Button
TransactionItem ->> CancelTransactionButton: Renders Cancel Button
CancelTransactionButton ->> App: Triggers Cancel Transaction Action
App ->> CancelStxTransactionDialog: Opens Cancel Transaction Dialog
CancelStxTransactionDialog ->> StacksNetwork: Submits Cancel Transaction Request
StacksNetwork ->> CancelStxTransactionDialog: Returns Transaction Status
CancelStxTransactionDialog ->> User: Displays Cancellation Result
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
I tested this feature and seems works ok @314159265359879 could you please test as well |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (4)
src/app/components/stacks-transaction-item/cancel-transaction-button.tsx (1)
3-6: Ensure proper documentation forCancelTransactionButtonProps.Adding comments to describe each property can improve code readability and maintainability, especially for
onCancelTransactionwhich is a crucial callback.src/app/features/dialogs/cancel-transaction-dialog/components/cancel-transaction-actions.tsx (1)
7-11: Ensure proper documentation forCancelTransactionActionsProps.Adding detailed comments for each property, especially
onCancelandisBroadcasting, can enhance understanding and maintainability.src/app/components/transaction-item/transaction-item.layout.tsx (1)
3-11: Ensure proper documentation forTransactionItemLayoutProps.Adding detailed comments for each property, particularly
actionButtonGroupElement, can enhance understanding and maintainability.src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (1)
Line range hint
40-40: Replace deprecated hookuseCurrentAccountNativeSegwitAddressIndexZerowithsigner.addressto ensure future compatibility.- const bitcoinAddress = useCurrentAccountNativeSegwitAddressIndexZero(); + const bitcoinAddress = signer.address;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- src/app/common/hooks/use-loading.ts (1 hunks)
- src/app/common/utils/get-burn-address.ts (1 hunks)
- src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (1 hunks)
- src/app/components/stacks-transaction-item/cancel-transaction-button.tsx (1 hunks)
- src/app/components/stacks-transaction-item/increase-fee-button.tsx (1 hunks)
- src/app/components/stacks-transaction-item/stacks-transaction-item.tsx (4 hunks)
- src/app/components/transaction-item/transaction-item.layout.tsx (3 hunks)
- src/app/features/dialogs/cancel-transaction-dialog/cancel-stx-transaction-dialog.tsx (1 hunks)
- src/app/features/dialogs/cancel-transaction-dialog/components/cancel-transaction-actions.tsx (1 hunks)
- src/app/features/dialogs/cancel-transaction-dialog/hooks/use-selected-tx.ts (1 hunks)
- src/app/features/dialogs/cancel-transaction-dialog/hooks/use-stx-cancel-transaction.ts (1 hunks)
- src/app/features/stacks-transaction-request/hooks/use-stacks-broadcast-transaction.tsx (4 hunks)
- src/app/routes/app-routes.tsx (2 hunks)
- src/shared/route-urls.ts (1 hunks)
Additional context used
GitHub Check: lint-eslint
src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx
[warning] 40-40:
'useCurrentAccountNativeSegwitAddressIndexZero' is deprecated. Use signer.address instead
Additional comments not posted (13)
src/app/features/dialogs/cancel-transaction-dialog/hooks/use-selected-tx.ts (1)
4-7: LGTM! Good use of fallback forrawTxIdto ensure robustness in data fetching.src/app/common/hooks/use-loading.ts (1)
5-5: LGTM! Ensure thatCANCEL_TRANSACTION_DRAWERis integrated properly across the application.Verification successful
The
CANCEL_TRANSACTION_DRAWERkey is properly integrated across the application.
- src/app/features/dialogs/cancel-transaction-dialog/cancel-stx-transaction-dialog.tsx
- src/app/features/dialogs/cancel-transaction-dialog/components/cancel-transaction-actions.tsx
- src/app/common/hooks/use-loading.ts
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `CANCEL_TRANSACTION_DRAWER` across the application. # Test: Search for the enum usage. Expect: Proper integration in relevant parts of the application. rg --type typescript $'CANCEL_TRANSACTION_DRAWER'Length of output: 83
Script:
#!/bin/bash # Description: Verify the usage of `CANCEL_TRANSACTION_DRAWER` across the application. # Test: Search for the enum usage. Expect: Proper integration in relevant parts of the application. rg --type ts 'CANCEL_TRANSACTION_DRAWER'Length of output: 482
src/app/components/stacks-transaction-item/increase-fee-button.tsx (1)
15-37: LGTM! Verify the conditional rendering logic ofIncreaseFeeButtonin different scenarios to ensure it behaves as expected.Verification successful
The conditional rendering logic of the
IncreaseFeeButtoncomponent is used in two files:stacks-transaction-item.tsxandbitcoin-transaction-item.tsx. TheisEnabledandisSelectedprops are passed to theIncreaseFeeButtoncomponent in both files, ensuring that the button is rendered based on specific conditions.
src/app/components/stacks-transaction-item/stacks-transaction-item.tsxsrc/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsxThe conditional rendering logic appears to be correctly implemented in these files.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the conditional rendering logic of `IncreaseFeeButton`. # Test: Search for the component usage and test in different scenarios. rg --type typescript $'IncreaseFeeButton'Length of output: 75
Script:
#!/bin/bash # Description: Verify the conditional rendering logic of `IncreaseFeeButton`. # Test: Search for the component usage and test in different scenarios. rg --type ts,tsx 'IncreaseFeeButton'Length of output: 67
Script:
#!/bin/bash # Description: Verify the conditional rendering logic of `IncreaseFeeButton`. # Test: Search for the component usage and test in different scenarios. rg --type ts 'IncreaseFeeButton' rg --type tsx 'IncreaseFeeButton'Length of output: 822
Script:
#!/bin/bash # Description: Extract relevant lines to verify the conditional rendering logic of `IncreaseFeeButton`. # Extract lines where `IncreaseFeeButton` is used in `stacks-transaction-item.tsx`. rg -A 10 'IncreaseFeeButton' src/app/components/stacks-transaction-item/stacks-transaction-item.tsx # Extract lines where `IncreaseFeeButton` is used in `bitcoin-transaction-item.tsx`. rg -A 10 'IncreaseFeeButton' src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsxLength of output: 2079
src/app/components/stacks-transaction-item/cancel-transaction-button.tsx (1)
9-38: Review the conditional rendering logic and event handling inCancelTransactionButton.The use of conditional rendering based on
isActiveis clear and effective. The event handling correctly stops propagation after executing the callback, which is crucial for nested interactive elements.src/app/features/dialogs/cancel-transaction-dialog/components/cancel-transaction-actions.tsx (1)
12-39: Review the integration of Formik and custom hooks inCancelTransactionActions.The integration of Formik for handling form submission and the use of custom hooks for loading and wallet type detection are well implemented. The dynamic text for the action button based on the wallet type is a nice touch for user experience.
src/app/components/transaction-item/transaction-item.layout.tsx (1)
Line range hint
22-50: Review the JSX structure and conditional rendering inTransactionItemLayout.The JSX structure is well-organized, and the use of conditional rendering for displaying elements based on the presence of
actionButtonGroupElementis effective. The use ofPressablefor the entire item enhances user interaction.src/app/features/dialogs/cancel-transaction-dialog/hooks/use-stx-cancel-transaction.ts (1)
25-88: Review the complex logic and hook integration inuseStxCancelTransaction.The hook integrates multiple custom hooks and external utilities effectively. The submission logic is robust, handling various edge cases and refreshing relevant data post-transaction. Good use of
useCallbackfor optimizing performance.src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (1)
89-89: RenamingrightElementtoactionButtonGroupElementaligns with the standardization of prop names across components.src/shared/route-urls.ts (1)
30-30: Addition ofCancelStxTransactionroute supports the new feature for canceling Stacks transactions.src/app/components/stacks-transaction-item/stacks-transaction-item.tsx (1)
24-24: Integration ofCancelTransactionButtonandonCancelTransactionfunction enhances transaction management capabilities.Also applies to: 70-84, 97-113, 120-120
src/app/features/stacks-transaction-request/hooks/use-stacks-broadcast-transaction.tsx (1)
30-30: Enhancements touseStacksBroadcastTransactionto handle cancel transactions improve the robustness of transaction management.Also applies to: 37-37, 64-104, 144-144
src/app/features/dialogs/cancel-transaction-dialog/cancel-stx-transaction-dialog.tsx (1)
54-61: Verify the logic for setting transaction IDs and managing loading states.The logic in this effect seems to be crucial for the component's functionality. Please ensure that the conditions and the sequence of state updates are thoroughly tested, especially for edge cases where
rawTxIdortxIdFromParamsmight not behave as expected.src/app/routes/app-routes.tsx (1)
104-106: Ensure the new route configuration integrates seamlessly with existing routes.The addition of the
CancelStxTransactionDialogroute is a significant change. Verify that this new route does not conflict with existing routes and that it is accessible as expected in the application flow.
| availableUnlockedBalance, | ||
| stxFees, | ||
| } = useStxCancelTransaction(); | ||
| const { isLoading, setIsIdle } = useLoading(LoadingKeys.CANCEL_TRANSACTION_DRAWER); | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
| const [searchParams] = useSearchParams(); | ||
| const txIdFromParams = searchParams.get('txId'); | ||
| const toast = useToast(); | ||
|
|
||
| const fee = Number(rawTx?.auth.spendingCondition?.fee); | ||
|
|
||
| useEffect(() => { | ||
| if (tx?.tx_status !== 'pending' && rawTx) { | ||
| setTxId(null); | ||
| toast.info('Your transaction went through! Cancellation not possible.'); | ||
| } | ||
| }, [rawTx, tx?.tx_status, setTxId, toast]); | ||
|
|
||
| useEffect(() => { | ||
| if (!rawTxId && txIdFromParams) { | ||
| setRawTxId(txIdFromParams); | ||
| } | ||
| if (isLoading && !rawTxId) { | ||
| setIsIdle(); | ||
| } | ||
| }, [isLoading, rawTxId, setIsIdle, setRawTxId, txIdFromParams]); | ||
|
|
||
| if (!tx || !fee) return <LoadingSpinner />; | ||
|
|
||
| const onClose = () => { | ||
| setRawTxId(null); | ||
| navigate(RouteUrls.Home); | ||
| }; | ||
|
|
||
| return ( | ||
| <> | ||
| <Formik | ||
| initialValues={{ fee: new BigNumber(microStxToStx(fee)).toNumber() }} | ||
| onSubmit={onSubmit} | ||
| validateOnChange={false} | ||
| validateOnBlur={false} | ||
| validateOnMount={true} | ||
| validationSchema={validationSchema} | ||
| > | ||
| {props => ( | ||
| <> | ||
| <Dialog | ||
| isShowing={location.pathname === RouteUrls.CancelStxTransaction} | ||
| onClose={onClose} | ||
| header={<DialogHeader title="Cancel transaction" />} | ||
| footer={ | ||
| <Footer flexDirection="row"> | ||
| <CancelTransactionActions | ||
| onCancel={() => { | ||
| setTxId(null); | ||
| navigate(RouteUrls.Home); | ||
| }} | ||
| isDisabled={stxToMicroStx(props.values.fee).isEqualTo(fee)} | ||
| /> | ||
| </Footer> | ||
| } | ||
| > | ||
| <Stack gap="space.05" px="space.05" pb="space.05"> | ||
| <Suspense | ||
| fallback={ | ||
| <Flex alignItems="center" justifyContent="center" p="space.06"> | ||
| <Spinner /> | ||
| </Flex> | ||
| } | ||
| > | ||
| <Caption> | ||
| Canceling a transaction isn't guaranteed to work. A higher fee can help replace | ||
| the old transaction | ||
| </Caption> | ||
| <Stack gap="space.06"> | ||
| {tx && <StacksTransactionItem transaction={tx} />} | ||
| <Stack gap="space.04"> | ||
| <FeesRow fees={stxFees} defaultFeeValue={fee + 1} isSponsored={false} /> | ||
| {availableUnlockedBalance?.amount && ( | ||
| <Caption> | ||
| Balance: | ||
| {stacksValue({ | ||
| value: availableUnlockedBalance.amount, | ||
| fixedDecimals: true, | ||
| })} | ||
| </Caption> | ||
| )} | ||
| </Stack> | ||
| </Stack> | ||
| </Suspense> | ||
| </Stack> | ||
| </Dialog> | ||
| <Outlet /> | ||
| </> | ||
| )} | ||
| </Formik> | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling and edge case management in CancelStxTransactionDialog.
The function CancelStxTransactionDialog handles various states and transitions, but there seems to be a lack of explicit error handling for network or transaction failures. Consider adding error handling mechanisms to improve the robustness of the component.
| useEffect(() => { | ||
| if (tx?.tx_status !== 'pending' && rawTx) { | ||
| setTxId(null); | ||
| toast.info('Your transaction went through! Cancellation not possible.'); | ||
| } | ||
| }, [rawTx, tx?.tx_status, setTxId, toast]); |
There was a problem hiding this comment.
Refactor to simplify the effect handling transaction status.
The effect on lines 47-52 could be simplified or broken down into smaller, more testable functions. This would improve readability and maintainability.
| } | ||
| }, [isLoading, rawTxId, setIsIdle, setRawTxId, txIdFromParams]); | ||
|
|
||
| if (!tx || !fee) return <LoadingSpinner />; |
There was a problem hiding this comment.
Handle potential null values gracefully in rendering logic.
The rendering logic on line 63 directly accesses properties which might be null or undefined. This could lead to runtime errors. Consider adding null checks or using optional chaining to safeguard against these issues.
| return ( | ||
| <> | ||
| <Formik | ||
| initialValues={{ fee: new BigNumber(microStxToStx(fee)).toNumber() }} | ||
| onSubmit={onSubmit} | ||
| validateOnChange={false} | ||
| validateOnBlur={false} | ||
| validateOnMount={true} | ||
| validationSchema={validationSchema} | ||
| > | ||
| {props => ( | ||
| <> | ||
| <Dialog | ||
| isShowing={location.pathname === RouteUrls.CancelStxTransaction} | ||
| onClose={onClose} | ||
| header={<DialogHeader title="Cancel transaction" />} | ||
| footer={ | ||
| <Footer flexDirection="row"> | ||
| <CancelTransactionActions | ||
| onCancel={() => { | ||
| setTxId(null); | ||
| navigate(RouteUrls.Home); | ||
| }} | ||
| isDisabled={stxToMicroStx(props.values.fee).isEqualTo(fee)} | ||
| /> | ||
| </Footer> | ||
| } | ||
| > | ||
| <Stack gap="space.05" px="space.05" pb="space.05"> | ||
| <Suspense | ||
| fallback={ | ||
| <Flex alignItems="center" justifyContent="center" p="space.06"> | ||
| <Spinner /> | ||
| </Flex> | ||
| } | ||
| > | ||
| <Caption> | ||
| Canceling a transaction isn't guaranteed to work. A higher fee can help replace | ||
| the old transaction | ||
| </Caption> | ||
| <Stack gap="space.06"> | ||
| {tx && <StacksTransactionItem transaction={tx} />} | ||
| <Stack gap="space.04"> | ||
| <FeesRow fees={stxFees} defaultFeeValue={fee + 1} isSponsored={false} /> | ||
| {availableUnlockedBalance?.amount && ( | ||
| <Caption> | ||
| Balance: | ||
| {stacksValue({ | ||
| value: availableUnlockedBalance.amount, | ||
| fixedDecimals: true, | ||
| })} | ||
| </Caption> | ||
| )} | ||
| </Stack> | ||
| </Stack> | ||
| </Suspense> | ||
| </Stack> | ||
| </Dialog> | ||
| <Outlet /> | ||
| </> | ||
| )} | ||
| </Formik> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
Review and optimize the Formik usage for performance.
The Formik component is used extensively in the rendering logic. Consider optimizing the re-renders and validations to enhance performance, especially since validateOnMount is set to true, which might cause unnecessary validations on each render.
| export function getBurnAddress(network: StacksNetwork): string { | ||
| switch (network.chainId) { | ||
| case ChainID.Mainnet: | ||
| return 'SP00000000000003SCNSJTCSE62ZF4MSE'; | ||
| case ChainID.Testnet: | ||
| return 'ST000000000000000000002AMW42H'; | ||
| default: | ||
| return 'ST000000000000000000002AMW42H'; | ||
| } |
There was a problem hiding this comment.
Consider handling the default case explicitly in getBurnAddress to avoid potential issues in production environments.
- return 'ST000000000000000000002AMW42H';
+ throw new Error('Unsupported network chain ID');Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getBurnAddress(network: StacksNetwork): string { | |
| switch (network.chainId) { | |
| case ChainID.Mainnet: | |
| return 'SP00000000000003SCNSJTCSE62ZF4MSE'; | |
| case ChainID.Testnet: | |
| return 'ST000000000000000000002AMW42H'; | |
| default: | |
| return 'ST000000000000000000002AMW42H'; | |
| } | |
| export function getBurnAddress(network: StacksNetwork): string { | |
| switch (network.chainId) { | |
| case ChainID.Mainnet: | |
| return 'SP00000000000003SCNSJTCSE62ZF4MSE'; | |
| case ChainID.Testnet: | |
| return 'ST000000000000000000002AMW42H'; | |
| default: | |
| throw new Error('Unsupported network chain ID'); | |
| } |
| export function useStxCancelTransaction() { | ||
| const [rawTxId, setRawTxId] = useRawTxIdState(); | ||
| const tx = useSelectedTx(); | ||
| const [, setTxId] = useRawTxIdState(); |
There was a problem hiding this comment.
I am going to block this PR from getting merged. So much of this code is just duplicated w/out evaluating if needed or even working correctly. I have had to refactor quite a bit of the increase fee dialog changes here: #5500
See here where I'm making this comment, there are two uses of useRawTxIdState() in this file to set the id. This component is a mess and caused an infinite loop and broke the wallet in the linked PR.
| return ( | ||
| isActive && ( | ||
| <styled.button | ||
| _hover={{ color: 'ink.text-subdued' }} | ||
| bg="ink.background-primary" | ||
| maxWidth="125px" | ||
| ml="auto" | ||
| onClick={e => { | ||
| onCancelTransaction(); | ||
| e.stopPropagation(); | ||
| }} | ||
| pointerEvents={!isActive ? 'none' : 'all'} | ||
| position="relative" | ||
| px="space.02" | ||
| py="space.01" | ||
| rounded="xs" | ||
| zIndex={999} | ||
| > | ||
| <HStack gap="space.01"> | ||
| <styled.span textStyle="label.03" color="yellow.action-primary-default"> | ||
| Cancel transaction | ||
| </styled.span> | ||
| </HStack> | ||
| </styled.button> | ||
| ) | ||
| ); |
There was a problem hiding this comment.
nit: I'd suggest short circuiting the component's render with an if statement
| return ( | |
| isActive && ( | |
| <styled.button | |
| _hover={{ color: 'ink.text-subdued' }} | |
| bg="ink.background-primary" | |
| maxWidth="125px" | |
| ml="auto" | |
| onClick={e => { | |
| onCancelTransaction(); | |
| e.stopPropagation(); | |
| }} | |
| pointerEvents={!isActive ? 'none' : 'all'} | |
| position="relative" | |
| px="space.02" | |
| py="space.01" | |
| rounded="xs" | |
| zIndex={999} | |
| > | |
| <HStack gap="space.01"> | |
| <styled.span textStyle="label.03" color="yellow.action-primary-default"> | |
| Cancel transaction | |
| </styled.span> | |
| </HStack> | |
| </styled.button> | |
| ) | |
| ); | |
| if(!isActive) { | |
| return null; | |
| } | |
| return ( | |
| <styled.button | |
| _hover={{ color: 'ink.text-subdued' }} | |
| bg="ink.background-primary" | |
| maxWidth="125px" | |
| ml="auto" | |
| onClick={e => { | |
| onCancelTransaction(); | |
| e.stopPropagation(); | |
| }} | |
| pointerEvents={!isActive ? 'none' : 'all'} | |
| position="relative" | |
| px="space.02" | |
| py="space.01" | |
| rounded="xs" | |
| zIndex={999} | |
| > | |
| <HStack gap="space.01"> | |
| <styled.span textStyle="label.03" color="yellow.action-primary-default"> | |
| Cancel transaction | |
| </styled.span> | |
| </HStack> | |
| </styled.button> | |
| ); |
| <styled.span textStyle="label.03">Increase fee</styled.span> | ||
| </HStack> | ||
| </styled.button> | ||
| isActive && ( |
edgarkhanzadian
dismissed
their stale review
Whoops, dismissing my review per Fara's comment
|
@alter-eggo reworked PR opened for review #5533 |
|
I'll close this as a new PR from a different branch name was opened so I set it up here |
Copy of #5501
Summary by CodeRabbit
New Features
Enhancements
Refactor
rightElementprop toactionButtonGroupElementacross various components for consistency.