-
-
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?
Changes from 5 commits
1fc3f8b
59e7d49
b00c588
306b99a
3fec6ed
0571e3c
75f7376
e0aafca
379a83d
4229621
d662640
ef7bb24
aff979d
d2e2794
6262575
c7cd81e
5dc3f8a
c30f1ac
07fafdd
3c1aabe
dee63cc
8518e61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,6 +84,7 @@ import type { | |
SubmitHistoryEntry, | ||
} from './types'; | ||
import { | ||
BlockaidResultType, | ||
TransactionEnvelopeType, | ||
TransactionType, | ||
TransactionStatus, | ||
|
@@ -3599,9 +3600,42 @@ export class TransactionController extends BaseController< | |
); | ||
} | ||
|
||
if (this.#checkIfSimulationRetriggerNeeded(transactionMeta)) { | ||
this.#updateSimulationData(transactionMeta).catch((error) => { | ||
log('Error updating simulation data', error); | ||
throw error; | ||
}); | ||
} | ||
|
||
return transactionMeta; | ||
} | ||
|
||
#checkIfSimulationRetriggerNeeded(transactionMeta: TransactionMeta) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't this keep firing unless we also pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good spot Matthew, this is now fixed, comparing before and after |
||
const { | ||
securityAlertResponse, | ||
simulationData, | ||
txParams: { value }, | ||
} = transactionMeta; | ||
|
||
const isSimulationDataAvailable = Boolean(simulationData); | ||
const isMaliciousTransfer = | ||
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 commentThe 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 commentThe 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 |
||
simulationNativeBalanceDifference === value; | ||
|
||
if ( | ||
!isSimulationDataAvailable || | ||
!isMaliciousTransfer || | ||
!isBalanceDifferenceEqual | ||
) { | ||
return false; | ||
} | ||
|
||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor, can we simplify to a single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} | ||
|
||
#checkIfTransactionParamsUpdated(newTransactionMeta: TransactionMeta) { | ||
const { id: transactionId, txParams: newParams } = newTransactionMeta; | ||
|
||
|
@@ -3649,7 +3683,12 @@ export class TransactionController extends BaseController< | |
transactionMeta: TransactionMeta, | ||
{ traceContext }: { traceContext?: TraceContext } = {}, | ||
) { | ||
const { id: transactionId, chainId, txParams } = transactionMeta; | ||
const { | ||
id: transactionId, | ||
chainId, | ||
txParams, | ||
simulationData: prevSimulationData, | ||
} = transactionMeta; | ||
const { from, to, value, data } = txParams; | ||
|
||
let simulationData: SimulationData = { | ||
|
@@ -3679,6 +3718,13 @@ export class TransactionController extends BaseController< | |
data: data as Hex, | ||
}), | ||
); | ||
|
||
if (prevSimulationData && !isEqual(simulationData, prevSimulationData)) { | ||
simulationData = { | ||
...simulationData, | ||
changeInSimulationData: true, | ||
}; | ||
} | ||
} | ||
|
||
const finalTransactionMeta = this.getTransaction(transactionId); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1119,9 +1119,9 @@ export type TransactionError = { | |
export type SecurityAlertResponse = { | ||
reason: string; | ||
features?: string[]; | ||
// TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||
// This is API specific hence naming convention is not followed. | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
result_type: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
result_type: BlockaidResultType | string; | ||
providerRequestsCount?: Record<string, number>; | ||
}; | ||
|
||
|
@@ -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 commentThe 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 Is this needed at all? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
/** Error data if the simulation failed or the transaction reverted. */ | ||
error?: SimulationError; | ||
|
||
|
@@ -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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
Malicious = 'Malicious', | ||
Warning = 'Warning', | ||
Benign = 'Benign', | ||
Errored = 'Error', | ||
|
||
// MetaMask defined result types | ||
NotApplicable = 'NotApplicable', | ||
Loading = 'loading', | ||
} |
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!