-
-
Notifications
You must be signed in to change notification settings - Fork 256
refactor: deprecate transaction history and sendFlowHistory properties
#7326
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
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
history and sendFlowHistory properties
|
Extension draft PR: MetaMask/metamask-extension#38665 |
| /** TransactionController constructor options. */ | ||
| export type TransactionControllerOptions = { | ||
| /** Whether to disable storing history in transaction metadata. */ | ||
| /** @deprecated Whether to disable storing history in transaction metadata. */ |
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.
Can we update all the deprecated comments to just confirm No longer used for example?
| readonly #trace: TraceCallback; | ||
|
|
||
| readonly #transactionHistoryLimit: number; | ||
| // readonly #transactionHistoryLimit: number; |
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.
This isn't related to the history properties, but how many transactions we retain. We need to keep this.
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.
reverted a changes related with transactionHistoryLimit
| ); | ||
|
|
||
| this.update((state) => { | ||
| state.transactions = this.#trimTransactionsForState(newTransactions); |
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.
We still need to trim the transactions according to the transaction state limit?
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.
Reverted #trimTransactionsForState.
| * @param sendFlowHistoryToAdd - The sendFlowHistory entries to add. | ||
| * @returns The updated transactionMeta. | ||
| */ | ||
| updateTransactionSendFlowHistory( |
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.
If we're avoiding breaking changes in this PR, should we empty this and mark it deprecated?
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.
Marked as deprecated and removed code.
| it('does not if all unique transactions are truncated', async () => { | ||
| const helper = new IncomingTransactionHelper({ | ||
| ...CONTROLLER_ARGS_MOCK, | ||
| trimTransactions: (): TransactionMeta[] => [], |
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.
Still needed as mentioned in other comment.
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.
rollback this change.
Co-authored-by: Matthew Walsh <[email protected]>
…e into fix/remove-history-property
Explanation
This PR deprecates the
historyandsendFlowHistoryproperties and all the code related.Problem
Extension state size has been identified as a major root cause for the app becoming bricked, particularly for power users. Analysis by the platform team revealed that
historyandsendFlowHistoryare among the biggest contributors to excessive state size, leading to frequent disk writes and the infinite spinner issue.References
fixes https://github.com/MetaMask/MetaMask-planning/issues/2465
Checklist
Note
Deprecates and removes internal handling of
historyandsendFlowHistoryfrom TransactionController and types, simplifying updates and state while keeping deprecated fields for compatibility.utils/historyand all calls toaddInitialHistorySnapshot/updateTransactionHistory.historyandsendFlowHistoryonTransactionMeta; mark related types (TransactionHistory,TransactionHistoryEntry,SendFlowHistoryEntry) as deprecated.disableHistoryanddisableSendFlowHistory(no longer used); keep for backward compatibility.updateTransactionSendFlowHistorya no-op returning the transaction unchanged.updateTransactionto log the note instead of writing to history.index.ts.history/sendFlowHistory; deleteutils/history.test.ts; adjust unit/integration tests accordingly.Written by Cursor Bugbot for commit 0e32918. This will update automatically on new commits. Configure here.