-
Notifications
You must be signed in to change notification settings - Fork 5.2k
feat: add info alert linked to account type #31840
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
Conversation
fece23e
to
bb47510
Compare
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
bb47510
to
28102bc
Compare
✨ Files requiring CODEOWNER review ✨✅ @MetaMask/confirmations
|
8b153cd
to
0c3ccb5
Compare
Builds ready [0c3ccb5]
UI Startup Metrics (1213 ± 67 ms)
|
Builds ready [b6f4539]
UI Startup Metrics (1223 ± 62 ms)
Benchmark value 199 exceeds gate value 195 for firefox browserify home p95 domInteractive Sum of mean exceeds: 4ms | Sum of p95 exceeds: 4ms Sum of all benchmark exceeds: 8ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
b6f4539
to
f3f6cfd
Compare
Builds ready [f3f6cfd]
UI Startup Metrics (1192 ± 64 ms)
Benchmark value 111 exceeds gate value 110 for firefox browserify home mean domInteractive Benchmark value 198 exceeds gate value 195 for firefox browserify home p95 domInteractive Sum of mean exceeds: 1ms | Sum of p95 exceeds: 8ms Sum of all benchmark exceeds: 9ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
The changes look good, but it conflict with my PR: #32034
nestedTransactions: NestedTransactionMetadata[] | undefined, | ||
): boolean { | ||
return Boolean(nestedTransactions?.length); | ||
} |
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.
Is it possible to use the check: transaction.type === TransactionType.Batch
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.
That was my first approach, but in a previous PR, Matthew said that we may have other batch transaction types in future, so safer to do it using the length of the nestedTransactions
.
|
||
const isBatch = | ||
Boolean(nestedTransactions?.length) && | ||
to?.toLowerCase() === from.toLowerCase(); |
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.
Utility fn can be used here
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.
Pushed the utility function here
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.
link led to merge commit. isBatchTransaction added in the commit before 7078eb1
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.
Lgtm!
Builds ready [a5433db]
UI Startup Metrics (1202 ± 68 ms)
Benchmark value 2227 exceeds gate value 2192 for chrome webpack home mean uiStartup Benchmark value 1735 exceeds gate value 1711 for chrome webpack home mean load Benchmark value 1728 exceeds gate value 1704 for chrome webpack home mean domContentLoaded Benchmark value 1722 exceeds gate value 1699 for chrome webpack home mean loadScripts Benchmark value 2573 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 112 exceeds gate value 110 for firefox browserify home mean domInteractive Benchmark value 1496 exceeds gate value 1495 for firefox browserify home p95 load Benchmark value 1480 exceeds gate value 1475 for firefox browserify home p95 loadScripts Sum of mean exceeds: 108ms | Sum of p95 exceeds: 125ms Sum of all benchmark exceeds: 233ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [6e39fb1]
UI Startup Metrics (1181 ± 58 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
This PR aims to:
Type
toAccount Type
for batch transactions.Account
row for batch transactions.Interacting with
fromTransactionDetails
and show inTransactionAccountDetails
for batch transactions.Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4660
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist