-
Notifications
You must be signed in to change notification settings - Fork 5k
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
feat: Add new completion_time_onchain property to Transaction Finalized events #29474
base: main
Are you sure you want to change the base?
Conversation
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. |
completion_time_onchain
property to Transaction Finalized events
completion_time_onchain
property to Transaction Finalized events673ca86
to
af59f40
Compare
Builds ready [af59f40]
Page Load Metrics (1837 ± 182 ms)
|
const DECIMAL_DIGITS = 2; | ||
|
||
return ( | ||
Math.round( |
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, could we use some extra variables here to make this logic more readable?
submittedTime: number, | ||
blockTimestamp: string, | ||
): string { | ||
const DECIMAL_DIGITS = 2; |
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, constants at top of file with extra context?
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 this is only really going to be used in this function, I find it better to keep it as local constant, which makes it easier to understand its context too.
|
||
if (submittedTime) { | ||
extraParams.completion_time = getTransactionCompletionTime(submittedTime); | ||
} | ||
|
||
if (submittedTime && blockTimestamp) { |
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, but is it worth a unit test given the amount of logic in this file?
c372981
to
03f5006
Compare
Description
The new property
completion_time_onchain
has the number of seconds (rounded to the hundredths) between submitted time and the block timestamp.This number is necessarily lower than the previously existing
completion_time
property, which captures the time between block validation and UI finalization and depends on the polling interval.Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3503
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist