chore(wallet)_: remove unused fields from the token type #6375
Conversation
saledjenic
requested review from
alaibe,
shivekkhurana,
vkjr,
dlipicar and
Cuteivist
Jenkins BuildsClick to see older builds (46)
|
services/wallet/reader.go
Outdated
| BalancesPerChain: balancesPerChain, | ||
| } | ||
| for symbol, tokens := range getTokenBySymbols(allTokens) { | ||
| balancesPerChain := r.createBalancePerChainPerSymbol(address, balances, tokens, cachedTokens, connectedPerChain, dayAgoTimestamp) |
There was a problem hiding this comment.
the goal behind splitting verified and unverified tokens was so that, for example, balances of some random community token with symbol DAI would not be grouped together with (or even overwrite) the balances for the real DAI. We mark tokens coming from a token list as "verified" and expect symbols to be unique among verified tokens. It's a workaround since we're using the symbols as a key to identify a token across multiple chains, we cannot remove the workaround until we identify our tokens somehow else.
There was a problem hiding this comment.
Not sure if some change was done in the meantime to account for that, making the workaround unnecessary. Please test the condition I speak about (i.e. create a community token with symbol DAI and check that the balances are not mixed up)
There was a problem hiding this comment.
we should probably have a unit test covering that scenario
There was a problem hiding this comment.
It's a workaround since we're using the symbols as a key to identify a token across multiple chains
Yes, that was before (that's even now), but this PR, as well as the base PR of this one, are just small steps towards the final idea where we will make tokens unique by chain + address combination cause that's the only unique thing.
We mark tokens coming from a token list as "verified" and expect symbols to be unique among verified tokens.
I wonder how we handle (in the current code, from develop) if there is more than a single community and each has the same symbol for their token? I guess in that case with this approach, even Verified flag doesn't help, no?
I don't mind reverting changes for the Verified flag, keeping the reset, and getting back to it in one of the follow-up PRs, sound good?
There was a problem hiding this comment.
until we identify our tokens somehow else
Can't we say that tokens with CommunityData != nil are actually custom tokens, not verified?
saledjenic
force-pushed
the
feat/tokens-list
branch
from
fe83473 to
820c95a
Compare
saledjenic
force-pushed
the
chore/remove-unused-fields-from-token
branch
from
0f22d58 to
f472a7a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/tokens-list #6375 +/- ##
====================================================
+ Coverage 61.00% 61.05% +0.05%
====================================================
Files 873 873
Lines 112760 112601 -159
====================================================
- Hits 68787 68747 -40
+ Misses 35964 35871 -93
+ Partials 8009 7983 -26
Flags with carried forward coverage won't be shown. Click here to find out more.
|
saledjenic
force-pushed
the
chore/remove-unused-fields-from-token
branch
from
f472a7a to
4b44c2a
Compare
saledjenic
force-pushed
the
feat/tokens-list
branch
from
8f365ba to
bb06216
Compare
saledjenic
force-pushed
the
chore/remove-unused-fields-from-token
branch
from
4b44c2a to
8c6cdad
Compare
saledjenic
force-pushed
the
feat/tokens-list
branch
from
bb06216 to
d229265
Compare
saledjenic
force-pushed
the
chore/remove-unused-fields-from-token
branch
3 times, most recently
from
b1509b8 to
b053191
Compare
|
I've just reverted the |
Removed fields: - `TokenListID`
…onding field in the response Before this change updatedAt was pointing to the when the last list from the stores was updated. That's sorted out. `LastUpdateTimestamp` field added to the `List` type. A client needs to display the time of the last update.
saledjenic
force-pushed
the
chore/remove-unused-fields-from-token
branch
from
b053191 to
bde82ae
Compare
Before this change updatedAt was pointing to the when the last list from the stores was updated. That's sorted out.
LastUpdateTimestampfield added to theListtype. A client needs to display the time of the last update.