LL 7340 & LL-6671 - Sync ERC721 & ERC1155 NFT

[ghost]

Context (issues, jira)

LL-6671
LL-7340

Context (issues, jira)

This PR is a proposal for the implementation of NFTs and synchronization of ERC721 & ERC1155 tokens in live-common.

This PR is done using the staging server for the /transactions call and a mock for the NFTs' metadata

Source for the metadata mock: https://github.com/klambert-ledger/mock-nft-metadata-api

Description / Usage

Account should now have a NFT property filled with owned NFTs:

Expectations

• Test coverage: The changes of this PR are covered by test. Unit test were added with mocks when depending on a backend/device.
• No impact: The changes of this PR have ZERO impact on the userland. Meaning, we can use these changes without modifying LLD/LLM at all. It will be a "noop" and the maintainers will be able to bump it without changing anything.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ledgerhq/ledger-live-common/GZUKt9rdKr8BAoW5DAFBtMMWvveT
✅ Preview: https://ledger-live-common-git-ll-7340-sync-erc1155-nft-ledgerhq.vercel.app

[ghost]

This PR is a proposal, and it needs some insights for the architecture and need some help to have a more coherent testing since right now I'm only comparing our results with what OpenSea's API is responding (which doesn't contain the amount of ERC 1155 for example).

[LFBarreto]

Some stuff you could do to improve, it looks fine by me just different from what should be done i think 👍

[juan-cortes]

My understanding was that the review wasn't so much about code specifics like but more about the architectural approach and how it fits in the schema of our codebase. NFTs are by definition a token and if we follow the convention we have in live-common they actually would fit in the TokenAccount definition, making them be inside the parent Account.

Keeping them as TokenAccount allows us to keep track of the movements we had for them. The only peculiarity is that the ERC-721 ones would only have a balance of 1 and there would only be the incoming transaction, but we could actually have the outgoing one if we wanted to track the tokens we once had. It's also compatible with the tokenId structure we keep where ethereum/erc721/tokenIdentifier

We would need to build a list of TokenCurrency on sync dynamically, as opposed to based on a hardcoded list like we do for ERC-20. This dynamic list would come from the operations for the user and would be used to know what they can send, I don't think we need to support a list of what we can receive since that's an immense amount of different tokens.

The TokenCurrency type already was thought of with the concept of a tokenType in mind, we currently use it for erc20, trc10 and trc20 from what I see in the code. If we need some extra fields they should added to this type instead of creating a whole new monster.

This fits 100% the UI we saw on figma, and it would require minimal code changes (asset distribution would need to filter by tokenType to not show NFTs on the table for example) but I might be missing something.

---

As for how to handle the sync and the sending we already talked about it but I think it should be a module inside the families/ethereum/modules folder, the same way we do for erc20 and compound with any peculiarities we might have for them handled there.

[IAmMorrow]

as far as I remember @gre was opposed to the idea of reusing "TokenAccount" which are used everywhere in the app (and it would break everywhere).

[juan-cortes]

I actually initially wrote having another type of account, NonFungibleTokenAccount or whatever, that can residen alongside the other types of subaccounts, but I guess I changed my mind midway.

If TokenAccount breaks things, then for sure, another type. Just not fields on Account 🤷

[gre]

We would need to build a list of TokenCurrency on sync dynamically, as opposed to based on a hardcoded list like we do for ERC-20. This dynamic list would come from the operations for the user and would be used to know what they can send, I don't think we need to support a list of what we can receive since that's an immense amount of different tokens.

I would prefer not to touch Currency type which is not designed to be dynamic today. indeed it's going to happen in future (dynamic tokens) but i'm not sure we should put this idea in the NFT implementation yet. Lot of parts works in this "load at boot" mode and are not ready for this dynamically.

Also, I feel "Currency" type don't match a lot the idea of an NFT. it is not a currency (look at all the fields of currency, not a lot of them makes sense, like what's the ticker of a NFT? are you going to try to get a countervalues from countervalues api? because with our current logic it's going to try this if you make it such an account =D anyway, just to say that I believe it will be more work to hack that way rather than just making a new type for "NFTs" ).

[gre]

For me, the general architecture is good to go