-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
fix: Add simulation retrigger #4792
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since string
encompasses all the BlockaidResultType
options, should we just leave it as string
to minimise coupling to Blockaid?
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.
Done
return transactionMeta; | ||
} | ||
|
||
#checkIfSimulationRetriggerNeeded(transactionMeta: 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we definitely looking for an exact comparison rather than a percentage difference?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, can we simplify to a single return
since it's a 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.
Done
@@ -3599,9 +3600,42 @@ export class TransactionController extends BaseController< | |||
); | |||
} | |||
|
|||
if (this.#checkIfSimulationRetriggerNeeded(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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than defining a full enum, could we just define a local constant such as BLOCKAID_RESULT_TYPE_MALICIOUS
?
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.
Done
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
const percentageDifference = difference.dividedBy(average).multipliedBy(100); | ||
|
||
return percentageDifference.isLessThanOrEqualTo(threshold); | ||
} |
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.
Weill be nice to add test coverage for this util.
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 call, added unit tests for this util now
07fafdd
@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.
|
Explanation
This PR adds a mechanism to re-trigger of simulations if the security provider mark transaction as
malicious
and 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-controller
malicious
and the previous simulation native balance change is different then the previous simulation.changeInSimulationData
property tosimulationData
in order to detect change of simulation data.Checklist