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

remove unused scribe config params #1383

Merged
merged 1 commit into from
Sep 15, 2023
Merged

Conversation

trajan0x
Copy link
Contributor

@trajan0x trajan0x commented Sep 15, 2023

Description

Remove two global scribe params that appear to be unused found when working on #1381

Summary by CodeRabbit


  • Refactor: Removed RefreshRate and ConfirmationRefreshRate fields from the Config struct in services/scribe/config/config.go. This change simplifies the configuration by eliminating unused refresh rates for updating block heights.
  • Test: Updated tests in services/scribe/config/config_test.go and services/scribe/service/scribe_test.go to reflect the removal of the RefreshRate field. These changes ensure that our tests remain accurate and reliable after the refactor.

@github-actions github-actions bot added go Pull requests that update Go code size/xs labels Sep 15, 2023
@trajan0x
Copy link
Contributor Author

@nautsimon if neither of these two params are in use how often do we hit the latest block?

image

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.06008% 🎉

Comparison is base (b57714b) 50.88921% compared to head (9e01c3b) 50.94929%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@                 Coverage Diff                 @@
##              master       #1383         +/-   ##
===================================================
+ Coverage   50.88921%   50.94929%   +0.06007%     
===================================================
  Files            353         353                 
  Lines          24966       24966                 
  Branches         277         277                 
===================================================
+ Hits           12705       12720         +15     
+ Misses         10981       10971         -10     
+ Partials        1280        1275          -5     
Flag Coverage Δ
agents 47.19455% <ø> (+0.26219%) ⬆️
cctp-relayer 63.33333% <ø> (ø)
explorer 25.43225% <ø> (ø)
promexporter 41.02564% <ø> (ø)
scribe 52.06408% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
services/scribe/config/config.go 37.50000% <ø> (ø)

... and 3 files with indirect coverage changes

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

@nautsimon
Copy link
Contributor

@nautsimon if neither of these two params are in use how often do we hit the latest block?

image

One of the changes to scribe was to set refresh rates at a contract level rather at a chain level. Refresh rate is an optional param that can be set for each contract, but will default to 1 second if not provided.

Once contracts are near head they are lumped into the big get logs query for multiple contracts and assume a 1 second refresh rate regardless of the set param.

@trajan0x trajan0x marked this pull request as ready for review September 15, 2023 17:48
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 15, 2023

Walkthrough

The changes primarily revolve around the removal of RefreshRate and ConfirmationRefreshRate fields from the Config struct in the scribe service. This has led to corresponding updates in the related test functions and YAML tags.

Changes

File Path Summary
.../scribe/config/config.go Removed RefreshRate and ConfirmationRefreshRate fields from Config struct, along with their YAML tags.
.../scribe/config/config_test.go Updated TestConfigEncodeDecode function to reflect removal of RefreshRate.
.../scribe/service/scribe_test.go Removed RefreshRate from scribeConfig in TestLivefillParity function.

🐇💻

"In the land of code, where logic is king,

We trim and we tweak, for performance we cling.

Refresh rates are gone, no longer they stay,

In the realm of the scribe, it's a brighter day!

Tests are updated, all neat and fine,

Celebrate this change, with carrots and wine!" 🥕🍷


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between e480b2b and 9e01c3b commits.
Files selected for processing (3)
  • services/scribe/config/config.go (1 hunks)
  • services/scribe/config/config_test.go (1 hunks)
  • services/scribe/service/scribe_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
  • services/scribe/config/config_test.go
  • services/scribe/service/scribe_test.go
Additional comments (Suppressed): 1
services/scribe/config/config.go (1)
  • 14-19: The RefreshRate and ConfirmationRefreshRate fields have been removed from the Config struct. Ensure that all parts of the codebase that previously accessed these fields have been updated to handle their absence.

@trajan0x trajan0x merged commit 98c4d3b into master Sep 15, 2023
44 checks passed
@trajan0x trajan0x deleted the feat/scribe-improvements branch September 15, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code size/xs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants