Temp TokenHero hook

[OGPoyraz]

Description

Related issues

Fixes:

Manual testing steps

1. Go to this page...

Screenshots/Recordings

Before

After

ERC 20 transfer

ERC20.transfer.mp4

ERC 721 transfer

ERC721.transfer.mp4

Native transfer

native.transfer.mp4

Pre-merge author checklist

• I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
• I've completed the PR template to the best of my ability
• I’ve included tests if applicable
• I’ve documented my code using JSDoc format if applicable
• I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

• I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
• I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

[digiwand]

I think transferData?.args[1] should equate to transactionData?.args?._value

[digiwand]

rather than using tokenDetails to fetch decimals, I think fetchErc20Decimals is a nice option since it will return the default of 18 decimals if not found. This keeps the type consistent and avoids decimals ?? 18 below.

tokenDetails may only be used for nft logic

[OGPoyraz]

It's a bit of tradeoff I would say - to use TokenListController or make component to fetch the details itself. In ideal world we should use the same token state which is from TokenListController so that we don't have to make all components to fetch their own details.

[digiwand]

I agree it would be better to resolve this issue with a single source of truth.

I'll look into this more. Planning to examine if AssetsContractController is a candidate as well

[digiwand]

hi @OGPoyraz, thanks for creating this logic! It's helpful

From my QA, the components and hooks I created the last couple of days mostly resulted in the same display values as this PR while also supporting staking.

There was a bug supporting simpleSend network image and fiat values that I was able to fix in my PR after analyzing this logic.

I am leaning towards keeping NFT logic separate from ERC20 logic to apply the separation of concerns.

The extension code has a more simplified display value aggregation, similar to the PR I have open. It also utilizes SendHeading and NFTSendHeading to separate logic, which I think also makes sense in mobile. In our case, we can have something like TokenHero/HeroToken and HeroTokenNFT.

[digiwand]

somewhere there is a bug here.

#15259 appears to cover this

[digiwand]

merging fiat logic

[OGPoyraz]

@digiwand , it's totally fine if we want to have separate 3 components like NativeInfo, ERC20Info, NFTInfo (Maybe even ERC721 ERC1155 UnknownTokenInfo) and have those components under token-hero folder, not outside like we did it in the extension. Because those components tightly coupled to token-hero context.

Also if we want to make this separation. It should be very clear and easy to follow in terms of logic. Let's collect all relevant information under their own hook for those 3 component.

It's reasonable to use utilities like fetchErc20Decimals from some other files but if we want to do that please move them into utilities if we need to. For example we shouldn't be reusing useDisplayName hook, it may give us what info we need but there could be more information collected in petnames context it may be irrelevant to make it calculate everything.

[digiwand]

@OGPoyraz

separate 3 components like NativeInfo, ERC20Info, NFTInfo (Maybe even ERC721 ERC1155 UnknownTokenInfo) and have those components under token-hero folder, not outside like we did it in the extension.

I will consider this. I'm a fan of separating the logic by transaction type like you've suggested with the hook! It would be beneficial in that it would help us understand the code more easily and aid debugging the logic in the future. Following this, I think we'll want to update the logic in the extension as well

It's reasonable to use utilities like fetchErc20Decimals from some other files but if we want to do that please move them into utilities if we need to.
👍🏼

thanks for the reminder with useDisplayHook too! I was planning to move this as well when we get closer to finalizing the logic in case we go with different logic