Fix EIP-55 support on confirmation screens #6133
Conversation
| @@ -38,11 +38,17 @@ export default class SenderToRecipient extends PureComponent { | |||
| recipientAddressCopied: false, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Are any of the changes in this file required for the fix? Or is this just a refactor?
There was a problem hiding this comment.
The two separate commits here are independent, yup. The changes to this file (the first commit) aren't needed per se, but do improve the component. The 2nd commit with the fix doesn't touch this file, no.
| ? checksumAddress(this.props.recipientAddress) | ||
| : this.props.recipientAddress | ||
|
|
||
| senderAddress = checksumAddress(this.props.senderAddress) |
There was a problem hiding this comment.
Using a class property for this would be a new pattern for us. Do you see any benefit to this over just applying the checksumAddress call in the parent?
There was a problem hiding this comment.
The benefit would be that we don't currently have a way for a component to document/verify/express that it requires a pre-checksummed addressed (e.g. a custom prop type validator). It makes sense to me that we would checksum the address for display purposes.
That said, we could cache this result in a bunch of other ways—would keeping it in the component state be more conventional?
There was a problem hiding this comment.
Depends on the convention 😛
I like, and have recently been trying to follow, the convention of not manipulating data to be rendered in components. So the data the component receives is what it renders. In this case, this would require checksumming the address in /home/danjm/kyokan/metamask-extension/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.container.js
Why do we need to cache the result?
There was a problem hiding this comment.
Why do we need to cache the result?
If we checksum the address at all it's better to that once?
whymarrh
force-pushed
the
fix-eip-55-support
branch
from
f2a59dc to
eb47036
Compare
|
@danjm dropped de0ea5812 from this PR, we can address that separately (no pun intended) |
I'm seeing |
This comment has been minimized.
This comment has been minimized.
Fixes #3514
This PR updates our
ConfirmTransactionBasecontainer to checksumtoaddresses before truncating them for display purposes.