metamask-extension
metamask-extension copied to clipboard
Fix Personal Sign Display
Explanation
This PR makes a change to the personal_sign page. Currently there is an icon that is rendered based on the address passed, but the address passed is null
. This PR changes the address passed to use the same value that is used to select the account (fromAccount
).
More Information
Related to https://github.com/MetaMask/metamask-extension/pull/14438#issuecomment-1204225916
Screenshots/Screencaps
Before

After

Manual Testing Steps
Go to the test-dapp and hit personal sign
Pre-Merge Checklist
- [ ] PR template is filled out
- [ ] IF this PR fixes a bug, a test that would have caught the bug has been added
- [ ] PR is linked to the appropriate GitHub issue
- [ ] PR has been added to the appropriate release Milestone
+ If there are functional changes:
- [ ] Manual testing complete & passed
- [ ] "Extension QA Board" label has been applied
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.
@georgewrmarshall Not sure what the identicon is supposed to show (it has always showed up blank for me) but with the context it should show the icon/ logo for the dApp requesting the signature. Doesn't make sense to show the account avatar again. We can remove it as long as the icon is displayed in the origin along with the dApp URL.
Also, this soon will be replaced by new designs @holantonela has worked on which solve these issues.
We are having requesterAddress
in three components set from respective containers to render the icon using <Identicon/> , SignatureRequestOriginal ConfirmEncryptionPublicKey, and ConfirmDecryptMessage, but in all three places requesterAddress
is assigned to null
in the respective containers. Based on the above comment by @SayaGT maybe we can remove it from all three places?
cc @danjm and @Gudahtt to know if they have more context for the use of requesterAddress
.
requesterAddress
has always been null
. See the original PR here. At one point it showed the Ethereum logo as a default, that must have broken at some point. @danjm might know what the original intent was, but it was a long time ago, and I'm not sure it matters.
I agree that it doesn't make sense to show the account avatar twice. Leaving this as-is or removing it would make the most sense to me for now. The new designs will certainly solve this for us.
+1 vote for removing
@skgbafa could you remove the renderRequestIcon as per @georgewrmarshall 's request so we can move this forward
I believe we're removing this icon already in this PR: https://github.com/MetaMask/metamask-extension/pull/15776
@bschorchit @georgewrmarshall is this PR still needed then? Does this mean its good to go as is?
@bschorchit @georgewrmarshall is this PR still needed then? Does this mean its good to go as is?
I don't think this PR is needed. Let's prioritize https://github.com/MetaMask/metamask-extension/pull/15776 and the issue that motivated this PR won't exist anymore 🙂
Closing in favor of #15776