fix: Add simulation retrigger #4792
Conversation
|
@metamaskbot publish-preview |
| @@ -1307,6 +1307,9 @@ export type SimulationError = { | |||
|
|
|||
| /** Simulation data for a transaction. */ | |||
| export type SimulationData = { | |||
| /** Set to true if simulationData changed after updating security alert */ | |||
| changeInSimulationData?: boolean; | |||
There was a problem hiding this comment.
What is the purpose of this property?
So the client can trigger an additional alert if the simulation isn't consistent?
If so, how do we differentiate between expected updates if the user changes the value or data?
Is this needed at all?
There was a problem hiding this comment.
According to the designs, UI need to know if that re-simulation done because of the security alert. I thought this will be extra overhead in the UI to old and new values, so I've put it because of that.
Also I didn't have a chance to test this one out yet, so please consider this as draft for now.
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| result_type: string; |
There was a problem hiding this comment.
Since string encompasses all the BlockaidResultType options, should we just leave it as string to minimise coupling to Blockaid?
| return transactionMeta; | ||
| } | ||
|
|
||
| #checkIfSimulationRetriggerNeeded(transactionMeta: TransactionMeta) { |
There was a problem hiding this comment.
Won't this keep firing unless we also pass the previousTransactionMeta and also check if any of the relevant inputs have changed?
There was a problem hiding this comment.
Good spot Matthew, this is now fixed, comparing before and after
| securityAlertResponse?.result_type === BlockaidResultType.Malicious; | ||
| const simulationNativeBalanceDifference = | ||
| simulationData?.nativeBalanceChange?.difference; | ||
| const isBalanceDifferenceEqual = |
There was a problem hiding this comment.
Are we definitely looking for an exact comparison rather than a percentage difference?
There was a problem hiding this comment.
What do we think about the value on this? Would it make sense to make it 5%?
| return false; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Minor, can we simplify to a single return since it's a boolean?
| @@ -3599,9 +3600,42 @@ export class TransactionController extends BaseController< | |||
| ); | |||
| } | |||
|
|
|||
| if (this.#checkIfSimulationRetriggerNeeded(transactionMeta)) { | |||
There was a problem hiding this comment.
We still have the condition above and onTransactionParamsUpdated that also re-triggers a simulation, is it worth combining those for clarity?
There was a problem hiding this comment.
I've refactored that part a bit more, feedbacks appreciated!
| @@ -1368,3 +1371,14 @@ export type SubmitHistoryEntry = { | |||
| /** The transaction parameters that were submitted. */ | |||
| transaction: TransactionParams; | |||
| }; | |||
|
|
|||
| export enum BlockaidResultType { | |||
There was a problem hiding this comment.
Rather than defining a full enum, could we just define a local constant such as BLOCKAID_RESULT_TYPE_MALICIOUS?
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
Removed dependencies detected. Learn more about Socket for GitHub ↗︎ 🚮 Removed packages: npm/[email protected] |
Explanation
This PR adds a mechanism to re-trigger of simulations if the security provider mark transaction as
maliciousand the previous simulation native balance change is different then the previous simulation.References
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3380
Changelog
@metamask/transaction-controllermaliciousand the previous simulation native balance change is different then the previous simulation.changeInSimulationDataproperty tosimulationDatain order to detect change of simulation data.Checklist