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.

Sign up for GitHub

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

Jump to bottom

feat: compute circulating supply and store it in DB #136

Merged
merged 8 commits into from
Aug 11, 2021
Merged

feat: compute circulating supply and store it in DB #136

merged 8 commits into from
Aug 11, 2021

Conversation

telezhnaya
Copy link
Contributor

You can take a look at how it works at 34.141.101.9 (ask @khorolets for access)

tmux attach
cargo run --release -- --home-dir ~/.near/mainnet run --store-genesis --stream-while-syncing --allow-missing-relations-in-first-blocks 1000 --concurrency 0 sync-from-latest

The testing DB is available at 35.233.39.106, there are some numbers calculated.

It works well in general, but I am still thinking about working with threads. I am killing the thread if anything bad happens. On the one hand, that's exactly what I wanted to achieve. On the other, it's better to try to recreate the thread from time to time. Want to know your opinion, I guess the best solution will be born after the discussion.

@telezhnaya
Copy link
Contributor Author

telezhnaya commented Jul 28, 2021 •
edited
Loading

I fixed formatting, cargo fmt is calm both on my machine and at Bohdan's server, but CI formatter is still angry at me.
The language version looks the same, 1.51.0

khorolets
khorolets approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@khorolets khorolets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! A few comments here and there. Overall great effort!

src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/db_adapters/blocks.rs Outdated Show resolved Hide resolved
src/db_adapters/accounts.rs Outdated Show resolved Hide resolved
src/circulating_supply/lockup.rs Outdated Show resolved Hide resolved
src/circulating_supply/lockup.rs Outdated Show resolved Hide resolved
src/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
frol
frol requested changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@frol frol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not finished the full review just yet, but decided to share the preliminary review since it is already not tiny.

migrations/2021-06-15-151515_circulating_supply_table/up.sql Outdated Show resolved Hide resolved
migrations/2021-06-15-151515_circulating_supply_table/up.sql Outdated Show resolved Hide resolved
migrations/2021-06-15-151515_circulating_supply_table/up.sql Outdated Show resolved Hide resolved
migrations/2021-06-15-151515_circulating_supply_table/up.sql Outdated Show resolved Hide resolved
migrations/2021-06-15-151515_circulating_supply_table/up.sql Outdated Show resolved Hide resolved
src/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/circulating_supply/lockup.rs Outdated Show resolved Hide resolved
@telezhnaya
Copy link
Contributor Author

Thank you so much @frol @khorolets, you've done amazing amount or work reviewing this.
I've rewritten it almost from scratch 😄
Had to rebase it to the current master. All new changes are in the second commit.
Please have a look again.

@telezhnaya
fixes after review
f8bd85a 
cleanup by fmt
the thread is alive in any scenario (no except/panic/etc)
use same connection to DB (pool) if possible
mark circulating supply logging with my own AGGREGATED tag
check that we calc circulating supply only for mainnet
add documentation
unify logic of computing historical and current circ supply
massive renaming in the DB
reorder files
frol
frol reviewed Aug 9, 2021
View reviewed changes
migrations/2021-08-04-151515_circulating_supply_table/up.sql Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/db_adapters/accounts.rs Outdated Show resolved Hide resolved
src/db_adapters/accounts.rs Outdated Show resolved Hide resolved
src/aggregated/account_details.rs Outdated Show resolved Hide resolved
src/aggregated/account_details.rs Outdated Show resolved Hide resolved
src/aggregated/account_details.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
frol
frol reviewed Aug 9, 2021
View reviewed changes
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/models/accounts.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
khorolets
khorolets approved these changes Aug 10, 2021
View reviewed changes
src/aggregated/account_details.rs Show resolved Hide resolved
src/aggregated/circulating_supply/lockup_types.rs Outdated Show resolved Hide resolved
@telezhnaya telezhnaya mentioned this pull request Aug 10, 2021
Closed
@telezhnaya
Copy link
Contributor Author

telezhnaya commented Aug 10, 2021 •
edited
Loading

@frol @khorolets please have a look

frol
frol reviewed Aug 10, 2021
View reviewed changes
src/main.rs Outdated
Comment on lines 23 to 26
const INDEXER_FOR_EXPLORER: &str = "indexer_for_explorer";
const AGGREGATED: &str = "aggregated";
const INTERVAL: std::time::Duration = std::time::Duration::from_millis(100);
const MAX_DELAY_TIME: std::time::Duration = std::time::Duration::from_secs(120);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI, these start bothering me. There is no action item at the moment, but before we extend indexer-for-explorer with even more background work, let's consider whether we need those and how to name those properly.

aggregated is too generic naming and it is not immediately clear why this const is even defined (indexer_for_explorer is not much better, but at least it is a name of the package, and kind of makes sense to use in the logging.

src/aggregated/circulating_supply/lockup.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/lockup.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/lockup.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
src/aggregated/circulating_supply/circulating_supply_provider.rs Outdated Show resolved Hide resolved
@telezhnaya
Copy link
Contributor Author

@frol please have a look again

@frol frol merged commit c55f7b2 into master Aug 11, 2021
@frol frol deleted the olya/circulating branch August 11, 2021 12:16
@telezhnaya telezhnaya linked an issue Aug 25, 2021 that may be closed by this pull request
How should we calculate circulating supply? #85
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khorolets khorolets khorolets approved these changes

@frol frol frol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How should we calculate circulating supply?
3 participants
@telezhnaya @frol @khorolets