Replaced addresses by the address component on SignTypedData v4 signatures #16018
Conversation
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
|
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. |
VSaric
force-pushed
the
replace-addresses-by-the-address-component
branch
from
4cf7674 to
ad47843
Compare
Builds ready [f1be052]
Page Load Metrics (2347 ± 102 ms)
highlights:storybook
|
VSaric
force-pushed
the
replace-addresses-by-the-address-component
branch
from
f1be052 to
9fdef79
Compare
Builds ready [9fdef79]
Page Load Metrics (2373 ± 204 ms)
highlights:storybook
|
VSaric
force-pushed
the
replace-addresses-by-the-address-component
branch
from
9fdef79 to
f84cdc2
Compare
Builds ready [f84cdc2]
Page Load Metrics (2285 ± 71 ms)
highlights:storybook
|
Builds ready [ecf9f25]
Page Load Metrics (2488 ± 130 ms)
highlights:storybook
|
|
Verified by QA |
VSaric
requested review from
NiranjanaBinoy,
georgewrmarshall,
brad-decker and
danjm
brad-decker
suggested changes
Oct 6, 2022
|
|
||
| const renderNode = (renderData) => { | ||
| return ( | ||
| <div className="signature-request-message--node"> |
There was a problem hiding this comment.
So, a few things.
- This isn't proper BEM syntax for class names
--is a modifier spacer and__is a block spacer. so i think we'd wantsignature-request-message__node. - Later you have
--node-labeland use&-labelto style it, this also isn't proper BEM syntax, node-label would be its own new block not a nested block. - I realize you are trying to use the existing styles here, but I would prefer that if you are going to remove old patterns (thank you!!! this is awesome!!!) by refactoring class components to functional, dropping the '.component' syntax that we also use
BoxandTypographyto ease in the transition to the new design system in the future.
let me know if that makes sense
There was a problem hiding this comment.
You are right @brad-decker, sorry for that! I know that we are using Box and Typography by refactoring class components to functional and I know about proper BEM syntax, sorry for that once again! I will take care of this in the future PRs!
I left only one div because it has the ref property. I have not been able to do this with the Box component, because Box component don't have that property. If it is okay to stay like this great, but if you have some suggestions about this please comment and I will make changes. I made changes here.
Thank you in advance!
VSaric
force-pushed
the
replace-addresses-by-the-address-component
branch
from
ecf9f25 to
d327788
Compare
Builds ready [820c8d0]
Page Load Metrics (2407 ± 105 ms)
highlights:storybook
|
VSaric
force-pushed
the
replace-addresses-by-the-address-component
branch
from
820c8d0 to
3c55ea2
Compare
|
I added your suggestion changes manually here @georgewrmarshall. Thank you! 😄 |
Builds ready [3c55ea2]
Page Load Metrics (2260 ± 85 ms)
highlights:storybook
|
georgewrmarshall
previously approved these changes
Nov 7, 2022
brad-decker
suggested changes
Nov 7, 2022
Comment on lines
49
to
105
| const renderNode = (renderData) => { | ||
| return ( | ||
| <Box className="signature-request-message__node"> | ||
| {Object.entries(renderData).map(([label, value], i) => ( | ||
| <Box | ||
| className="signature-request-message__node" | ||
| key={i} | ||
| paddingLeft={2} | ||
| display={ | ||
| typeof value !== 'object' || value === null ? DISPLAY.FLEX : null | ||
| } | ||
| > | ||
| <Typography | ||
| as="span" | ||
| color={COLORS.TEXT_DEFAULT} | ||
| marginLeft={4} | ||
| fontWeight={ | ||
| typeof value === 'object' | ||
| ? FONT_WEIGHT.BOLD | ||
| : FONT_WEIGHT.NORMAL | ||
| } | ||
| > | ||
| {label.charAt(0).toUpperCase() + label.slice(1)}:{' '} | ||
| </Typography> | ||
| {typeof value === 'object' && value !== null ? ( | ||
| renderNode(value) | ||
| ) : ( | ||
| <Typography | ||
| as="span" | ||
| color={COLORS.TEXT_DEFAULT} | ||
| marginLeft={4} | ||
| className="signature-request-message__node__value" | ||
| > | ||
| {isValidHexAddress(value, { | ||
| mixedCaseUseChecksum: true, | ||
| }) ? ( | ||
| <Typography | ||
| variant={TYPOGRAPHY.H7} | ||
| color={COLORS.INFO_DEFAULT} | ||
| className="signature-request-message__node__value__address" | ||
| > | ||
| <Address | ||
| addressOnly | ||
| checksummedRecipientAddress={toChecksumHexAddress(value)} | ||
| recipientName={getAccountName(identities, value)} | ||
| /> | ||
| </Typography> | ||
| ) : ( | ||
| `${value}` | ||
| )} | ||
| </Typography> | ||
| )} | ||
| </Box> | ||
| ))} | ||
| </Box> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
We shouldn't create these functions for intermediary components, just render them inline unless they are rendered multiple times then lift them into a new component.
There was a problem hiding this comment.
@brad-decker Done here.
Created a new issue for this here: #16405 |
…ere hex address is not valid
VSaric
force-pushed
the
replace-addresses-by-the-address-component
branch
from
3efda34 to
a691f66
Compare
Builds ready [2be69d3]
Page Load Metrics (2205 ± 115 ms)
highlights:storybook
|
brad-decker
approved these changes
Nov 9, 2022
georgewrmarshall
approved these changes
Nov 9, 2022
There was a problem hiding this comment.
I completely agree with @brad-decker's suggestion to move the SignatureRequestData component to it's own file and will promote this pattern for maintainability in future review. Following this I think we could have a unit test and story for each new component but because it used in signature-request-message.js and there is a story for this component I think it's ok. Nice work! 💯
Builds ready [8ef44b0]
Page Load Metrics (2060 ± 88 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
highlights:storybook
|
danjm
approved these changes
Nov 10, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Explanation
MetaMask has no control on what the message that is displayed is this screen is - this is something provided by the dapps and will look differently for each one of them. MetaMask does have control on how such messages are displayed and how the UI looks like - and this is what we are trying to achieve with this PR. So with this PR, we want every value that is an address to be displayed using the address component.
More Information
Screenshots
Before (test-dapp)
After (test-dapp)
Screencaps
Before (test-dapp)
Screen.Recording.2022-09-28.at.15.15.24.mov
After (test-dapp)
Screen.Recording.2022-10-04.at.16.59.21.mov
Before (OpenSea)
Screen.Recording.2022-09-29.at.15.03.37.mov
After (OpenSea)
Screen.Recording.2022-10-04.at.17.12.57.mov
Manual Testing Steps
Signor
NFTson https://faucet.paradigm.xyz/Rinkeby test networkNFTsSellConfirm listingon OpenSea - this will trigger aSignTypedData v4prompt in MM (you might have to sign a different message and approve a transaction if it's the first time you're going through this flow)