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

feat(eventindexer): handle reorg #13841

Merged
merged 11 commits into from
Jun 1, 2023
Merged

feat(eventindexer): handle reorg #13841

merged 11 commits into from
Jun 1, 2023

Conversation

cyberhorsey
Copy link
Contributor

handle reorg for relayer + eventindexer since david and i discovered event.Raw.Removed is unreliable, and fixed in taiko-client

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this pull request looks good. The code changes are well written and the commit messages are descriptive.

The changes to the event indexer looks like it is handling a reorg in an efficient manner. The addition of the 'blockID' column to the events table also looks appropriate.

I have a few minor suggestions:

  • In the save_block_proposed_event.go file, there is a comment that says "reorg detected". It might be worth adding a more descriptive comment here explaining what is happening and why.
  • In the save_block_verified_event.go file, the address field is set to an empty string when saving the event. It might be worth adding a comment here to explain why this is necessary.
  • The function names in the detect_and_handle_reorg.go file could be made more descriptive. For example, "detectReorgAndHandleEvent" might be more appropriate than "detectAndHandleReorg".
  • In the 1666650599_create_events_table.sql file, it might be worth adding a comment explaining why the blockID column was added.

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #13841 (bc14fff) into main (fed0ca0) will decrease coverage by 0.19%.
The diff coverage is 35.29%.

@@            Coverage Diff             @@
##             main   #13841      +/-   ##
==========================================
- Coverage   47.45%   47.26%   -0.19%     
==========================================
  Files         133      134       +1     
  Lines        3416     3459      +43     
  Branches      310      310              
==========================================
+ Hits         1621     1635      +14     
- Misses       1685     1710      +25     
- Partials      110      114       +4     
Flag Coverage Δ *Carryforward flag
bridge-ui 96.16% <ø> (ø) Carriedforward from 5cf4791
eventindexer 78.02% <86.66%> (+0.50%) ⬆️
protocol 0.00% <ø> (ø) Carriedforward from 5cf4791
relayer 59.57% <13.88%> (-1.02%) ⬇️
ui 100.00% <ø> (ø) Carriedforward from 5cf4791

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
packages/relayer/indexer/handle_event.go 40.54% <0.00%> (-2.50%) ⬇️
...ayer/indexer/save_message_status_changed_events.go 12.50% <0.00%> (-2.50%) ⬇️
packages/relayer/repo/event.go 78.66% <0.00%> (-4.44%) ⬇️
...ackages/relayer/indexer/detect_and_handle_reorg.go 23.80% <23.80%> (ø)
packages/eventindexer/repo/event.go 73.43% <86.66%> (+4.04%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cyberhorsey
Copy link
Contributor Author

will merge in the morning since it will require me to deploy eventindexer migrations and stuff on A#

@dantaik dantaik added this pull request to the merge queue May 31, 2023
@dantaik dantaik removed this pull request from the merge queue due to a manual request May 31, 2023
@davidtaikocha davidtaikocha added this pull request to the merge queue Jun 1, 2023
Merged via the queue into main with commit 0a26ce5 Jun 1, 2023
@davidtaikocha davidtaikocha deleted the handle_reorg branch June 1, 2023 05:29
@github-actions github-actions bot mentioned this pull request Jun 1, 2023
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.

4 participants