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

Add feature flag on account_changes and fungible_token_events #349

Merged
merged 6 commits into from
Feb 15, 2023

Conversation

eduohe
Copy link
Contributor

@eduohe eduohe commented Feb 11, 2023

Closes #346

@eduohe eduohe added the enhancement New feature or request label Feb 11, 2023
@eduohe eduohe self-assigned this Feb 11, 2023
@eduohe eduohe requested a review from telezhnaya February 11, 2023 01:34
Copy link
Contributor

@telezhnaya telezhnaya left a comment

Choose a reason for hiding this comment

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

Great job, thank you!
Could you please add the comment to readme about this, with the instruction how to enable this functionality?
We will add the link to the announcement later when we post it, but anyway we need to say at least 1 sentence here what's going on

database/Cargo.toml Outdated Show resolved Hide resolved
indexer/Cargo.toml Outdated Show resolved Hide resolved
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.

Overall LGTM! Although, I don't like the naming you've chosen. How about making it a bit shorter:

  • ft_events
  • account_changes

I propose dropping the load_ stuff as I haven't seen such naming in Rust features. @telezhnaya what do you think? I don't insist.

@telezhnaya
Copy link
Contributor

I don't have the strong opinion about the naming, we can go with any option.
I'm OK with dropping load_ prefix, but I personally prefer leaving fungible_token_events to match the naming we use in the code right now.

Copy link
Contributor

@telezhnaya telezhnaya left a comment

Choose a reason for hiding this comment

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

Major:
If we want to deploy this to production (we really want to), we need to rewrite start-from-interruption logic as well
The easiest way is to copy-paste the solution from microindexers: https://github.com/near/near-microindexers/blob/main/indexer-opts/src/lib.rs#L79

@eduohe
Copy link
Contributor Author

eduohe commented Feb 13, 2023

Thanks @telezhnaya and @khorolets , suggestions implemented

@eduohe eduohe requested a review from telezhnaya February 13, 2023 18:15
telezhnaya
telezhnaya approved these changes Feb 14, 2023
@telezhnaya telezhnaya force-pushed the feat/feature-flag-on-ft-and-account-changes-tables branch from 0af0523 to 0b2e007 Compare February 14, 2023 06:17
Copy link
Contributor

@telezhnaya telezhnaya left a comment

Choose a reason for hiding this comment

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

Please run cargo fmt & cargo clippy

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@telezhnaya telezhnaya merged commit 72440ef into master Feb 15, 2023
@telezhnaya telezhnaya deleted the feat/feature-flag-on-ft-and-account-changes-tables branch February 15, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sunset the collecting of account_changes and assets__fungible_token_events
3 participants