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

fix WS_NEW_HEADS_ENABLED default #3226

Merged

Conversation

treethought
Copy link
Contributor

@treethought treethought commented Nov 6, 2024

Description:
Restores the behavior of defaulting to true for the env var.

Fixes #3227

This change was originally made in #2361, but seems to have been lost in #3047 (more specific at this line)

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@quiet-node
Copy link
Member

quiet-node commented Nov 6, 2024

Hello @treethought, thank you very much for your contribution! Great catch!! However, could you please create a GitHub issue detailing the problem and the intended purpose of this fix? Additionally, kindly link the issue to this PR.

For guidance on structuring your contribution to align with our standards, please refer to this document.

@quiet-node quiet-node added good first issue Good for newcomers Internal For changes that affect the project's internal workings but not its outward-facing functionality. labels Nov 6, 2024
@quiet-node quiet-node added this to the 0.60.0 milestone Nov 6, 2024
quiet-node
quiet-node previously approved these changes Nov 6, 2024
Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LGTM! TYVM for the great catch!

@quiet-node
Copy link
Member

@natanasow Hey Nikki could you please double check to make sure that the loss at at this line wasn't on purpose?

@natanasow
Copy link
Collaborator

@natanasow Hey Nikki could you please double check to make sure that the loss at at this line wasn't on purpose?

@quiet-node It wasn't on purpose and most likely happened during resolving conflicts.

@treethought There is another approach I would choose for that issue. The new config service supports default values so we can add it here https://github.com/hashgraph/hedera-json-rpc-relay/blob/main/packages/config-service/src/services/globalConfig.ts#L713

@victor-yanev
Copy link
Contributor

@treethought There is another approach I would choose for that issue. The new config service supports default values so we can add it here https://github.com/hashgraph/hedera-json-rpc-relay/blob/main/packages/config-service/src/services/globalConfig.ts#L713

+1, it would be better for default values to be defined inside globalConfig.ts

Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution!

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.17%. Comparing base (4f0b2ea) to head (d2f61ea).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3226   +/-   ##
=======================================
  Coverage   83.17%   83.17%           
=======================================
  Files          66       66           
  Lines        4314     4314           
  Branches      843      843           
=======================================
  Hits         3588     3588           
  Misses        483      483           
  Partials      243      243           
Flag Coverage Δ
config-service 98.14% <ø> (ø)
relay 85.29% <ø> (ø)
server 83.52% <ø> (ø)
ws-server 36.87% <ø> (ø)

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

Files with missing lines Coverage Δ
...ckages/config-service/src/services/globalConfig.ts 100.00% <ø> (ø)

Copy link
Collaborator

@natanasow natanasow left a comment

Choose a reason for hiding this comment

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

LGTM.

@quiet-node
Copy link
Member

quiet-node commented Nov 7, 2024

@treethought Looks like your commit wasn't signed by your GPG key. Here’s a helpful resource: Signing Commits.

Copy link

sonarcloud bot commented Nov 7, 2024

@treethought
Copy link
Contributor Author

@treethought Looks like your commit wasn't signed by your GPG key. Here’s a helpful resource: Signing Commits.

pushed with signed commit

Copy link
Contributor

@ebadiere ebadiere left a comment

Choose a reason for hiding this comment

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

LG

@quiet-node quiet-node merged commit 3bd07ef into hashgraph:main Nov 8, 2024
36 of 37 checks passed
@quiet-node
Copy link
Member

@treethought merged the commit! 🚀🚀🚀 Thank you very much for the work! Look forward to more contributions in the future! 🙌🏼🙌🏼🙌🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers Internal For changes that affect the project's internal workings but not its outward-facing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eth_subscribe newHeads returns Unsupported JSON-RPC method
5 participants