-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: use correct property name for redesign #24579
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. |
Builds ready [141a1fe]
Page Load Metrics (1083 ± 539 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #24579 +/- ##
========================================
Coverage 67.37% 67.37%
========================================
Files 1289 1289
Lines 50219 50219
Branches 13005 13005
========================================
Hits 33834 33834
Misses 16385 16385 ☔ View full report in Codecov by Sentry. |
@@ -60,6 +60,8 @@ const ETH_METHOD_TO_APPROVAL_TYPE = { | |||
[EthMethod.PersonalSign]: ApprovalType.PersonalSign, | |||
[EthMethod.Sign]: ApprovalType.Sign, | |||
[EthMethod.SignTransaction]: ApprovalType.SignTransaction, | |||
// This method does not seem to be available in the EthMethod but supported by the test dapp | |||
eth_signTypedData: ApprovalType.EthSignTypedData, |
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 know it's not introduced your PR but can we use MESSAGE_TYPE
from shared/constants/app.ts
and remove EthMethod
from this file?
In order to do that we must add eth_signTypedData_v1
into MESSAGE_TYPE
as well which is totally fine.
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.
Happy to change it but I am not sure behind what the rationale was in adding it in the first place. @digiwand maybe you can clarify before I change it?
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.
Also the MESSAGE_TYPE misses eth_signTransaction and eth_signTypedData_v1
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.
My reasoning was just to remove a dependency to a package since we have these constants in app. And yes as I mentioned in the previous message it's totally fine to add that constant.
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.
makes sense, I will change it
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 @OGPoyraz
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.
Nice! I was confused with what this type in this middleware represented.
I assumed ETH_METHODS since these are coming from the JSON_RPC request and it seems I assumed incorrectly. Thanks for the catch and the fix! cc: @pnarayanaswamy @OGPoyraz
side note, though related: I have in my notes to understand more the differences and use-cases for our various transaction-related types. If anyone has more intel into any specifics of these, please share
Eth Methods - EthMethod
Signatures - SIGNING_METHODS
Transactions - TransactionType
Approvals - ApprovalType
Confirmations - CONFIRMATION_METHODS
Message - MESSAGE_TYPE
Builds ready [31eb4ce]
Page Load Metrics (730 ± 490 ms)
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.
Lgtm!
Description
Fixes the bug where the ui_customization property does not show up in the metrics event.
Also fixed the mapping such that the ui_customization property shows up for the eth_signTypedData as well.
Related issues
Fixes: #24578
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist