-
-
Notifications
You must be signed in to change notification settings - Fork 268
feat: implement enhanced transaction history retrieval #7689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…aMask/core into enhanced_transaction_history
|
|
||
| readonly #updateTransactions?: boolean; | ||
|
|
||
| readonly #isEnhancedHistoryEnabled: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, alphabetical properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable name is changed now.
| let connectionStateChangedHandler: (info: WebSocketConnectionInfo) => void; | ||
| let selectedAccountChangedHandler: () => void; | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, we usually avoid nested beforeEach and variables in describe to help readability and reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the beforeEach small but these test cases need different mocking for feature flag thus this nested beforeEach is needed.
| */ | ||
| export enum FeatureFlag { | ||
| EIP7702 = 'confirmations_eip_7702', | ||
| EnhancedHistoryRetrieval = 'enhanced_history_retrieval', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a stand alone boolean, can we instead re-use confirmations_incoming_transactions?
And could we be more specific with the property name, such as useWebsockets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I updated the PR
…aMask/core into enhanced_transaction_history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Explanation
This PR implements enhanced transaction history retrieval for the IncomingTransactionHelper. When the enhanced_history_retrieval feature flag is enabled and WebSocket is connected, incoming transactions are fetched via event-driven updates instead of polling, reducing unnecessary API calls and improving responsiveness.
References
Ref: https://github.com/MetaMask/MetaMask-planning/issues/6682
Checklist
Note
Introduces enhanced, event-driven incoming transaction history retrieval.
IncomingTransactionHelpersubscribes toBackendWebSocketService:connectionStateChanged,AccountActivityService:transactionUpdated, andAccountsController:selectedAccountChange; polling is skipped when enabledisIncomingTransactionsUseWebsocketsEnabledfeature flag and tests; retains legacy polling when disabledAllowedEventsinTransactionControllerand adds@metamask/core-backenddependency plus TS project referencesWritten by Cursor Bugbot for commit 8453f3b. This will update automatically on new commits. Configure here.