Skip to content

chore: fix incorrect logging in updateHead #4191

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

santamasa
Copy link

I noticed that s.networkHead was being updated after the log statement, causing "from_height" to always match "to_height".
Now, the old value is stored before updating, ensuring the log accurately reflects the height change.

@github-actions github-actions bot added the external Issues created by non node team members label Apr 2, 2025
cristaloleg
cristaloleg previously approved these changes Apr 2, 2025
Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

Nice

renaynay
renaynay previously approved these changes Apr 2, 2025
@cristaloleg
Copy link
Contributor

Could you run make fmt and make lint locally? and commit changes, please.

@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 35.71429% with 9 lines in your changes missing coverage. Please review.

Project coverage is 44.83%. Comparing base (2469e7a) to head (ec0b498).
Report is 472 commits behind head on main.

Files with missing lines Patch % Lines
das/state.go 35.71% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             main    #4191     +/-   ##
=========================================
  Coverage   44.83%   44.83%             
=========================================
  Files         265      311     +46     
  Lines       14620    22901   +8281     
=========================================
+ Hits         6555    10268   +3713     
- Misses       7313    11525   +4212     
- Partials      752     1108    +356     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@santamasa santamasa dismissed stale reviews from renaynay and cristaloleg via 83a6fce April 2, 2025 08:57
@santamasa
Copy link
Author

Could you run make fmt and make lint locally? and commit changes, please.

cristaloleg done.

@cristaloleg
Copy link
Contributor

You've added lots of dots? why?

@santamasa
Copy link
Author

You've added lots of dots? why?

This is a logical conclusion to the sentence.

@cristaloleg
Copy link
Contributor

This is a logical conclusion to the sentence.

Please revert these dots. 1) that's not the scope of PR, 2) that's adds additional changes in git history for not a big reason.

Thanks.

@santamasa
Copy link
Author

This is a logical conclusion to the sentence.

Please revert these dots. 1) that's not the scope of PR, 2) that's adds additional changes in git history for not a big reason.

Thanks.

cristaloleg, np! updated.

Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Nice find! Would appreciate if you can do small good to haves before we merge

Comment on lines +156 to +159
oldHead := s.networkHead
s.networkHead = newHead
log.Debugw("updated head", "from_height", s.networkHead, "to_height", newHead)

log.Debugw("updated head", "from_height", oldHead, "to_height", newHead)
Copy link
Member

Choose a reason for hiding this comment

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

How about just swap order of lines instead of introducing new var?

Comment on lines +156 to +159
oldHead := s.networkHead
s.networkHead = newHead
log.Debugw("updated head", "from_height", s.networkHead, "to_height", newHead)

log.Debugw("updated head", "from_height", oldHead, "to_height", newHead)
Copy link
Member

Choose a reason for hiding this comment

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

Also looks like keys for values in the log are a bit off. Do you mind to update them for something better while we are on it? Like "old_head" and "new_head" or similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Issues created by non node team members kind:chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants