Remove hexdata field from token send #12565
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. |
|
I have read the CLA Document and I hereby sign the CLA |
| } = this.props; | ||
|
|
||
| let gasError; | ||
| if (gasIsExcessive) gasError = GAS_PRICE_EXCESSIVE_ERROR_KEY; | ||
| else if (noGasPrice) gasError = GAS_PRICE_FETCH_FAILURE_ERROR_KEY; | ||
| else if (getIsBalanceInsufficient) | ||
| gasError = INSUFFICIENT_FUNDS_FOR_GAS_ERROR_KEY; | ||
| const showHexData = | ||
| this.props.showHexData && | ||
| asset.type !== ASSET_TYPES.NATIVE && |
There was a problem hiding this comment.
Why don't we want to showHexData is the asset type is NATIVE?
There was a problem hiding this comment.
When I added in hex data for an Ether send it no longer showed the transaction as a transfer on etherscan. I'm also not sure why we would want to allow hex data for a NATIVE token send, open to hearing your thoughts :)
There was a problem hiding this comment.
But... this would disable custom hex data for all transactions 😅 There are only two types - native and token.
The sole purpose of this field is to add additional data to simple send transactions. Sometimes people want to attach messages to their transfers, and sometimes they just want a way to embed a message on the blockchain. Maybe there are other reasons, I'm not sure.
Either way, I don't see why we'd care how Etherscan displays such transactions.
There was a problem hiding this comment.
Okay, yeah, you can just hide hex data if asset.type !== ASSET_TYPES.TOKEN but continue to show it for ether sends.
In addition to the users that @Gudahtt mentioned, this is also how people can manually make calls to smart contract methods.
There was a problem hiding this comment.
@Gudahtt That's true, I suppose hex data wouldn't matter for a native token send, I hadn't realized we were not wanting to enable hex data for ERC-20 was because the hex data would potentially mess up the transfer invocation 😅. A transfer can be successful with hex data if the ERC20 has extra functions with an extra field on transfer but I guess that would be too much of a pain to deal with. Update coming in a few!
There was a problem hiding this comment.
I hadn't realized we were not wanting to enable hex data for ERC-20 was because the hex data would potentially mess up the transfer invocation
To clarify that a little bit, it's not so much that it would mess up the transfer invocation. But rather that right now we overwrite the custom hex data entered by the user with the transfer invocation. So it's misleading to include the field when it just gets overwritten.
If there's a demand, we can definitely consider allowing users to add additional fields as you're suggesting here, as a later feature. That seems like a reasonable request.
There was a problem hiding this comment.
To clarify that a little bit, it's not so much that it would mess up the transfer invocation. But rather that right now we overwrite the custom hex data entered by the user with the transfer invocation. So it's misleading to include the filed when it just gets overwritten.
Good to know, thanks!
Builds ready [4806c7b]
Page Load Metrics (593 ± 81 ms)
highlights: |
Fixes: #10194
Explanation: Added
assetas another piece of state mapped to props for theSendContentcomponent. Then usedassetto implement logic to not render theSendHexDataRowcomponent for assets of typeTOKEN(ERC-20).Manual testing steps: