feat: implement distinction between BumpFee RBF & Cancel RBF -> utilise it to display error in case TX gets confirmed in cancel flow #16905
Conversation
WalkthroughThis pull request implements a comprehensive renaming and type update across multiple modules handling transaction logic. The primary change is the renaming of the type Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🚀 Expo preview is ready!
|
peter-sanderson
force-pushed
the
cancel-transaction-screen-2-error-for-cancel-sign
branch
from
ac9840b to
f1a775a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-common/wallet-core/src/send/sendFormThunks.ts (1)
542-553: Consider using a type guard to simplify type assertions.The multiple type assertions to
PrecomposedTransactionFinalBumpFeeRbfcould be simplified using a type guard function.Consider adding a type guard:
function isBumpFeeRbf(tx: GeneralPrecomposedTransactionFinal): tx is PrecomposedTransactionFinalBumpFeeRbf { return 'prevTxid' in tx && 'feeDifference' in tx; }Then simplify the code:
- (enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf).prevTxid = - formValues.rbfParams.txid; - (enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf).feeDifference = - new BigNumber(precomposedTransaction.fee) - .minus(formValues.rbfParams.baseFee) - .toFixed(); - (enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf).useNativeRbf = - !!useNativeRbf; - ( - enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf - ).useDecreaseOutput = !!hasDecreasedOutput; + const bumpFeeTx = enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf; + bumpFeeTx.prevTxid = formValues.rbfParams.txid; + bumpFeeTx.feeDifference = new BigNumber(precomposedTransaction.fee) + .minus(formValues.rbfParams.baseFee) + .toFixed(); + bumpFeeTx.useNativeRbf = !!useNativeRbf; + bumpFeeTx.useDecreaseOutput = !!hasDecreasedOutput;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/suite/src/actions/wallet/send/sendFormThunks.ts(4 hunks)packages/suite/src/actions/wallet/stakeActions.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(6 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx(3 hunks)packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts(1 hunks)packages/suite/src/utils/suite/transactionReview.ts(2 hunks)suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(2 hunks)suite-common/wallet-core/src/transactions/transactionsThunks.ts(5 hunks)suite-common/wallet-types/src/transaction.ts(2 hunks)suite-common/wallet-utils/src/accountUtils.ts(2 hunks)suite-common/wallet-utils/src/reviewTransactionUtils.ts(3 hunks)suite-common/wallet-utils/src/transactionUtils.ts(2 hunks)suite-native/module-send/src/selectors.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/suite/src/actions/wallet/stakeActions.ts
- suite-native/module-send/src/selectors.ts
- suite-common/wallet-utils/src/reviewTransactionUtils.ts
- suite-common/wallet-utils/src/accountUtils.ts
- suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
- suite-common/wallet-core/src/transactions/transactionsThunks.ts
- suite-common/wallet-utils/src/transactionUtils.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (6)
packages/suite/src/utils/suite/transactionReview.ts (1)
4-9: LGTM! Clear distinction between RBF actions.The changes effectively separate RBF actions into bump fee and cancel operations, improving clarity and type safety.
Also applies to: 11-16, 27-33
packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts (2)
11-21: LGTM! Well-structured helper function.The
getPrevTxidhelper function effectively handles both bump fee and cancel RBF cases with proper null safety.
29-43: LGTM! Improved error handling flow.The middleware's early return pattern and null checks enhance code reliability.
suite-common/wallet-types/src/transaction.ts (3)
123-129: LGTM! Clear type definition for bump fee transactions.The renamed type
PrecomposedTransactionFinalBumpFeeRbfmaintains all necessary properties with clear documentation.
131-133: LGTM! Well-defined cancel transaction type.The new
PrecomposedTransactionFinalCancelRbftype correctly captures the requirements for cancel transactions.
135-139: LGTM! Clear union type definition.The updated
PrecomposedTransactionFinaltype properly represents all transaction variants.
peter-sanderson
force-pushed
the
cancel-transaction-screen-2-error-for-cancel-sign
branch
from
f1a775a to
44f7fb1
Compare
...odals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx
Outdated
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
130-163: Consider refactoring the nested ternary operator.While the logic is correct, the nested ternary operator can be hard to read. Consider using a more readable approach.
- reportTransactionCreatedEvent( - // eslint-disable-next-line no-nested-ternary - isBumpFeeRbfAction ? 'replaced' : isCancelRbfAction ? 'canceled' : 'sent', - ); + const getTransactionAction = () => { + if (isBumpFeeRbfAction) return 'replaced'; + if (isCancelRbfAction) return 'canceled'; + return 'sent'; + }; + reportTransactionCreatedEvent(getTransactionAction());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/suite-analytics/src/types/events.ts(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: EAS Update
🔇 Additional comments (5)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (4)
17-18: LGTM!The imports correctly reflect the new distinction between BumpFee RBF and Cancel RBF transactions.
74-75: LGTM!The variable renaming from
isRbfActiontoisBumpFeeRbfActionand the addition ofisCancelRbfActionimprove code clarity by explicitly distinguishing between different RBF types.Also applies to: 119-119
238-242: LGTM!The component now correctly displays different error messages based on whether it's a cancel action or a replace-by-fee action.
252-252: LGTM!The prop renaming maintains consistency with the variable renaming changes.
packages/suite-analytics/src/types/events.ts (1)
204-204: LGTM!The addition of 'canceled' to the action type union correctly reflects the new possible action for transaction analytics.
peter-sanderson
force-pushed
the
cancel-transaction-screen-2-error-for-cancel-sign
branch
from
5ed49fb to
5a88230
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-common/wallet-core/src/send/sendFormThunks.ts (1)
542-554: Reduce repeated type casting by using a temporary variable.The code performs the same type cast multiple times which could be simplified for better maintainability.
Consider this refactor:
- (enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf).prevTxid = - formValues.rbfParams.txid; - ( - enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf - ).feeDifference = new BigNumber(precomposedTransaction.fee) - .minus(formValues.rbfParams.baseFee) - .toFixed(); - ( - enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf - ).useNativeRbf = !!useNativeRbf; - ( - enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf - ).useDecreaseOutput = !!hasDecreasedOutput; + const bumpFeeRbf = enhancedPrecomposedTransaction as PrecomposedTransactionFinalBumpFeeRbf; + bumpFeeRbf.prevTxid = formValues.rbfParams.txid; + bumpFeeRbf.feeDifference = new BigNumber(precomposedTransaction.fee) + .minus(formValues.rbfParams.baseFee) + .toFixed(); + bumpFeeRbf.useNativeRbf = !!useNativeRbf; + bumpFeeRbf.useDecreaseOutput = !!hasDecreasedOutput;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/suite-analytics/src/types/events.ts(1 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(4 hunks)packages/suite/src/actions/wallet/stakeActions.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(6 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx(3 hunks)packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts(1 hunks)packages/suite/src/utils/suite/transactionReview.ts(2 hunks)suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(2 hunks)suite-common/wallet-core/src/transactions/transactionsThunks.ts(5 hunks)suite-common/wallet-types/src/transaction.ts(2 hunks)suite-common/wallet-utils/src/accountUtils.ts(2 hunks)suite-common/wallet-utils/src/reviewTransactionUtils.ts(4 hunks)suite-common/wallet-utils/src/transactionUtils.ts(2 hunks)suite-native/module-send/src/selectors.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- suite-native/module-send/src/selectors.ts
- packages/suite/src/actions/wallet/stakeActions.ts
- packages/suite-analytics/src/types/events.ts
- suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts
- packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts
- suite-common/wallet-types/src/transaction.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx
- suite-common/wallet-utils/src/accountUtils.ts
- packages/suite/src/utils/suite/transactionReview.ts
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
- suite-common/wallet-utils/src/reviewTransactionUtils.ts
- suite-common/wallet-core/src/transactions/transactionsThunks.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (3)
suite-common/wallet-core/src/send/sendFormThunks.ts (1)
16-16: LGTM! Type import aligns with RBF distinction.The addition of
PrecomposedTransactionFinalBumpFeeRbftype import supports the PR's objective to distinguish between BumpFee RBF and Cancel RBF transactions.suite-common/wallet-utils/src/transactionUtils.ts (2)
13-14: LGTM! Import changes align with the PR objectives.The addition of
PrecomposedTransactionFinalBumpFeeRbfandPrecomposedTransactionFinalCancelRbftypes supports the distinction between BumpFee RBF and Cancel RBF transactions.
66-73: LGTM! Function changes implement clear distinction between RBF types.The implementation correctly distinguishes between BumpFee RBF and Cancel RBF transactions through distinct type guards and property checks.
Let's verify the usage of these functions in the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the usage of RBF type guard functions. # Test: Search for usages of both functions. Expect: Consistent usage across the codebase. echo "Searching for isRbfBumpFeeTransaction usage:" rg -A 2 $'isRbfBumpFeeTransaction' echo -e "\nSearching for isRbfCancelTransaction usage:" rg -A 2 $'isRbfCancelTransaction'Length of output: 8698
LGTM! The type guards now are correctly implemented and used consistently across the codebase.
- isRbfBumpFeeTransaction is widely referenced in transaction processing and state management files.
- isRbfCancelTransaction is appropriately used in transaction review components and error middleware.
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
peter-sanderson
force-pushed
the
cancel-transaction-screen-2-error-for-cancel-sign
branch
from
5a88230 to
4acdabc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
160-163: Consider using early returns for better readability.The nested ternary operator makes the code harder to read.
- reportTransactionCreatedEvent( - // eslint-disable-next-line no-nested-ternary - isBumpFeeRbfAction ? 'replaced' : isCancelRbfAction ? 'canceled' : 'sent', - ); + if (isBumpFeeRbfAction) return reportTransactionCreatedEvent('replaced'); + if (isCancelRbfAction) return reportTransactionCreatedEvent('canceled'); + reportTransactionCreatedEvent('sent');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/suite-analytics/src/types/events.ts(1 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(4 hunks)packages/suite/src/actions/wallet/stakeActions.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(6 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx(3 hunks)packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts(1 hunks)packages/suite/src/utils/suite/transactionReview.ts(2 hunks)suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(2 hunks)suite-common/wallet-core/src/transactions/transactionsThunks.ts(5 hunks)suite-common/wallet-types/src/transaction.ts(3 hunks)suite-common/wallet-utils/src/accountUtils.ts(2 hunks)suite-common/wallet-utils/src/reviewTransactionUtils.ts(4 hunks)suite-common/wallet-utils/src/transactionUtils.ts(2 hunks)suite-native/module-send/src/selectors.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- suite-native/module-send/src/selectors.ts
- packages/suite/src/actions/wallet/stakeActions.ts
- packages/suite-analytics/src/types/events.ts
- packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts
- suite-common/wallet-utils/src/reviewTransactionUtils.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx
- suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts
- suite-common/wallet-core/src/transactions/transactionsThunks.ts
- packages/suite/src/utils/suite/transactionReview.ts
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- suite-common/wallet-utils/src/accountUtils.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (12)
suite-common/wallet-types/src/transaction.ts (4)
106-112: LGTM!The renaming of
TxFinaltoPrecomposedTransactionBasebetter reflects its role as a base type for all final transactions.
123-130: LGTM!The
PrecomposedTransactionFinalBumpFeeRbftype correctly extends the base type and adds RBF-specific properties with a discriminator propertyrbfType: 'bump-fee'.
132-135: LGTM!The
PrecomposedTransactionFinalCancelRbftype correctly extends the base type and adds cancel-specific properties with a discriminator propertyrbfType: 'cancel'.
137-141: LGTM!The union type
PrecomposedTransactionFinalnow clearly distinguishes between normal, bump-fee, and cancel transactions using the discriminator property.packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (3)
74-75: LGTM!The variable name
isBumpFeeRbfActionand the use ofisRbfBumpFeeTransactionclearly indicate the type of RBF action being performed.
119-119: LGTM!Added check for cancel RBF transactions using the new type guard function.
238-242: LGTM!The component correctly displays different error messages based on whether it's a cancel or replace-by-fee transaction.
suite-common/wallet-core/src/send/sendFormThunks.ts (2)
536-553: LGTM!The
createGeneralPrecomposedTransactionFinalfunction correctly handles the enhancement of RBF transactions:
- Checks for non-Cardano transactions with RBF parameters
- Adds the
rbfTypediscriminator- Preserves all required RBF-specific properties
555-555: LGTM!The function is correctly used to enhance the precomposed transaction.
suite-common/wallet-utils/src/transactionUtils.ts (3)
66-69: LGTM!The
isRbfTransactiontype guard correctly uses therbfTypediscriminator to identify RBF transactions.
71-74: LGTM!The
isRbfBumpFeeTransactiontype guard correctly identifies bump fee transactions by checking both the presence ofrbfTypeand its value.
75-77: LGTM!The
isRbfCancelTransactiontype guard correctly identifies cancel transactions by checking both the presence ofrbfTypeand its value.
| return precomposedTransaction; | ||
| }; | ||
|
|
||
| const enhancedPrecomposedTransaction = createGeneralPrecomposedTransactionFinal(); |
There was a problem hiding this comment.
This whole diff is awesome 🎉 the original code was barely legible with (as) on lefthand side of assignment operation 😅
peter-sanderson
force-pushed
the
cancel-transaction-screen-2-error-for-cancel-sign
branch
from
ece6c12 to
a499e63
Compare
| @@ -70,10 +71,13 @@ export const TransactionReviewModalContent = ({ | |||
| ); | |||
|
|
|||
| const isTradingAction = !!precomposedForm?.isTrading; | |||
| const isRbfAction = precomposedTx !== undefined && isRbfTransaction(precomposedTx); | |||
| const isBumpFeeRbfAction = | |||
| precomposedTx !== undefined && isRbfBumpFeeTransaction(precomposedTx); | |||
There was a problem hiding this comment.
nit: handling of undefined could be moved inside of the isRbfBumpFeeTransaction type guard.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
167-171: Simplify the event reporting logic.The nested ternary could be simplified using early returns for better readability.
- reportTransactionCreatedEvent( - isRbfTransaction(precomposedTx) - ? mapRbfTypeToReporting[precomposedTx.rbfType] - : 'sent', - ); + if (isRbfTransaction(precomposedTx)) { + return reportTransactionCreatedEvent(mapRbfTypeToReporting[precomposedTx.rbfType]); + } + reportTransactionCreatedEvent('sent');
🛑 Comments failed to post (1)
packages/suite-analytics/src/types/events.ts (1)
39-55: 💡 Verification agent
🧩 Analysis chain
LGTM! New analytics event type is well-structured.
The
TransactionCreatedEventtype is well-defined with a comprehensive payload structure that includes the new 'canceled' action.Based on the past review comments, please verify that the analytics documentation has been updated to reflect these changes:
🌐 Web query:
Is there documentation for the "canceled" transaction action in the Trezor Suite analytics documentation?💡 Result:
Trezor Suite's analytics documentation does not explicitly mention a "canceled" transaction action as a tracked event. However, several related aspects of transaction handling and error tracking are documented:
Error Tracking:
Trezor Suite uses Sentry.io for error monitoring[1], which would capture transaction failures or rejections like canceled actions. The system stores error-related data for 90 days[1].Transaction Status Terminology:
Code references show status labels like:
TR_EXCHANGE_STATUS_ERROR: 'Rejected'[2]TR_EXCHANGE_DETAIL_ERROR_TITLE: 'Transaction failed'[2]
While not explicitly "canceled," these indicate similar terminal states.Data Collection Scope:
Analytics collect "user interactions with app functions" and errors[1], suggesting canceled transactions would fall under either:
- Functional interaction data (if user-initiated cancellation)
- Error reports (if system-rejected)[1]
Export Capabilities:
Transaction exports (JSON/CSV/PDF) include comprehensive metadata[5], though cancellation status isn't specifically mentioned in documentation.There's no direct reference to a dedicated "canceled" transaction metric in the official docs[1][3][9], but the infrastructure exists to capture related events through error tracking and interaction logging[1][2]. For implementation details, developers would need to consult Trezor Suite's source code[2].
Citations:
- 1: https://docs.trezor.io/trezor-suite/analytics/
- 2: https://github.com/trezor/trezor-suite/blob/develop/packages/suite/src/support/messages.ts
- 3: https://docs.trezor.io/trezor-suite/
- 4: https://www.nuvei.com/solutions/fraud-risk-management?818a72c6_page=4&818a72f8_page=6
- 5: https://docs.trezor.io/trezor-suite/features/transactions/export.html
- 6: https://securityaffairs.com/173784/cyber-crime/crazy-evil-runs-10-social-media-scams.html
- 7: https://trezor.io/learn
- 8: https://www.calibraint.com/blog/crypto-order-book-data-strategies-guide
- 9: https://trezor.io/learn/a/user-data-protection
- 10: https://cases.stretto.com/public/X274/12452/PLEADINGS/1245211212480000000130.pdf
Documentation Update Needed: "canceled" Transaction Event Missing
The analytics documentation does not explicitly document the new "canceled" transaction action. While error tracking and related behaviors are mentioned, there’s no dedicated explanation or reference to this new action.
- Verify if the intended design was to document "canceled" directly.
- Consider updating the analytics docs to explicitly include the "canceled" transaction action for clarity.
peter-sanderson
force-pushed
the
cancel-transaction-screen-2-error-for-cancel-sign
branch
from
a499e63 to
d544557
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-common/wallet-core/src/send/sendFormThunks.ts (1)
536-553: Add JSDoc documentation for the new function.The function
createGeneralPrecomposedTransactionFinalhandles important RBF transaction logic. Consider adding JSDoc documentation to explain:
- The purpose of the function
- The conditions under which it enhances transactions
- The significance of the RBF properties being set
+/** + * Creates an enhanced transaction with RBF (Replace-By-Fee) properties if applicable. + * For non-Cardano transactions with RBF parameters, it adds bump-fee specific properties + * such as rbfType, prevTxid, feeDifference, and RBF capability flags. + * @returns {GeneralPrecomposedTransactionFinal} Enhanced transaction with RBF properties if applicable + */ const createGeneralPrecomposedTransactionFinal = (): GeneralPrecomposedTransactionFinal => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/suite-analytics/src/types/events.ts(2 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(4 hunks)packages/suite/src/actions/wallet/stakeActions.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(7 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/BumpFeeModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx(2 hunks)packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts(1 hunks)packages/suite/src/utils/suite/transactionReview.ts(2 hunks)suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(2 hunks)suite-common/wallet-core/src/transactions/transactionsThunks.ts(5 hunks)suite-common/wallet-types/src/transaction.ts(3 hunks)suite-common/wallet-utils/src/accountUtils.ts(2 hunks)suite-common/wallet-utils/src/reviewTransactionUtils.ts(4 hunks)suite-common/wallet-utils/src/transactionUtils.ts(2 hunks)suite-native/module-send/src/selectors.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/suite/src/actions/wallet/stakeActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/BumpFeeModal.tsx
- packages/suite-analytics/src/types/events.ts
- packages/suite/src/utils/suite/transactionReview.ts
- suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx
- suite-common/wallet-utils/src/reviewTransactionUtils.ts
- suite-native/module-send/src/selectors.ts
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- suite-common/wallet-core/src/transactions/transactionsThunks.ts
- suite-common/wallet-utils/src/accountUtils.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
- packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts
- suite-common/wallet-types/src/transaction.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
🔇 Additional comments (5)
suite-common/wallet-utils/src/transactionUtils.ts (2)
13-14: LGTM! Import changes align with RBF distinction.The addition of separate types for BumpFee and Cancel RBF transactions enhances type safety and clarity.
66-78: LGTM! Well-structured type guards for RBF transactions.The implementation:
- Uses TypeScript type guards effectively for type safety
- Clearly distinguishes between BumpFee and Cancel RBF transactions
- Follows a consistent pattern across all three functions
suite-common/wallet-core/src/send/sendFormThunks.ts (3)
16-16: LGTM! Import of new RBF type.The import of
PrecomposedTransactionFinalBumpFeeRbfaligns with the PR's objective to distinguish between different RBF transaction types.
555-555: LGTM! Clean integration of the enhanced transaction.The enhanced transaction is properly integrated into the existing flow, maintaining compatibility with token verification and redux state management.
Also applies to: 576-581
538-547: Verify the impact on Cancel RBF flow.The implementation clearly handles the bump-fee RBF case. Let's verify that this doesn't adversely affect the cancel RBF flow.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for cancel RBF related code to ensure it's properly handled rg -A 5 "rbfType.*cancel" || echo "No explicit cancel RBF type handling found" # Search for RBF type definitions ast-grep --pattern 'type RbfTransactionType = $_'Length of output: 2157
Verify Cancel RBF Flow Handling
The shell script output confirms that the cancel RBF flow is explicitly managed in the codebase:
- The
CancelTransactionModal.tsxfile setsrbfTypeto'cancel'when preparing a cancel transaction.- The type definition in
suite-common/wallet-types/src/transaction.tsclearly distinguishes between'bump-fee'and'cancel'via theRbfTransactionTypeunion.These findings indicate that the bump-fee enhancements will not adversely affect the cancel RBF flow.
peter-sanderson
changed the title
peter-sanderson
changed the title
…se it to display error in case TX gets confirmed in cancel flow feat: extend reporting for 'canceled' event in sendform
peter-sanderson
force-pushed
the
cancel-transaction-screen-2-error-for-cancel-sign
branch
from
d544557 to
817360d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-common/wallet-core/src/send/sendFormThunks.ts (1)
536-556: Consider enhancing type safety and modularity.The implementation of
createGeneralPrecomposedTransactionFinalis solid and aligns well with the PR's objective. However, there are a few potential improvements:
- Extract fee difference calculation to a separate function for better modularity.
- Add type guard for
formValues.rbfParamsto enhance type safety.Consider this refactoring:
+const calculateFeeDifference = (newFee: string, baseFee: string): string => + new BigNumber(newFee).minus(baseFee).toFixed(); const createGeneralPrecomposedTransactionFinal = (): GeneralPrecomposedTransactionFinal => { - if (!isCardanoTx(selectedAccount, precomposedTransaction) && formValues.rbfParams) { + if (!isCardanoTx(selectedAccount, precomposedTransaction) && + formValues.rbfParams && + 'fee' in precomposedTransaction) { const enhancedRbfPrecomposedTx: PrecomposedTransactionFinalBumpFeeRbf = { ...precomposedTransaction, rbfType: 'bump-fee', prevTxid: formValues.rbfParams.txid, - feeDifference: new BigNumber(precomposedTransaction.fee) - .minus(formValues.rbfParams.baseFee) - .toFixed(), + feeDifference: calculateFeeDifference( + precomposedTransaction.fee, + formValues.rbfParams.baseFee + ), useNativeRbf: !!useNativeRbf, useDecreaseOutput: !!hasDecreasedOutput, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/suite-analytics/src/types/events.ts(2 hunks)packages/suite/src/actions/wallet/send/sendFormThunks.ts(4 hunks)packages/suite/src/actions/wallet/stakeActions.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(7 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/BumpFeeModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx(2 hunks)packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts(1 hunks)packages/suite/src/utils/suite/transactionReview.ts(2 hunks)suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts(2 hunks)suite-common/wallet-core/src/send/sendFormThunks.ts(2 hunks)suite-common/wallet-core/src/transactions/transactionsThunks.ts(5 hunks)suite-common/wallet-types/src/transaction.ts(3 hunks)suite-common/wallet-utils/src/accountUtils.ts(2 hunks)suite-common/wallet-utils/src/reviewTransactionUtils.ts(4 hunks)suite-common/wallet-utils/src/transactionUtils.ts(2 hunks)suite-native/module-send/src/selectors.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/suite-analytics/src/types/events.ts
- packages/suite/src/actions/wallet/stakeActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/BumpFeeModal.tsx
- suite-common/wallet-core/src/send/sendFormBitcoinThunks.ts
- packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts
- suite-native/module-send/src/selectors.ts
- suite-common/wallet-utils/src/accountUtils.ts
- packages/suite/src/utils/suite/transactionReview.ts
- suite-common/wallet-utils/src/reviewTransactionUtils.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx
- packages/suite/src/actions/wallet/send/sendFormThunks.ts
- suite-common/wallet-core/src/transactions/transactionsThunks.ts
- suite-common/wallet-types/src/transaction.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (9)
suite-common/wallet-utils/src/transactionUtils.ts (3)
13-14: LGTM! Clear type imports for RBF transaction handling.The addition of separate types for BumpFee and Cancel RBF transactions improves type safety and clarity.
66-69: LGTM! Improved type safety in RBF transaction check.The function now correctly returns a union type and uses a more reliable property check.
71-77: LGTM! Well-structured type guard functions.The new functions provide clear and type-safe ways to distinguish between bump-fee and cancel RBF transactions. The implementation is clean and follows TypeScript best practices.
suite-common/wallet-core/src/send/sendFormThunks.ts (1)
16-16: LGTM!The addition of
PrecomposedTransactionFinalBumpFeeRbftype import aligns with the PR's objective to distinguish between different RBF transaction types.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx (5)
1-1: LGTM! Good type safety improvements.The changes improve type safety by:
- Using a dedicated
RbfTransactionTypeinstead of string literals- Making the type definition exportable for reuse
Also applies to: 13-15
32-51: LGTM! Clean and user-friendly error UI implementation.The component provides a good user experience by:
- Displaying a clear warning icon
- Showing appropriate error messages based on the RBF type
- Providing help resources through documentation links
22-25: Verify description translation keys exist.Please ensure that the translation keys
TR_REPLACE_BY_FEE_FAILED_ALREADY_MINED_DESCRIPTIONandTR_CANCEL_TX_FAILED_ALREADY_MINED_DESCRIPTIONexist in the translation files.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the translation keys in translation files rg "TR_REPLACE_BY_FEE_FAILED_ALREADY_MINED_DESCRIPTION|TR_CANCEL_TX_FAILED_ALREADY_MINED_DESCRIPTION" -A 1Length of output: 2189
Translation key verification completed: Keys exist.
Both translation keys
TR_REPLACE_BY_FEE_FAILED_ALREADY_MINED_DESCRIPTIONandTR_CANCEL_TX_FAILED_ALREADY_MINED_DESCRIPTIONwere found in the translation data as well as referenced in support messages.
17-20: Verify translation keys exist.Please ensure that the translation keys
TR_REPLACE_BY_FEE_FAILED_ALREADY_MINEDandTR_CANCEL_TX_FAILED_ALREADY_MINEDexist in the translation files.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the translation keys in translation files rg "TR_REPLACE_BY_FEE_FAILED_ALREADY_MINED|TR_CANCEL_TX_FAILED_ALREADY_MINED" -A 1Length of output: 3675
Translation keys have been verified successfully.
- The keys
TR_REPLACE_BY_FEE_FAILED_ALREADY_MINEDandTR_CANCEL_TX_FAILED_ALREADY_MINEDare present in the English translation file (packages/suite-data/files/translations/en.json) as well as in the support messages file.- Both the primary keys and their corresponding description keys exist, ensuring proper mapping for UI messages.
27-30: Verify help center URLs are valid.Please ensure that the imported URL constants
HELP_CENTER_REPLACE_BY_FEE_BITCOINandHELP_CENTER_CANCEL_TRANSACTIONpoint to valid help center articles.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for URL constant definitions rg "HELP_CENTER_REPLACE_BY_FEE_BITCOIN|HELP_CENTER_CANCEL_TRANSACTION.*=.*" -A 1Length of output: 1749
Help Center URLs Verified
The search confirms that the URL constants are defined in the repository as follows:
- HELP_CENTER_REPLACE_BY_FEE_BITCOIN:
'https://trezor.io/learn/a/replace-by-fee-rbf-bitcoin'- HELP_CENTER_CANCEL_TRANSACTION:
'https://trezor.io/support/a/can-i-cancel-or-reverse-a-transaction'Both constants correctly point to valid help center articles as expected. No issues were found.
peter-sanderson
deleted the
cancel-transaction-screen-2-error-for-cancel-sign
branch
Resolve: #16875