-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Confirmation page alerts #20125
Merged
Merged
Confirmation page alerts #20125
Changes from 40 commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
952b89e
Adding metrics events for blockaid
jpuri 3c2e7fd
Adding blockaid alert on confirmation pages
jpuri 997c035
Fix
jpuri bba6e59
Update
jpuri bd8e6f9
Update
jpuri e2fcb68
Update
jpuri 6d26d7b
Update
jpuri 353e0e0
Update
jpuri 6e35f9a
Update
jpuri 347d80f
Update
jpuri afef9fd
Adding metrics events for blockaid
jpuri f582b52
Merge
jpuri ba11b40
Merge
jpuri bc29cb7
Adding unit tests
jpuri d268a09
Update
jpuri 5ac22df
Update
jpuri 52d93fd
Merge branch 'develop' into blockaid_metrics
jpuri b6b06bb
Merge branch 'develop' into blockaid_metrics
jpuri 75ae51b
merge
jpuri c09365e
Update
jpuri 8c0d769
Merge branch 'blockaid_metrics' of github.com:MetaMask/metamask-exten…
jpuri cc6314e
Merge branch 'blockaid_metrics' into confirmation_page_alerts
jpuri e8927e6
Merge branch 'develop' into blockaid_metrics
jpuri 76ea113
Update
jpuri a5c7964
Merge branch 'confirmation_page_alerts' of github.com:MetaMask/metama…
jpuri 5cc93ee
Update
jpuri 43d23eb
Merge branch 'develop' into blockaid_metrics
jpuri 439e080
Merge branch 'develop' of github.com:MetaMask/metamask-extension into…
jpuri 9c65250
Update
jpuri 5d5561e
Update
jpuri 98e14dc
Update
jpuri 4a88a9c
Update
jpuri e7befbd
merge
jpuri 521ff7e
Update
jpuri 77d2266
Update
jpuri a673b8f
Update LavaMoat policies
metamaskbot f18118c
Merge branch 'confirmation_page_alerts' of github.com:MetaMask/metama…
jpuri c5205a8
Update
jpuri e499b26
Update
jpuri d8192ee
Update
jpuri 9fd2df1
Merge branch 'develop' into confirmation_page_alerts
jpuri 7f4a05f
Update
jpuri 2ac201c
Merge branch 'confirmation_page_alerts' of github.com:MetaMask/metama…
jpuri 6d6ce95
Merge branch 'develop' of github.com:MetaMask/metamask-extension into…
jpuri d88685e
Update
jpuri File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2652,6 +2652,83 @@ describe('Transaction Controller', function () { | |
); | ||
}); | ||
|
||
it('should call _trackMetaMetricsEvent with the correct payload when blockaid verification fails', async function () { | ||
const txMeta = { | ||
id: 1, | ||
status: TransactionStatus.unapproved, | ||
txParams: { | ||
from: fromAccount.address, | ||
to: '0x1678a085c290ebd122dc42cba69373b5953b831d', | ||
gasPrice: '0x77359400', | ||
gas: '0x7b0d', | ||
nonce: '0x4b', | ||
}, | ||
type: TransactionType.simpleSend, | ||
origin: 'other', | ||
chainId: currentChainId, | ||
time: 1624408066355, | ||
metamaskNetworkId: currentNetworkId, | ||
securityAlertResponse: { | ||
result_type: BlockaidResultType.Failed, | ||
reason: 'some error', | ||
}, | ||
}; | ||
const expectedPayload = { | ||
actionId, | ||
initialEvent: 'Transaction Added', | ||
successEvent: 'Transaction Approved', | ||
failureEvent: 'Transaction Rejected', | ||
uniqueIdentifier: 'transaction-added-1', | ||
persist: true, | ||
category: MetaMetricsEventCategory.Transactions, | ||
properties: { | ||
network: '5', | ||
referrer: 'other', | ||
source: MetaMetricsTransactionEventSource.Dapp, | ||
status: 'unapproved', | ||
transaction_type: TransactionType.simpleSend, | ||
chain_id: '0x5', | ||
eip_1559_version: '0', | ||
gas_edit_attempted: 'none', | ||
gas_edit_type: 'none', | ||
account_type: 'MetaMask', | ||
asset_type: AssetType.native, | ||
token_standard: TokenStandard.none, | ||
device_model: 'N/A', | ||
transaction_speed_up: false, | ||
ui_customizations: ['security_alert_failed'], | ||
security_alert_reason: 'some error', | ||
security_alert_response: BlockaidResultType.Failed, | ||
}, | ||
sensitiveProperties: { | ||
baz: 3.0, | ||
foo: 'bar', | ||
gas_price: '2', | ||
gas_limit: '0x7b0d', | ||
transaction_contract_method: undefined, | ||
transaction_replaced: undefined, | ||
first_seen: 1624408066355, | ||
transaction_envelope_type: TRANSACTION_ENVELOPE_TYPE_NAMES.LEGACY, | ||
}, | ||
}; | ||
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. [nit] I think rather than rebuilding the mock obj for each test, we could reuse the mock data e.g.
|
||
|
||
await txController._trackTransactionMetricsEvent( | ||
txMeta, | ||
TransactionMetaMetricsEvent.added, | ||
actionId, | ||
{ | ||
baz: 3.0, | ||
foo: 'bar', | ||
}, | ||
); | ||
assert.equal(createEventFragmentSpy.callCount, 1); | ||
assert.equal(finalizeEventFragmentSpy.callCount, 0); | ||
assert.deepEqual( | ||
createEventFragmentSpy.getCall(0).args[0], | ||
expectedPayload, | ||
); | ||
}); | ||
|
||
it('should call _trackMetaMetricsEvent with the correct payload (extra params) when flagAsDangerous is malicious', async function () { | ||
const txMeta = { | ||
id: 1, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[nit] could use constant from
shared/constants/security-provider.ts
here. might be helpful to know there are finite values for this propertyThere 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.
For failed request reason is actual exception message.
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.
hm. odd. We typically use this value as a key reference e.g. when we get the description, we use this value, but now we might need to check result_type as well for our description.. It's adding slight complexity, but avoiding any complexity will help avoid build up and keep the code more scalable
@jpuri
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.
transferring knowledge and convo from our DM's to continue here:
BlockaidResultType.Failed
and related reason value are generated on our side@jpuri's proposal:
my (@digiwand's) proposal:
securityAlertResponse.error
. Send error as a separate property to metametricsBlockaidResultType.Failed
happens, use a snake case key value for reason e.g.local_var_failed_request
the reason for my proposal is to have consistency in the values and avoid supporting more complexities as mentioned in the comment above.
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.
alternatively, we could also provide the error message in the description property which is currently used for arbitrary text