fix: Display ERC20 token metadata correctly for recently fetched tokens #7063
Conversation
Jenkins BuildsClick to see older builds (24)
|
vkjr
force-pushed
the
fix/erc20-token-displaying
branch
from
a93fbf7 to
0ed2f22
Compare
vkjr
changed the title
vkjr
requested review from
alaibe,
dlipicar,
igor-sirotin and
saledjenic
and removed request for
dlipicar
| } | ||
|
|
||
| // Request async token discovery for ERC20 transfers to make sure we have their metadata | ||
| if deps.tokenPublisher != nil { |
There was a problem hiding this comment.
hmmm, the way we normally do it, modules publish events in their "domain" (alchemy knows deals with transfers, so it publishes events like "hey I've just fetched these transfers".
It is the subscriber's responsibility to interpret those events and react in accordance (that is, tokenManager should subscribe to "here's some new transfers" signal and know it needs to potentially fetch metadata for some new token). The benefit of this becomes clearer when there's multiple subscribers for a given publisher, it doesn't make sense for the publisher to have knowledge of every subscriber.
There was a problem hiding this comment.
also, requesting the token metadata every time the client fetches activity entries seems like a bit of an overkill. Can't we detect tokens for which for some reason the metadata is not available and only request those? You can do that request directly (ie call some method from an interface) instead of using the pubsub mechanism for that.
There was a problem hiding this comment.
@dlipicar, thanks for clarification, that of course makes much more sense 👍
Yes, I also thought about checking if metadata exists but didn't want to introduce relation to TokenManager and use only publisher, but I will change this
| } | ||
|
|
||
| // Request async token discovery for ERC20 transfers to make sure we have their metadata | ||
| if m.tokenPublisher != nil { |
There was a problem hiding this comment.
the first part of the previous comment belongs here, we probably want to publish something like a "transfersFetched" event here, which potentially many other modules will want to subscribe to (to refresh balances, collectibles ownership, etc)
There was a problem hiding this comment.
@dlipicar,
Fixed pub/sub pattern. Events now describe "what happened" (ERC20ActivityFetched) not "what to do" (TokenDiscoveryRequest).
Publisher moved from TokenManager to ActivityFetcher abstraction level.
Alchemy emits events -> ActivityFetcher publishes -> TokenManager subscribes and reacts.
|
|
||
| if lastFetchedBlock == nil { | ||
| fromBlock := gethrpc.EarliestBlockNumber | ||
| fromBlock := gethrpc.BlockNumber(0) |
There was a problem hiding this comment.
I still have my doubts about this :D
#7035 (comment)
There was a problem hiding this comment.
@dlipicar, wondering if we are referring to the same gethrpc)
My lsp server jumps to this:
vkjr
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7063 +/- ##
===========================================
- Coverage 60.05% 59.98% -0.08%
===========================================
Files 808 808
Lines 114203 114277 +74
===========================================
- Hits 68582 68544 -38
- Misses 38651 38727 +76
- Partials 6970 7006 +36
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@vkjr have to ask you if not a problem to postpone merging of this PR until this one gets merged #6912 ? Even changes you did here are not too complex I see that some code has been removed in my PR and there will be conflicts for sure and I would like to avoid them. My giant PR brings a lot of changes, basically it changes a fundamental approach on how we deal with tokens/token lists and basically affects many parts of the app. It's planned to have it merged by the end of the month. Until then, I will have a hard time keeping it in shape (it's not statusgo rebase only, but the desktop pr also). Having all that in mind and since this change is not that urgent, it would be really nice to postpone it. In the meantime, if you want, you can rebase it on top of my work. |
4 participants
Description
This PR fixes an issue where ERC20 tokens discovered in wallet activity transactions were not having their metadata fetched, causing incorrect values to be displayed
Notes for reviewers
When fetching wallet activity from Alchemy, ERC20 token transfers would be detected, but if the token metadata wasn't already known to the system, the token would display without info (no symbol, name, or decimals).
In this PR:
Closes #7036