Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/precision optimization #1960

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

n13
Copy link
Member

@n13 n13 commented Jun 22, 2024

Description

Problem: Updating precision for all tokens is very expensive

Fix: Don't load precision from stats table - load it dynamically whenever the token balance is loaded

Why this works: We only need token precision for display and importantly for "send" - prior to display and send we must update the balances; therefore this will not cause any problems.

Tech Notes

  • Made precision a dynamic variable in TokenModel that can either be set - as with our static tokens - or it can be null in which case it looks up the precision from the static table.

In theory token balances are always loaded before we display, and certainly before we send. Also precision never dynamically changes so once we load it, it never changes - ever. That's guaranteed by the token contract in EOSIO.

Testing
Tested "send" with a few different tokens, it works.

Also monitored that as token balances are received, we update the precision.

Future Improvements
It would be nice to remove the tokens table and to package the side effect code a bit better.

I didn't remove the static table because I wasn't sure we have a comprehensive list of "all tokens" anymore since the token master contract change, so I don't know if all known tokens are always there. I believe they probably are - but not sure.

@chuck-h
Copy link
Collaborator

chuck-h commented Jun 23, 2024

Good cleanup on some mess I left behind. Probably would benefit from a deeper clean too.

BTW I think (a) we want getTokenBalance to be called when a currency card slides into view; and (b) we don't want to go out and get all the balances at startup. Neither of these seems to be true.

When getTokenBalance is called, we either
(1) get a balance back from the chain api, which tells us the precision, so we update the TokenModel.contractPrecisions table, or
(2) get "no balance entry" back from the chain api; whereupon we should look in the contractPrecisions table and
(2a) we find the token precision is already listed there -- we're done, or
(2b) the token precision is not in the contractPrecisions table, we need to get it from a Stat table lookup.

I think this PR handles (1) perfectly but not (2a)/2(b).

I expect that with the upcoming new logic for manually enabling/disabling currency card visibility, a user can enable a card for a token he doesn't own, then click "receive" to generate a "please send me 10 tokens" QR code. That operation will require a Stat table lookup in order to create a valid transaction for the QR.

@chuck-h
Copy link
Collaborator

chuck-h commented Jun 23, 2024

TL;DR The whole TokenModel structure deserves a review since it became more dynamic.

@n13 I think you were asking why we should keep a separate contractPrecisions static table instead of just holding that precision directly as a field in the TokenModel. My answer is "no good reason".

There already is a precision field in the TokenModel, which IIRC was being used as a hint to the UI display formatter. I believe that UI hinting function is probably worth keeping so the TokenModel needs two distinct precision fields.

Before I added the ability to download the token list from the tokensmaster contract, TokenModel was just a simple static list of the tokens. So I grafted on the dynamic loading to that. But I really did not address the lifecycle issues of new tokens appearing in the master list or old ones disappearing (e.g. being removed for bad behavior). I wrote a couple of functions that look related but they're dead code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants