Fix/5501/rework (Stacks transaction cancellations) #5536
Conversation
WalkthroughThe update introduces new functionalities primarily revolving around transaction management within a blockchain application. New components and custom hooks are added for handling transaction actions like increasing fees and canceling transactions. Existing components are refactored to utilize these new features, and logical flow adjustments are made to enhance user interaction for both Bitcoin and Stacks network transactions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Hooks
participant Network
User->>UI: Initiate transaction action (e.g., cancel or increase fee)
UI->>Hooks: Call respective custom hook (useStxCancelTransaction or useStxIncreaseFee)
Hooks->>Network: Generate and send transaction request
Network->>Hooks: Return transaction response
Hooks->>UI: Update UI with transaction status
UI->>User: Display transaction action 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (3)
src/app/features/dialogs/transaction-action-dialog/components/transaction-actions.tsx (1)
6-8: Enhance theTransactionActionscomponent's prop documentation.Consider adding comments to describe the purpose and usage of each prop, especially
isBroadcastingandisLoading, to improve code maintainability.Also applies to: 12-12
src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (2)
26-26: Consider updating import paths to use absolute paths for better maintainability and to avoid potential issues with relative paths.
Line range hint
41-41: The use ofuseCurrentAccountNativeSegwitAddressIndexZerois deprecated. Update to the recommended method to ensure future compatibility.- const bitcoinAddress = useCurrentAccountNativeSegwitAddressIndexZero(); + const bitcoinAddress = signer.address;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (16)
- src/app/common/utils/get-burn-address.ts (1 hunks)
- src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (2 hunks)
- src/app/components/stacks-transaction-item/stacks-transaction-item.tsx (5 hunks)
- src/app/components/stacks-transaction-item/transaction-action-button.tsx (1 hunks)
- src/app/components/transaction-item/transaction-item.layout.tsx (3 hunks)
- src/app/features/dialogs/transaction-action-dialog/cancel-stx-transaction-dialog.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/components/transaction-actions.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-cancel-transaction.ts (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-increase-fee.ts (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-transaction-actions.ts (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/increase-btc-fee-dialog.tsx (2 hunks)
- src/app/features/dialogs/transaction-action-dialog/increase-stx-fee-dialog.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/stx-transaction-action-dialog.tsx (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] 41-41:
'useCurrentAccountNativeSegwitAddressIndexZero' is deprecated. Use signer.address instead
Additional comments not posted (17)
src/app/features/dialogs/transaction-action-dialog/increase-stx-fee-dialog.tsx (1)
7-16: Good use of theStxTransactionActionDialogcomponent inIncreaseStxFeeDialog.The component is well-structured, and the properties like
title,description, androuteUrlare appropriately set.src/app/components/stacks-transaction-item/transaction-action-button.tsx (2)
3-10: The updated interfaceActionButtonPropsintroduces flexibility in button configuration. Ensure that the new optional properties are consistently handled across the application.
13-39: The implementation ofTransactionActionButtonis solid, with appropriate conditional rendering and event handling. Consider adding a comment explaining the use ofe.stopPropagation()for clarity.src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-increase-fee.ts (1)
9-42: TheuseStxIncreaseFeehook is well-implemented with clear logic flow and appropriate use of dependencies inuseCallback. Consider adding error handling in theonSubmitfunction to manage potential exceptions during the transaction update process.src/app/components/transaction-item/transaction-item.layout.tsx (1)
Line range hint
3-50: TheTransactionItemLayoutcomponent is well-structured with effective use of layout primitives and conditional rendering. Ensure that accessibility standards are met, particularly regarding the clickable areas and dynamic text elements.src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-transaction-actions.ts (1)
18-56: TheuseStxTransactionActionhook is effectively managing transaction actions with good integration of custom hooks and utilities. Consider adding detailed documentation on how theactionTypeparameter influences the behavior of the hook, to enhance maintainability and clarity for other developers.src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (1)
80-84: Ensure theisEnabledproperty is correctly passed toTransactionActionButtonto manage button state effectively.Verification successful
The
isEnabledproperty inbitcoin-transaction-item.tsxis correctly defined and used. It is determined based on whether the transaction is not inbound, not confirmed, and does not contain a Taproot input. This ensures that theTransactionActionButtonis enabled only when appropriate.
src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx: TheisEnabledproperty is correctly set and passed to theTransactionActionButton.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `isEnabled` property is used correctly in all instances of `TransactionActionButton`. # Test: Search for the component usage. Expect: Correct usage of the isEnabled prop. rg --type typescript $'TransactionActionButton'Length of output: 81
Script:
#!/bin/bash # Description: Verify the `isEnabled` property is used correctly in all instances of `TransactionActionButton`. # Test: Search for the component usage. Expect: Correct usage of the isEnabled prop. rg --type ts 'TransactionActionButton'Length of output: 692
Script:
#!/bin/bash # Description: Extract lines containing `TransactionActionButton` from the identified files to verify `isEnabled` property usage. # Extract lines from stacks-transaction-item.tsx rg --type ts 'TransactionActionButton' src/app/components/stacks-transaction-item/stacks-transaction-item.tsx # Extract lines from bitcoin-transaction-item.tsx rg --type ts 'TransactionActionButton' src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsxLength of output: 446
Script:
#!/bin/bash # Description: Extract 10 lines before and after the identified lines containing `TransactionActionButton` to verify `isEnabled` property usage. # Extract lines from stacks-transaction-item.tsx rg --type ts -C 10 'TransactionActionButton' src/app/components/stacks-transaction-item/stacks-transaction-item.tsx # Extract lines from bitcoin-transaction-item.tsx rg --type ts -C 10 'TransactionActionButton' src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsxLength of output: 3784
Script:
#!/bin/bash # Description: Extract the definition and usage of `isEnabled` in `bitcoin-transaction-item.tsx`. # Extract lines containing `isEnabled` from bitcoin-transaction-item.tsx rg --type ts 'isEnabled' src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsxLength of output: 220
Script:
#!/bin/bash # Description: Extract 10 lines before and after the identified lines containing `isEnabled` to verify its definition in `bitcoin-transaction-item.tsx`. # Extract lines from bitcoin-transaction-item.tsx rg --type ts -C 10 'isEnabled' src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsxLength of output: 1506
src/shared/route-urls.ts (1)
30-30: Adding a new route for canceling STX transactions is a significant update. Ensure all related components and hooks are updated to handle this new route correctly.Verification successful
The new route
CancelStxTransactionis correctly integrated into the following parts of the application:
- Defined in
src/shared/route-urls.ts.- Used in
src/app/features/dialogs/transaction-action-dialog/cancel-stx-transaction-dialog.tsx.- Integrated into the routing in
src/app/routes/app-routes.tsx.- Referenced in
src/app/components/stacks-transaction-item/stacks-transaction-item.tsx.These integrations indicate that the new route is handled appropriately in the relevant components and hooks.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new route is handled in all relevant parts of the application. # Test: Search for the route usage. Expect: Correct handling in components and hooks. rg --type typescript $'CancelStxTransaction'Length of output: 78
Script:
#!/bin/bash # Description: Verify the new route is handled in all relevant parts of the application. # Test: Search for the route usage. Expect: Correct handling in components and hooks. rg --type ts 'CancelStxTransaction'Length of output: 893
src/app/features/dialogs/transaction-action-dialog/increase-btc-fee-dialog.tsx (2)
24-24: Updated import fromIncreaseFeeActionstoTransactionActionsreflects the broader functionality now supported by this component.
73-73: The use ofTransactionActionsin the dialog footer correctly implements the updated component, enhancing its reusability.src/app/features/stacks-transaction-request/hooks/use-stacks-broadcast-transaction.tsx (2)
31-31: Addition of theisCancelTransactionparameter touseStacksBroadcastTransactionallows for specialized handling of cancellation scenarios, enhancing the hook's functionality.Also applies to: 38-38
65-65: The conditional logic usingisCancelTransactionto navigate and show success messages enhances user experience by providing specific feedback for transaction cancellations.
[APROVED]Also applies to: 103-105
src/app/components/stacks-transaction-item/stacks-transaction-item.tsx (3)
1-1: The updated imports and the new import ofTransactionActionButtonalign with the refactored functionality of handling transaction actions within the component.Also applies to: 3-3, 27-27
29-34: The introduction ofTransactionActionandActionRouteMaptypes supports dynamic transaction action handling by mapping actions to routes, enhancing maintainability and scalability.
Line range hint
77-120: The refactored component logic for handling transaction actions, including dynamic generation of action buttons and routing based on transaction status, significantly enhances user interaction and maintainability.src/app/routes/app-routes.tsx (2)
19-21: The addition of routes forCancelStxTransactionDialogand related error handling aligns with the new functionality for STX transaction cancellations, enhancing the app's navigation structure.Also applies to: 108-114
19-21: Updated imports for dialog components used in transaction actions correctly reflect the structural changes and new functionalities introduced in the app.
| export function CancelStxTransactionDialog() { | ||
| return ( | ||
| <StxTransactionActionDialog | ||
| title="Cancel transaction" | ||
| description="Canceling a transaction isn't guaranteed to work. A higher fee can help replace the old transaction" | ||
| routeUrl={RouteUrls.CancelStxTransaction} | ||
| useActionHook={useStxCancelTransaction} | ||
| FeeComponent={IncreaseFeeField} | ||
| isCancel={true} | ||
| /> |
There was a problem hiding this comment.
Clarify the transaction cancellation conditions in CancelStxTransactionDialog.
Consider updating the description to emphasize that even with a higher fee, cancellation is not guaranteed.
| export function useStxCancelTransaction(txid: string | undefined) { | ||
| const { | ||
| tx, | ||
| rawTx, | ||
| isLoadingTx, | ||
| isLoadingRawTx, | ||
| refreshAccountData, | ||
| submittedTransactionsActions, | ||
| stacksBroadcastTransaction, | ||
| availableUnlockedBalance, | ||
| fee, | ||
| validationSchema, | ||
| } = useStxTransactionAction(txid, 'cancel'); | ||
|
|
||
| const network = useCurrentStacksNetworkState(); | ||
| const account = useCurrentStacksAccount(); | ||
| const { data: stxFees } = useCalculateStacksTxFees(rawTx); | ||
|
|
||
| const onSubmit = useCallback( | ||
| async (values: any) => { | ||
| if (!tx || !rawTx || !account) return; | ||
|
|
||
| const options: GenerateUnsignedTransactionOptions = { | ||
| publicKey: account.stxPublicKey, | ||
| nonce: tx.nonce, | ||
| fee: stxToMicroStx(values?.fee || 0).toNumber(), | ||
| txData: { | ||
| txType: TransactionTypes.STXTransfer, | ||
| recipient: getBurnAddress(network), | ||
| amount: 1, | ||
| memo: '_cancel transaction', | ||
| network: network, | ||
| } as any, | ||
| }; | ||
| const newTx = await generateUnsignedTransaction(options); | ||
| const txId = tx.tx_id || safelyFormatHexTxid(rawTx.txid()); | ||
| await refreshAccountData(); | ||
| submittedTransactionsActions.transactionReplacedByFee(txId); | ||
| await stacksBroadcastTransaction(newTx); | ||
| }, | ||
| [ | ||
| tx, | ||
| rawTx, | ||
| account, | ||
| network, | ||
| refreshAccountData, | ||
| submittedTransactionsActions, | ||
| stacksBroadcastTransaction, | ||
| ] | ||
| ); | ||
|
|
||
| return { | ||
| availableUnlockedBalance, | ||
| fee, | ||
| rawTx, | ||
| tx, | ||
| stxFees, | ||
| txid, | ||
| onSubmit, | ||
| isLoadingRawTx, | ||
| isLoadingTx, | ||
| validationSchema, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The implementation of useStxCancelTransaction is well-structured and modular. However, consider adding error handling for the asynchronous operations within the onSubmit callback to enhance robustness.
- await refreshAccountData();
- submittedTransactionsActions.transactionReplacedByFee(txId);
- await stacksBroadcastTransaction(newTx);
+ try {
+ await refreshAccountData();
+ submittedTransactionsActions.transactionReplacedByFee(txId);
+ await stacksBroadcastTransaction(newTx);
+ } catch (error) {
+ console.error('Error during transaction cancellation:', error);
+ }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 useStxCancelTransaction(txid: string | undefined) { | |
| const { | |
| tx, | |
| rawTx, | |
| isLoadingTx, | |
| isLoadingRawTx, | |
| refreshAccountData, | |
| submittedTransactionsActions, | |
| stacksBroadcastTransaction, | |
| availableUnlockedBalance, | |
| fee, | |
| validationSchema, | |
| } = useStxTransactionAction(txid, 'cancel'); | |
| const network = useCurrentStacksNetworkState(); | |
| const account = useCurrentStacksAccount(); | |
| const { data: stxFees } = useCalculateStacksTxFees(rawTx); | |
| const onSubmit = useCallback( | |
| async (values: any) => { | |
| if (!tx || !rawTx || !account) return; | |
| const options: GenerateUnsignedTransactionOptions = { | |
| publicKey: account.stxPublicKey, | |
| nonce: tx.nonce, | |
| fee: stxToMicroStx(values?.fee || 0).toNumber(), | |
| txData: { | |
| txType: TransactionTypes.STXTransfer, | |
| recipient: getBurnAddress(network), | |
| amount: 1, | |
| memo: '_cancel transaction', | |
| network: network, | |
| } as any, | |
| }; | |
| const newTx = await generateUnsignedTransaction(options); | |
| const txId = tx.tx_id || safelyFormatHexTxid(rawTx.txid()); | |
| await refreshAccountData(); | |
| submittedTransactionsActions.transactionReplacedByFee(txId); | |
| await stacksBroadcastTransaction(newTx); | |
| }, | |
| [ | |
| tx, | |
| rawTx, | |
| account, | |
| network, | |
| refreshAccountData, | |
| submittedTransactionsActions, | |
| stacksBroadcastTransaction, | |
| ] | |
| ); | |
| return { | |
| availableUnlockedBalance, | |
| fee, | |
| rawTx, | |
| tx, | |
| stxFees, | |
| txid, | |
| onSubmit, | |
| isLoadingRawTx, | |
| isLoadingTx, | |
| validationSchema, | |
| }; | |
| } | |
| try { | |
| await refreshAccountData(); | |
| submittedTransactionsActions.transactionReplacedByFee(txId); | |
| await stacksBroadcastTransaction(newTx); | |
| } catch (error) { | |
| console.error('Error during transaction cancellation:', error); | |
| } |
| export function StxTransactionActionDialog({ | ||
| title, | ||
| description, | ||
| routeUrl, | ||
| useActionHook, | ||
| FeeComponent, | ||
| isCancel = false, | ||
| }: StxTransactionActionDialogProps) { | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
| const { txid } = useParams(); | ||
|
|
||
| const { | ||
| rawTx, | ||
| tx, | ||
| onSubmit, | ||
| validationSchema, | ||
| availableUnlockedBalance, | ||
| isLoadingRawTx, | ||
| isLoadingTx, | ||
| } = useActionHook(txid); | ||
|
|
||
| if (isLoadingRawTx || isLoadingTx) return <LoadingSpinner />; | ||
| if (!txid) return null; | ||
|
|
||
| const fee = Number(rawTx?.auth.spendingCondition?.fee); | ||
|
|
||
| const initialFee = isCancel ? fee + 1 : fee; | ||
|
|
||
| return ( | ||
| <> | ||
| <Formik | ||
| initialValues={{ fee: new BigNumber(microStxToStx(initialFee)).toNumber() }} | ||
| onSubmit={values => onSubmit(values, rawTx)} | ||
| validateOnChange={false} | ||
| validateOnBlur={false} | ||
| validateOnMount={false} | ||
| validationSchema={validationSchema} | ||
| > | ||
| {props => ( | ||
| <> | ||
| <Dialog | ||
| isShowing={location.pathname === routeUrl.replace(':txid', txid)} | ||
| onClose={() => navigate(RouteUrls.Home)} | ||
| header={<DialogHeader title={title} />} | ||
| footer={ | ||
| <Footer flexDirection="row"> | ||
| <TransactionActions | ||
| isDisabled={stxToMicroStx(props.values.fee).isEqualTo(fee)} | ||
| isLoading={isLoadingRawTx || isLoadingTx} | ||
| onCancel={() => navigate(RouteUrls.Home)} | ||
| /> | ||
| </Footer> | ||
| } | ||
| > | ||
| <Stack gap="space.05" px="space.05" pb="space.05"> | ||
| <Suspense | ||
| fallback={ | ||
| <Flex alignItems="center" justifyContent="center" p="space.06"> | ||
| <Spinner /> | ||
| </Flex> | ||
| } | ||
| > | ||
| <Caption>{description}</Caption> | ||
| <Stack gap="space.06"> | ||
| {tx && <StacksTransactionItem transaction={tx} />} | ||
| <Stack gap="space.04"> | ||
| <FeeComponent currentFee={initialFee} /> | ||
| {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.
The StxTransactionActionDialog component is well-structured and makes good use of hooks and other components. However, consider adding error handling for the onSubmit function to manage exceptions more gracefully.
- onSubmit={values => onSubmit(values, rawTx)}
+ onSubmit={values => {
+ try {
+ onSubmit(values, rawTx);
+ } catch (error) {
+ console.error('Error submitting STX transaction:', error);
+ }
+ }}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 StxTransactionActionDialog({ | |
| title, | |
| description, | |
| routeUrl, | |
| useActionHook, | |
| FeeComponent, | |
| isCancel = false, | |
| }: StxTransactionActionDialogProps) { | |
| const navigate = useNavigate(); | |
| const location = useLocation(); | |
| const { txid } = useParams(); | |
| const { | |
| rawTx, | |
| tx, | |
| onSubmit, | |
| validationSchema, | |
| availableUnlockedBalance, | |
| isLoadingRawTx, | |
| isLoadingTx, | |
| } = useActionHook(txid); | |
| if (isLoadingRawTx || isLoadingTx) return <LoadingSpinner />; | |
| if (!txid) return null; | |
| const fee = Number(rawTx?.auth.spendingCondition?.fee); | |
| const initialFee = isCancel ? fee + 1 : fee; | |
| return ( | |
| <> | |
| <Formik | |
| initialValues={{ fee: new BigNumber(microStxToStx(initialFee)).toNumber() }} | |
| onSubmit={values => onSubmit(values, rawTx)} | |
| validateOnChange={false} | |
| validateOnBlur={false} | |
| validateOnMount={false} | |
| validationSchema={validationSchema} | |
| > | |
| {props => ( | |
| <> | |
| <Dialog | |
| isShowing={location.pathname === routeUrl.replace(':txid', txid)} | |
| onClose={() => navigate(RouteUrls.Home)} | |
| header={<DialogHeader title={title} />} | |
| footer={ | |
| <Footer flexDirection="row"> | |
| <TransactionActions | |
| isDisabled={stxToMicroStx(props.values.fee).isEqualTo(fee)} | |
| isLoading={isLoadingRawTx || isLoadingTx} | |
| onCancel={() => navigate(RouteUrls.Home)} | |
| /> | |
| </Footer> | |
| } | |
| > | |
| <Stack gap="space.05" px="space.05" pb="space.05"> | |
| <Suspense | |
| fallback={ | |
| <Flex alignItems="center" justifyContent="center" p="space.06"> | |
| <Spinner /> | |
| </Flex> | |
| } | |
| > | |
| <Caption>{description}</Caption> | |
| <Stack gap="space.06"> | |
| {tx && <StacksTransactionItem transaction={tx} />} | |
| <Stack gap="space.04"> | |
| <FeeComponent currentFee={initialFee} /> | |
| {availableUnlockedBalance?.amount && ( | |
| <Caption> | |
| Balance:{' '} | |
| {stacksValue({ | |
| value: availableUnlockedBalance.amount, | |
| fixedDecimals: true, | |
| })} | |
| </Caption> | |
| )} | |
| </Stack> | |
| </Stack> | |
| </Suspense> | |
| </Stack> | |
| </Dialog> | |
| <Outlet /> | |
| </> | |
| )} | |
| </Formik> | |
| </> | |
| ); | |
| } | |
| <Formik | |
| initialValues={{ fee: new BigNumber(microStxToStx(initialFee)).toNumber() }} | |
| onSubmit={values => { | |
| try { | |
| onSubmit(values, rawTx); | |
| } catch (error) { | |
| console.error('Error submitting STX transaction:', error); | |
| } | |
| }} | |
| validateOnChange={false} | |
| validateOnBlur={false} | |
| validateOnMount={false} | |
| validationSchema={validationSchema} | |
| > |
pete-watters
force-pushed
the
fix/5501/rework
branch
from
f43e50a to
bc1f6e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (1)
Line range hint
41-41: The hookuseCurrentAccountNativeSegwitAddressIndexZerois marked as deprecated by the static analysis tool. It is recommended to replace this with the suggestedsigner.addressto ensure future compatibility and to remove deprecated code.- const bitcoinAddress = useCurrentAccountNativeSegwitAddressIndexZero(); + const bitcoinAddress = signer.address; // Assuming `signer` is already in the scope or appropriately imported
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (16)
- src/app/common/utils/get-burn-address.ts (1 hunks)
- src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (2 hunks)
- src/app/components/stacks-transaction-item/stacks-transaction-item.tsx (5 hunks)
- src/app/components/stacks-transaction-item/transaction-action-button.tsx (1 hunks)
- src/app/components/transaction-item/transaction-item.layout.tsx (3 hunks)
- src/app/features/dialogs/transaction-action-dialog/cancel-stx-transaction-dialog.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/components/transaction-actions.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-cancel-transaction.ts (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-increase-fee.ts (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-transaction-actions.ts (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/increase-btc-fee-dialog.tsx (2 hunks)
- src/app/features/dialogs/transaction-action-dialog/increase-stx-fee-dialog.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/stx-transaction-action-dialog.tsx (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)
Files skipped from review as they are similar to previous changes (15)
- src/app/common/utils/get-burn-address.ts
- src/app/components/stacks-transaction-item/stacks-transaction-item.tsx
- src/app/components/stacks-transaction-item/transaction-action-button.tsx
- src/app/components/transaction-item/transaction-item.layout.tsx
- src/app/features/dialogs/transaction-action-dialog/cancel-stx-transaction-dialog.tsx
- src/app/features/dialogs/transaction-action-dialog/components/transaction-actions.tsx
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-cancel-transaction.ts
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-increase-fee.ts
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-transaction-actions.ts
- src/app/features/dialogs/transaction-action-dialog/increase-btc-fee-dialog.tsx
- src/app/features/dialogs/transaction-action-dialog/increase-stx-fee-dialog.tsx
- src/app/features/dialogs/transaction-action-dialog/stx-transaction-action-dialog.tsx
- src/app/features/stacks-transaction-request/hooks/use-stacks-broadcast-transaction.tsx
- src/app/routes/app-routes.tsx
- src/shared/route-urls.ts
Additional context used
GitHub Check: lint-eslint
src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx
[warning] 41-41:
'useCurrentAccountNativeSegwitAddressIndexZero' is deprecated. Use signer.address instead
Additional comments not posted (3)
src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (3)
80-84: TheTransactionActionButtonis used here with a specific label and onClick handler for increasing the transaction fee. Ensure that theisEnabledandisSelectedprops are properly computed and passed to maintain the UI's reactive nature.
[APROVED]
91-91: TheactionButtonGroupElementprop correctly uses the conditional rendering of theincreaseFeeButtonbased on theisEnabledstate. This is a good use of conditional rendering to manage component visibility based on state.
26-26: The import forTransactionActionButtonreplaces the previousIncreaseFeeButton, aligning with the PR's objective to refactor transaction-related buttons to a more generic component. This change should be cross-verified to ensure that all instances whereIncreaseFeeButtonwas used are now correctly usingTransactionActionButton.Verification successful
The search for
IncreaseFeeButtondid not produce any results, indicating that it has likely been replaced throughout the codebase as intended.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `IncreaseFeeButton` is replaced everywhere by `TransactionActionButton`. # Test: Search for remaining instances of `IncreaseFeeButton`. Expect: No occurrences. rg --type tsx 'IncreaseFeeButton'Length of output: 61
Script:
#!/bin/bash # Description: Verify that `IncreaseFeeButton` is replaced everywhere by `TransactionActionButton`. # Test: Search for remaining instances of `IncreaseFeeButton`. Expect: No occurrences. rg 'IncreaseFeeButton'Length of output: 22
| isEnabled={isEnabled} | ||
| isSelected={pathname === RouteUrls.IncreaseBtcFee} | ||
| onIncreaseFee={onIncreaseFee} | ||
| onClick={onIncreaseFee} |
There was a problem hiding this comment.
Typ pref is to have the onClick be a more descriptive name if possible.
| /> | ||
| ); | ||
|
|
||
| return ( | ||
| <TransactionItemLayout | ||
| openTxLink={openTxLink} | ||
| rightElement={isEnabled ? increaseFeeButton : undefined} | ||
| actionButtonGroupElement={isEnabled ? increaseFeeButton : undefined} |
There was a problem hiding this comment.
Not sure why rightElement needs to be renamed bc here a group isn't being passed? 🤔
There was a problem hiding this comment.
Not sure why
rightElementneeds to be renamed bc here agroupisn't being passed? 🤔
It is mainly because even though this PR handles only STX cancelling the original issue addresses both STX and BTC tx cancelling issues. So once a solution is up for BTC Tx cancelling there would be a group and the TransactionItemLayout is used both in STX and BTC components
There was a problem hiding this comment.
Thanks Aman. I think it's better to do only what you need to do now, and then you could refactor this if / when adding the next part of cancelling the BTC transaction.
Doing it now just adds more code to this work that is un-related to the actual task
There was a problem hiding this comment.
Thanks Aman. I think it's better to do only what you need to do now, and then you could refactor this if / when adding the next part of cancelling the BTC transaction.
Doing it now just adds more code to this work that is un-related to the actual task
Even then the buttons are not placed in right side earlier there were only increase fee hence it makes sense for naming it rightElement. Now they are moved to a different layout; both of them on bottom. Should i name it bottomElement ? 🤔
There was a problem hiding this comment.
Can you link the designs you are following here for the changes? Has a designer approved them moving? They look a bit odd imo. cc @mica000
There was a problem hiding this comment.
I don't have it on my radar right now. We can open a task for it so you could tackle that after solving the integration tests? Then it would be a case of just swapping out the component when ready.
There was a problem hiding this comment.
I don't have it on my radar right now. We can open a task for it so you could tackle that after solving the integration tests? Then it would be a case of just swapping out the component when ready.
Alright! that works
There was a problem hiding this comment.
I double checked and we already have these icons. Did you try passing variant="small" to reduce the size?
There was a problem hiding this comment.
I double checked and we already have these icons. Did you try passing
variant="small"to reduce the size?
for the close icon? yes i did it still was different ; do you mean that the new icons in the above designs are already there?
There was a problem hiding this comment.
@pete-watters Could you let me know what is the icon name exported from the ui library for the icons used by @mica000, i couldn't find the icon used in the above mockup and the close icon currently used is still bigger even with variant set to small.
| cancel: keyof typeof RouteUrls; | ||
| } | ||
|
|
||
| const ACTION_ROUTE_MAP: ActionRouteMap = { |
There was a problem hiding this comment.
Typ we camelCase these unless they are a shared constant throughout the wallet.
| import { useCurrentStacksAccountAddress } from '@app/store/accounts/blockchain/stacks/stacks-account.hooks'; | ||
| import { useSubmittedTransactionsActions } from '@app/store/submitted-transactions/submitted-transactions.hooks'; | ||
|
|
||
| export function useStxTransactionAction( |
There was a problem hiding this comment.
Naming here doesn't match file name. useStxTransactionActions?
|
@pete-watters I started reviewing this here, but maybe I should do on the original PR, or are you taking this on to do further refactoring? I'm not understanding some of the changes here, so not sure if should continue review here? |
| title: string; | ||
| description: string; | ||
| routeUrl: string; | ||
| useActionHook(txid?: string): any; |
There was a problem hiding this comment.
Not sure about passing a hook with an any type here?
Thanks for checking here. I think what ever is easiest for you is fine. We will have to merge this one rather than the other so I think it's OK to mention here |
| const handleTransactionAction = (action: TransactionAction): void => { | ||
| if (!transaction) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
You don't need the brackets here:
if (!transaction) return;
| const increaseFeeButton = ( | ||
| <IncreaseFeeButton | ||
|
|
||
| const actionButtonGroup = [ |
There was a problem hiding this comment.
I think it's actually cleaner to just have 2 buttons, one for each action rather than mapping.
Mapping is good to keep the code DRY but with just two options I'm not sure if it's worth it
| key={label} | ||
| icon={ | ||
| action === 'cancel' ? ( | ||
| <CloseIcon color="stacks" variant="small" height={12} /> |
There was a problem hiding this comment.
Do you need height on this icon?
There was a problem hiding this comment.
Do you need
heighton this icon?
So if you check out the initial comments in the previous PR @314159265359879 mentioned about the cancel icon size being bigger than the increase fee icon. Even though i set both to small variant the cancel one was being bigger . I think the svg is bigger for close icon. Hence i had to override it. Editing the SVG would cause changes in other places where it's already used. So i thought i will override for this case
| rightElement={isOriginator && isPending ? increaseFeeButton : undefined} | ||
| actionButtonGroupElement={ | ||
| isOriginator && isPending ? ( | ||
| <HStack alignItems="start" gap="space.01"> |
There was a problem hiding this comment.
Maybe HStack can come as part of actionButtonGroup ?
If you don't map and just have:
function ActionButtons(){
return(
<HStack>
<IncreaseFee />
<CancelAction />
</HStack>
| } | ||
| titleRight={ | ||
| rightElement ? rightElement : <styled.span textStyle="label.02">{txValue}</styled.span> | ||
| !actionButtonGroupElement && <styled.span textStyle="label.02">{txValue}</styled.span> |
There was a problem hiding this comment.
Are you sure this will work?
Before we set it to rightElement if defined but here it seems to just use a fallback if !actionButtonGroupElement
There was a problem hiding this comment.
Are you sure this will work?
Before we set it to
rightElementif defined but here it seems to just use a fallback if!actionButtonGroupElement
The position of the element was moved from being on right side to the bottom, so in this logic im checking if there is no actionButtomGroupElement then i should just show the txValue. When tested there were no issues. I am open to any suggestions for the layout or conditional rendering
| isDisabled?: boolean; | ||
| isBroadcasting?: boolean; |
There was a problem hiding this comment.
We try and keep types in alphabetical order
|
Thanks for your work here @Aman-zishan . It's mostly looking good but if you can check some of the feedback that would be great. If you want to make changes you can do them on your PR here. If you could please rebase it with |
| return ( | ||
| <StxTransactionActionDialog | ||
| title="Cancel transaction" | ||
| description="Canceling a transaction isn't guaranteed to work. A higher fee can help replace the old transaction" |
There was a problem hiding this comment.
Canceling is not spelt correctly here (in standard British English)
Did you get the copy text from #3406?
There was a problem hiding this comment.
Cancelingis not spelt correctly here (in standard British English)Did you get the copy text from #3406?
Yes i did , i copied the text from one of the comments suggesting the workflow. If there is a new description which is better i can replace it along with the above suggested changes
Sure! will do so |
|
@pete-watters i have reopened the PR for review with changes requested, reverted the changes made to |
markmhendrickson
changed the title
fbwoolf
force-pushed
the
fix/5501/rework
branch
2 times, most recently
from
4013d2d to
b4e5b97
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
Outside diff range and nitpick comments (2)
src/app/features/dialogs/transaction-action-dialog/cancel-stx-transaction-dialog.tsx (1)
7-16: Well-implementedCancelStxTransactionDialogcomponent.The component is correctly set up to use the
StxTransactionActionDialogfor handling transaction cancellations. The props includingisCancelflag are appropriately used.Note: Ensure that the description about transaction cancellation not being guaranteed is clear and precise to manage user expectations effectively.
src/app/features/dialogs/transaction-action-dialog/components/transaction-actions.tsx (1)
7-7: Well-implementedTransactionActionscomponent.The component is structured effectively, using hooks and conditional rendering based on the props. The handling of form submission and button states is robust.
Note: Ensure that the props are maintained in alphabetical order as per the team's coding standards.
Also applies to: 13-13
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- src/app/common/utils/get-burn-address.ts (1 hunks)
- src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx (2 hunks)
- src/app/components/stacks-transaction-item/stacks-transaction-item.tsx (5 hunks)
- src/app/components/stacks-transaction-item/transaction-action-button.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/cancel-stx-transaction-dialog.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/components/transaction-actions.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-cancel-transaction.ts (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-increase-fee.ts (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-transaction-actions.ts (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/increase-btc-fee-dialog.tsx (2 hunks)
- src/app/features/dialogs/transaction-action-dialog/increase-stx-fee-dialog.tsx (1 hunks)
- src/app/features/dialogs/transaction-action-dialog/stx-transaction-action-dialog.tsx (1 hunks)
- src/app/features/stacks-transaction-request/hooks/use-stacks-broadcast-transaction.tsx (4 hunks)
Additional context used
GitHub Check: lint-eslint
src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsx
[warning] 39-39:
'useCurrentAccountNativeSegwitAddressIndexZero' is deprecated. Use signer.address instead
Additional comments not posted (3)
src/app/features/dialogs/transaction-action-dialog/increase-stx-fee-dialog.tsx (1)
7-16: Well-structuredIncreaseStxFeeDialogcomponent.The component is implemented correctly, utilizing the
StxTransactionActionDialogwith appropriate props for increasing the transaction fee. Good use of external hooks and components to keep the component concise.src/app/components/stacks-transaction-item/transaction-action-button.tsx (1)
13-39: Verify: Ensure correct handling ofisActivestate.The
isActivelogic determines the visibility of the button, which is crucial for user interaction. Verify that this behavior aligns with the intended user experience, especially in scenarios where the button should remain visible but disabled.Verification successful
Verify: Ensure correct handling of
isActivestate.The
isActivestate in theTransactionActionButtoncomponent is derived from theisEnabledandisSelectedprops. The usage and conditions for these props have been verified in the parent components:
In
stacks-transaction-item.tsx:
isEnabledis set toisOriginator && isPending.isSelectedis set toisTransactionActionRoute.In
bitcoin-transaction-item.tsx:
isEnabledis set toisOriginator && !transaction.status.confirmed && !containsTaprootInput(transaction).isSelectedis set topathname === RouteUrls.IncreaseBtcFee.These conditions appear to align with the intended user experience, ensuring the button visibility and state are managed correctly based on the transaction status and user actions.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for `TransactionActionButton` usage to ensure `isActive` is handled correctly. # Test: Look for the component's usage in scenarios where it should be disabled but visible. ast-grep --lang javascript --pattern $'TransactionActionButton($$$)'Length of output: 69
Script:
#!/bin/bash # Description: Search for `TransactionActionButton` usage across the codebase to ensure `isActive` is handled correctly. # Test: Look for the component's usage in various scenarios. rg 'TransactionActionButton' --type js --type ts --type tsxLength of output: 87
Script:
#!/bin/bash # Description: Search for `TransactionActionButton` usage across the codebase to ensure `isActive` is handled correctly. # Test: Look for the component's usage in various scenarios. rg 'TransactionActionButton' --glob '*.js' --glob '*.ts' --glob '*.tsx'Length of output: 829
Script:
#!/bin/bash # Description: Extract relevant lines from `stacks-transaction-item.tsx` and `bitcoin-transaction-item.tsx` to inspect the handling of `isActive`. # Test: Look for the component's usage and the values passed to `isEnabled` and `isSelected`. rg -A 10 'TransactionActionButton' src/app/components/stacks-transaction-item/stacks-transaction-item.tsx rg -A 10 'TransactionActionButton' src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsxLength of output: 2071
Script:
#!/bin/bash # Description: Extract relevant lines defining `isOriginator`, `isPending`, `isTransactionActionRoute`, and `isEnabled` in the parent components. # Test: Ensure that these variables are correctly set. rg 'const isOriginator' src/app/components/stacks-transaction-item/stacks-transaction-item.tsx rg 'const isPending' src/app/components/stacks-transaction-item/stacks-transaction-item.tsx rg 'const isTransactionActionRoute' src/app/components/stacks-transaction-item/stacks-transaction-item.tsx rg 'const isEnabled' src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsxLength of output: 639
Script:
#!/bin/bash # Description: Extract additional lines around the definition of `isEnabled` in `bitcoin-transaction-item.tsx` to understand its full context. # Test: Ensure that `isEnabled` is correctly set. rg -A 5 'const isEnabled' src/app/components/bitcoin-transaction-item/bitcoin-transaction-item.tsxLength of output: 284
src/app/features/dialogs/transaction-action-dialog/stx-transaction-action-dialog.tsx (1)
47-135: Enhance error handling in theonSubmitfunction.The current implementation of the
onSubmitfunction lacks error handling, which could lead to uncaught exceptions if an error occurs during the submission process.- onSubmit={values => onSubmit(values, rawTx)} + onSubmit={values => { + try { + onSubmit(values, rawTx); + } catch (error) { + console.error('Error submitting STX transaction:', error); + } + }}
| import { RouteUrls } from '@shared/route-urls'; | ||
|
|
||
| import { stacksValue } from '@app/common/stacks-utils'; | ||
| import { LoadingSpinner } from '@app/components/loading-spinner'; | ||
| import { StacksTransactionItem } from '@app/components/stacks-transaction-item/stacks-transaction-item'; | ||
| import { Dialog } from '@app/ui/components/containers/dialog/dialog'; | ||
| import { Footer } from '@app/ui/components/containers/footers/footer'; |
There was a problem hiding this comment.
Consider using absolute imports for clarity and consistency.
The file mixes relative and absolute imports. It's generally a good practice to use absolute imports for internal libraries and utilities to improve readability and maintainability.
|
|
||
| interface StxTransactionActionDialogProps { | ||
| description: string; | ||
| FeeComponent: React.ComponentType<any>; |
There was a problem hiding this comment.
Type safety issue: Use a more specific type instead of any.
The FeeComponent prop is currently typed as React.ComponentType<any>, which does not enforce any contract for the props that FeeComponent might expect.
- FeeComponent: React.ComponentType<any>;
+ FeeComponent: React.ComponentType<{ currentFee: number }>;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.
| FeeComponent: React.ComponentType<any>; | |
| FeeComponent: React.ComponentType<{ currentFee: number }>; |
There was a problem hiding this comment.
Can you please avoid any as suggested above?
There was a problem hiding this comment.
@pete-watters, I've confirmed that the FeeComponent prop can be safely updated to React.ComponentType<{ currentFee: number }> since the IncreaseFeeField component only expects the currentFee prop. Here is the updated type:
FeeComponent: React.ComponentType<{ currentFee: number }>;This change will enforce type safety by ensuring the correct props are passed to FeeComponent.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for the usage of `FeeComponent` within the codebase to determine its expected props.
rg 'FeeComponent' --glob '!node_modules' --glob '!dist'
Length of output: 671
Script:
#!/bin/bash
# Searching for the definition of `IncreaseFeeField` to determine its prop types.
rg 'function IncreaseFeeField' --glob '!node_modules' --glob '!dist'
Length of output: 239
Script:
#!/bin/bash
# Searching for the definition of `IncreaseFeeFieldProps` in the `increase-fee-field.tsx` file to determine its properties.
rg 'IncreaseFeeFieldProps' src/app/features/dialogs/transaction-action-dialog/components/increase-fee-field.tsx
Length of output: 229
Script:
#!/bin/bash
# Fetching the complete definition of `IncreaseFeeFieldProps` from `increase-fee-field.tsx`.
rg -A 10 'interface IncreaseFeeFieldProps' src/app/features/dialogs/transaction-action-dialog/components/increase-fee-field.tsx
Length of output: 517
Learnings added
Learnt from: pete-watters
PR: leather-io/extension#5536
File: src/app/features/dialogs/transaction-action-dialog/stx-transaction-action-dialog.tsx:40-40
Timestamp: 2024-07-02T12:35:48.358Z
Learning: The `FeeComponent` prop in `stx-transaction-action-dialog.tsx` should be typed as `React.ComponentType<{ currentFee: number }>` to ensure type safety, based on the `IncreaseFeeField` component's expected props.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Can you please avoid
anyas suggested above?
yeah i have updated this code with types
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| availableUnlockedBalance: Money; | ||
| isLoadingRawTx: boolean; | ||
| isLoadingTx: boolean; | ||
| onSubmit(values: any, rawTx?: StacksTransaction): Promise<void>; |
There was a problem hiding this comment.
Type safety issue: Consider specifying a more precise type than any.
The onSubmit function in the IStxTransactionActionHookReturn interface uses any for its values parameter. Using any can lead to potential runtime errors and makes the code less maintainable.
- onSubmit(values: any, rawTx?: StacksTransaction): Promise<void>;
+ onSubmit(values: { fee: number }, rawTx?: StacksTransaction): Promise<void>;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.
| onSubmit(values: any, rawTx?: StacksTransaction): Promise<void>; | |
| onSubmit(values: { fee: number }, rawTx?: StacksTransaction): Promise<void>; |
| if (isLoadingRawTx || isLoadingTx) return <LoadingSpinner />; | ||
| if (!txid) return null; |
There was a problem hiding this comment.
Improve UX for loading states.
Returning a spinner immediately might make the UI feel less responsive. Consider showing some UI elements or placeholders instead of blocking the entire UI.
- if (isLoadingRawTx || isLoadingTx) return <LoadingSpinner />;
+ if (isLoadingRawTx || isLoadingTx) {
+ return (
+ <>
+ <DialogHeader title={title} />
+ <LoadingSpinner />
+ </>
+ );
+ }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.
| if (isLoadingRawTx || isLoadingTx) return <LoadingSpinner />; | |
| if (!txid) return null; | |
| if (isLoadingRawTx || isLoadingTx) { | |
| return ( | |
| <> | |
| <DialogHeader title={title} /> | |
| <LoadingSpinner /> | |
| </> | |
| ); | |
| } | |
| if (!txid) return null; |
| 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.
Refine default case handling in getBurnAddress.
Currently, the default case returns the Testnet burn address. This might not be appropriate for all possible chainId values. Consider throwing an error or adding a logging statement to handle unexpected chainId values explicitly.
- return 'ST000000000000000000002AMW42H';
+ throw new Error(`Unsupported chainId: ${network.chainId}`);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 chainId: ${network.chainId}`); | |
| } |
| @@ -1,6 +1,9 @@ | |||
| import { useLocation, useNavigate } from 'react-router-dom'; | |||
| import { useMatch, useNavigate } from 'react-router-dom'; | |||
There was a problem hiding this comment.
Refactor suggestion: Use more descriptive names for hooks.
Consider renaming useMatch to something more descriptive, like useRouteMatch, to clarify its purpose.
- import { useMatch, useNavigate } from 'react-router-dom';
+ import { useRouteMatch as useMatch, useNavigate } from 'react-router-dom';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.
| import { useMatch, useNavigate } from 'react-router-dom'; | |
| import { useRouteMatch as useMatch, useNavigate } from 'react-router-dom'; |
| export function useStxIncreaseFee(txid: string | undefined) { | ||
| const { | ||
| tx, | ||
| rawTx, | ||
| isLoadingTx, | ||
| isLoadingRawTx, | ||
| refreshAccountData, | ||
| submittedTransactionsActions, | ||
| stacksBroadcastTransaction, | ||
| availableUnlockedBalance, | ||
| validationSchema, | ||
| } = useStxTransactionActions(txid, 'increaseFee'); | ||
|
|
||
| const onSubmit = useCallback( | ||
| async (values: { fee: number }, rawTx?: StacksTransaction) => { | ||
| if (!tx || !rawTx) return; | ||
| rawTx.setFee(stxToMicroStx(values.fee).toString()); | ||
| const txid = tx.tx_id || safelyFormatHexTxid(rawTx.txid()); | ||
| await refreshAccountData(); | ||
| submittedTransactionsActions.transactionReplacedByFee(txid); | ||
| await stacksBroadcastTransaction(rawTx); | ||
| }, | ||
| [tx, refreshAccountData, submittedTransactionsActions, stacksBroadcastTransaction] | ||
| ); | ||
|
|
||
| return { | ||
| availableUnlockedBalance, | ||
| isLoadingRawTx, | ||
| isLoadingTx, | ||
| onSubmit, | ||
| rawTx, | ||
| tx, | ||
| validationSchema, | ||
| }; |
There was a problem hiding this comment.
Refactor: Simplify the onSubmit callback logic.
The onSubmit function is crucial for the fee update process but appears complex due to multiple operations within a single callback. Consider breaking it down into smaller, more manageable functions. This could enhance readability and maintainability.
- const onSubmit = useCallback(
- async (values: { fee: number }, rawTx?: StacksTransaction) => {
- if (!tx || !rawTx) return;
- rawTx.setFee(stxToMicroStx(values.fee).toString());
- const txid = tx.tx_id || safelyFormatHexTxid(rawTx.txid());
- await refreshAccountData();
- submittedTransactionsActions.transactionReplacedByFee(txid);
- await stacksBroadcastTransaction(rawTx);
- },
- [tx, refreshAccountData, submittedTransactionsActions, stacksBroadcastTransaction]
- );
+ const handleFeeUpdate = useCallback((fee: number, transaction: StacksTransaction) => {
+ transaction.setFee(stxToMicroStx(fee).toString());
+ }, []);
+
+ const handleTransactionSubmission = useCallback(async (transaction: StacksTransaction) => {
+ const txid = tx.tx_id || safelyFormatHexTxid(transaction.txid());
+ await refreshAccountData();
+ submittedTransactionsActions.transactionReplacedByFee(txid);
+ await stacksBroadcastTransaction(transaction);
+ }, [refreshAccountData, submittedTransactionsActions, stacksBroadcastTransaction]);
+
+ const onSubmit = useCallback(
+ async (values: { fee: number }, rawTx?: StacksTransaction) => {
+ if (!tx || !rawTx) return;
+ handleFeeUpdate(values.fee, rawTx);
+ await handleTransactionSubmission(rawTx);
+ },
+ [tx, handleFeeUpdate, handleTransactionSubmission]
+ );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 useStxIncreaseFee(txid: string | undefined) { | |
| const { | |
| tx, | |
| rawTx, | |
| isLoadingTx, | |
| isLoadingRawTx, | |
| refreshAccountData, | |
| submittedTransactionsActions, | |
| stacksBroadcastTransaction, | |
| availableUnlockedBalance, | |
| validationSchema, | |
| } = useStxTransactionActions(txid, 'increaseFee'); | |
| const onSubmit = useCallback( | |
| async (values: { fee: number }, rawTx?: StacksTransaction) => { | |
| if (!tx || !rawTx) return; | |
| rawTx.setFee(stxToMicroStx(values.fee).toString()); | |
| const txid = tx.tx_id || safelyFormatHexTxid(rawTx.txid()); | |
| await refreshAccountData(); | |
| submittedTransactionsActions.transactionReplacedByFee(txid); | |
| await stacksBroadcastTransaction(rawTx); | |
| }, | |
| [tx, refreshAccountData, submittedTransactionsActions, stacksBroadcastTransaction] | |
| ); | |
| return { | |
| availableUnlockedBalance, | |
| isLoadingRawTx, | |
| isLoadingTx, | |
| onSubmit, | |
| rawTx, | |
| tx, | |
| validationSchema, | |
| }; | |
| export function useStxIncreaseFee(txid: string | undefined) { | |
| const { | |
| tx, | |
| rawTx, | |
| isLoadingTx, | |
| isLoadingRawTx, | |
| refreshAccountData, | |
| submittedTransactionsActions, | |
| stacksBroadcastTransaction, | |
| availableUnlockedBalance, | |
| validationSchema, | |
| } = useStxTransactionActions(txid, 'increaseFee'); | |
| const handleFeeUpdate = useCallback((fee: number, transaction: StacksTransaction) => { | |
| transaction.setFee(stxToMicroStx(fee).toString()); | |
| }, []); | |
| const handleTransactionSubmission = useCallback(async (transaction: StacksTransaction) => { | |
| const txid = tx.tx_id || safelyFormatHexTxid(transaction.txid()); | |
| await refreshAccountData(); | |
| submittedTransactionsActions.transactionReplacedByFee(txid); | |
| await stacksBroadcastTransaction(transaction); | |
| }, [refreshAccountData, submittedTransactionsActions, stacksBroadcastTransaction]); | |
| const onSubmit = useCallback( | |
| async (values: { fee: number }, rawTx?: StacksTransaction) => { | |
| if (!tx || !rawTx) return; | |
| handleFeeUpdate(values.fee, rawTx); | |
| await handleTransactionSubmission(rawTx); | |
| }, | |
| [tx, handleFeeUpdate, handleTransactionSubmission] | |
| ); | |
| return { | |
| availableUnlockedBalance, | |
| isLoadingRawTx, | |
| isLoadingTx, | |
| onSubmit, | |
| rawTx, | |
| tx, | |
| validationSchema, | |
| }; |
| export function useStxTransactionActions( | ||
| txid: string | undefined, | ||
| actionType: 'cancel' | 'increaseFee' | ||
| ) { | ||
| const { data: tx, isLoading: isLoadingTx } = useTransactionById(txid || ''); | ||
| const toast = useToast(); | ||
| const refreshAccountData = useRefreshAllAccountData(); | ||
| const stxAddress = useCurrentStacksAccountAddress(); | ||
| const availableUnlockedBalance = useStxAvailableUnlockedBalance(stxAddress); | ||
| const submittedTransactionsActions = useSubmittedTransactionsActions(); | ||
| const { isLoadingRawTx, rawTx } = useStacksRawTransaction(txid || ''); | ||
| const { stacksBroadcastTransaction } = useStacksBroadcastTransaction({ | ||
| token: 'STX', | ||
| isCancelTransaction: actionType === 'cancel', | ||
| isIncreaseFeeTransaction: actionType === 'increaseFee', | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (tx && tx.tx_status !== 'pending') { | ||
| toast.info('Your transaction went through!.'); | ||
| } | ||
| }, [toast, tx, tx?.tx_status]); | ||
|
|
||
| const validationSchema = yup.object({ fee: stxFeeValidator(availableUnlockedBalance) }); | ||
|
|
||
| return { | ||
| tx, | ||
| rawTx, | ||
| isLoadingTx, | ||
| isLoadingRawTx, | ||
| refreshAccountData, | ||
| submittedTransactionsActions, | ||
| stacksBroadcastTransaction, | ||
| availableUnlockedBalance, | ||
| validationSchema, | ||
| }; |
There was a problem hiding this comment.
Refactor: Optimize the structure of useStxTransactionActions.
The hook combines multiple responsibilities, which could be separated for better clarity and reusability. Consider splitting the hook into smaller, more focused hooks or helper functions.
- export function useStxTransactionActions(txid: string | undefined, actionType: 'cancel' | 'increaseFee') { ... }
+ // Split into more focused hooks
+ export function useTransactionData(txid: string | undefined) { ... }
+ export function useTransactionStatusNotifier(txid: string | undefined, toast) { ... }
+ export function useTransactionActions(txid: string | undefined, actionType: 'cancel' | 'increaseFee') { ... }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
I think this would be a good idea
There was a problem hiding this comment.
@pete-watters, great! Here's a detailed plan for refactoring the useStxTransactionActions hook into smaller, more focused hooks:
useTransactionDataHook: Responsible for fetching transaction data and raw transaction data.useTransactionStatusNotifierHook: Responsible for notifying the user about the transaction status.useTransactionActionsHook: Responsible for handling transaction actions like cancellation or fee increase.
Here are the code examples for each of the new hooks:
import { useEffect } from 'react';
import * as yup from 'yup';
import { useTransactionById, useStacksRawTransaction, useStxAvailableUnlockedBalance } from '@leather.io/query';
import { useToast } from '@app/features/toasts/use-toast';
import { useCurrentStacksAccountAddress } from '@app/store/accounts/blockchain/stacks/stacks-account.hooks';
import { stxFeeValidator } from '@app/common/validation/forms/fee-validators';
export function useTransactionData(txid: string | undefined) {
const { data: tx, isLoading: isLoadingTx } = useTransactionById(txid || '');
const { isLoading: isLoadingRawTx, data: rawTx } = useStacksRawTransaction(txid || '');
return { tx, isLoadingTx, rawTx, isLoadingRawTx };
}
export function useTransactionStatusNotifier(tx: any) {
const toast = useToast();
useEffect(() => {
if (tx && tx.tx_status !== 'pending') {
toast.info('Your transaction went through!');
}
}, [toast, tx, tx?.tx_status]);
}
export function useTransactionActions(txid: string | undefined, actionType: 'cancel' | 'increaseFee') {
const stxAddress = useCurrentStacksAccountAddress();
const availableUnlockedBalance = useStxAvailableUnlockedBalance(stxAddress);
const validationSchema = yup.object({ fee: stxFeeValidator(availableUnlockedBalance) });
const { stacksBroadcastTransaction } = useStacksBroadcastTransaction({
token: 'STX',
isCancelTransaction: actionType === 'cancel',
isIncreaseFeeTransaction: actionType === 'increaseFee',
});
return {
stacksBroadcastTransaction,
availableUnlockedBalance,
validationSchema,
};
}These hooks can then be used together in a component as shown below:
export function useStxTransactionActions(txid: string | undefined, actionType: 'cancel' | 'increaseFee') {
const { tx, isLoadingTx, rawTx, isLoadingRawTx } = useTransactionData(txid);
useTransactionStatusNotifier(tx);
const { stacksBroadcastTransaction, availableUnlockedBalance, validationSchema } = useTransactionActions(txid, actionType);
return {
tx,
rawTx,
isLoadingTx,
isLoadingRawTx,
stacksBroadcastTransaction,
availableUnlockedBalance,
validationSchema,
};
}This refactor should help improve the clarity and reusability of the code. Let me know if you need any further adjustments or have any questions!
Learnings added
Learnt from: pete-watters
PR: leather-io/extension#5536
File: src/app/features/dialogs/transaction-action-dialog/hooks/use-stx-transaction-actions.ts:18-53
Timestamp: 2024-07-02T12:33:53.994Z
Learning: When refactoring hooks, it's beneficial to split them into smaller, more focused hooks to improve clarity and reusability. For example, the `useStxTransactionActions` hook can be split into `useTransactionData`, `useTransactionStatusNotifier`, and `useTransactionActions`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| export function useStxCancelTransaction(txid: string | undefined) { | ||
| const { | ||
| tx, | ||
| rawTx, | ||
| isLoadingTx, | ||
| isLoadingRawTx, | ||
| refreshAccountData, | ||
| submittedTransactionsActions, | ||
| stacksBroadcastTransaction, | ||
| availableUnlockedBalance, | ||
| validationSchema, | ||
| } = useStxTransactionActions(txid, 'cancel'); | ||
|
|
||
| const network = useCurrentStacksNetworkState(); | ||
| const account = useCurrentStacksAccount(); | ||
|
|
||
| const onSubmit = useCallback( | ||
| async (values: { fee: number }) => { | ||
| if (!tx || !rawTx || !account) return; | ||
|
|
||
| const options: GenerateUnsignedTransactionOptions = { | ||
| publicKey: account.stxPublicKey, | ||
| nonce: tx.nonce, | ||
| fee: stxToMicroStx(values?.fee || 0).toNumber(), | ||
| txData: { | ||
| txType: TransactionTypes.STXTransfer, | ||
| recipient: getBurnAddress(network), | ||
| amount: 1, | ||
| memo: '_cancel transaction', | ||
| network: network, | ||
| } as any, | ||
| }; | ||
| const newTx = await generateUnsignedTransaction(options); | ||
| const txId = tx.tx_id || safelyFormatHexTxid(rawTx.txid()); | ||
| await refreshAccountData(); | ||
| submittedTransactionsActions.transactionReplacedByFee(txId); | ||
| await stacksBroadcastTransaction(newTx); | ||
| }, | ||
| [ | ||
| tx, | ||
| rawTx, | ||
| account, | ||
| network, | ||
| refreshAccountData, | ||
| submittedTransactionsActions, | ||
| stacksBroadcastTransaction, | ||
| ] | ||
| ); | ||
|
|
||
| return { | ||
| availableUnlockedBalance, | ||
| isLoadingRawTx, | ||
| isLoadingTx, | ||
| onSubmit, | ||
| rawTx, | ||
| tx, | ||
| validationSchema, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Enhance: Add error handling to the onSubmit function.
Given the critical nature of transaction cancellation, it's important to robustly handle errors that might occur during the process. This will improve the reliability of the function under various network conditions or unexpected failures.
- await refreshAccountData();
- submittedTransactionsActions.transactionReplacedByFee(txId);
- await stacksBroadcastTransaction(newTx);
+ try {
+ await refreshAccountData();
+ submittedTransactionsActions.transactionReplacedByFee(txId);
+ await stacksBroadcastTransaction(newTx);
+ } catch (error) {
+ console.error('Error during transaction cancellation:', error);
+ }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 useStxCancelTransaction(txid: string | undefined) { | |
| const { | |
| tx, | |
| rawTx, | |
| isLoadingTx, | |
| isLoadingRawTx, | |
| refreshAccountData, | |
| submittedTransactionsActions, | |
| stacksBroadcastTransaction, | |
| availableUnlockedBalance, | |
| validationSchema, | |
| } = useStxTransactionActions(txid, 'cancel'); | |
| const network = useCurrentStacksNetworkState(); | |
| const account = useCurrentStacksAccount(); | |
| const onSubmit = useCallback( | |
| async (values: { fee: number }) => { | |
| if (!tx || !rawTx || !account) return; | |
| const options: GenerateUnsignedTransactionOptions = { | |
| publicKey: account.stxPublicKey, | |
| nonce: tx.nonce, | |
| fee: stxToMicroStx(values?.fee || 0).toNumber(), | |
| txData: { | |
| txType: TransactionTypes.STXTransfer, | |
| recipient: getBurnAddress(network), | |
| amount: 1, | |
| memo: '_cancel transaction', | |
| network: network, | |
| } as any, | |
| }; | |
| const newTx = await generateUnsignedTransaction(options); | |
| const txId = tx.tx_id || safelyFormatHexTxid(rawTx.txid()); | |
| await refreshAccountData(); | |
| submittedTransactionsActions.transactionReplacedByFee(txId); | |
| await stacksBroadcastTransaction(newTx); | |
| }, | |
| [ | |
| tx, | |
| rawTx, | |
| account, | |
| network, | |
| refreshAccountData, | |
| submittedTransactionsActions, | |
| stacksBroadcastTransaction, | |
| ] | |
| ); | |
| return { | |
| availableUnlockedBalance, | |
| isLoadingRawTx, | |
| isLoadingTx, | |
| onSubmit, | |
| rawTx, | |
| tx, | |
| validationSchema, | |
| }; | |
| } | |
| export function useStxCancelTransaction(txid: string | undefined) { | |
| const { | |
| tx, | |
| rawTx, | |
| isLoadingTx, | |
| isLoadingRawTx, | |
| refreshAccountData, | |
| submittedTransactionsActions, | |
| stacksBroadcastTransaction, | |
| availableUnlockedBalance, | |
| validationSchema, | |
| } = useStxTransactionActions(txid, 'cancel'); | |
| const network = useCurrentStacksNetworkState(); | |
| const account = useCurrentStacksAccount(); | |
| const onSubmit = useCallback( | |
| async (values: { fee: number }) => { | |
| if (!tx || !rawTx || !account) return; | |
| const options: GenerateUnsignedTransactionOptions = { | |
| publicKey: account.stxPublicKey, | |
| nonce: tx.nonce, | |
| fee: stxToMicroStx(values?.fee || 0).toNumber(), | |
| txData: { | |
| txType: TransactionTypes.STXTransfer, | |
| recipient: getBurnAddress(network), | |
| amount: 1, | |
| memo: '_cancel transaction', | |
| network: network, | |
| } as any, | |
| }; | |
| const newTx = await generateUnsignedTransaction(options); | |
| const txId = tx.tx_id || safelyFormatHexTxid(rawTx.txid()); | |
| try { | |
| await refreshAccountData(); | |
| submittedTransactionsActions.transactionReplacedByFee(txId); | |
| await stacksBroadcastTransaction(newTx); | |
| } catch (error) { | |
| console.error('Error during transaction cancellation:', error); | |
| } | |
| }, | |
| [ | |
| tx, | |
| rawTx, | |
| account, | |
| network, | |
| refreshAccountData, | |
| submittedTransactionsActions, | |
| stacksBroadcastTransaction, | |
| ] | |
| ); | |
| return { | |
| availableUnlockedBalance, | |
| isLoadingRawTx, | |
| isLoadingTx, | |
| onSubmit, | |
| rawTx, | |
| tx, | |
| validationSchema, | |
| }; | |
| } |
| const navigate = useNavigate(); | ||
| const { whenWallet } = useWalletType(); | ||
|
|
||
| const cancelTransactionMatch = useMatch('/cancel-transaction/stx/:txid'); | ||
| const increaseFeeMatch = useMatch('/increase-fee/stx/:txid'); |
There was a problem hiding this comment.
We have this in IncreaseStxFee in route-urls. Can you please use that and add cancel-transaction there also?
| const actionButtonGroup = ( | ||
| <HStack alignItems="start" gap="space.01"> | ||
| <TransactionActionButton | ||
| key="Cancel" |
There was a problem hiding this comment.
I don't think you need to add a key prop here
| return ( | ||
| <styled.button | ||
| _hover={{ color: 'ink.text-subdued' }} | ||
| bg="ink.background-primary" | ||
| maxWidth="110px" | ||
| maxWidth={maxWidth} |
There was a problem hiding this comment.
Maybe you can just keep the maxWidth="110px" rather than exposing it as a prop and passing it in ?
There was a problem hiding this comment.
Maybe you can just keep the
maxWidth="110px"rather than exposing it as a prop and passing it in ?
@pete-watters yeah did this too in my PR; i was trying to implement the integration test but looks like there are a lot of selectors to be added. Can i leave the integration test to the team? i can fix the newly added reviews and also rebase with new changes.
|
Thanks for all your hard work here @Aman-zishan , it's getting close! I added some comments but the main issue to fix first is that when I checkout your branch there are a few typescript errors. You can check them yourself locally running |
|
@Aman-zishan any chance you've had a chance to take a look at the latest here? |
@markmhendrickson |
|
I'm closing this off for now as it's a bit outdated and we have some more work to finish in #5533 . We discussed it today and will schedule working on this and getting it finished |
This PR is from an external contributor #5533
Summary by CodeRabbit
New Features
StxTransactionActionDialogcomponent for managing transaction actions within a dialog interface.Improvements
Bug Fixes
Refactor
IncreaseFeeButtonwithTransactionActionButtonfor better consistency.