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

Recommend RPC-based pull event source over WebSocket-based push event source #4190

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

romac
Copy link
Member

@romac romac commented Sep 10, 2024

Closes: #XXX

Description


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@romac romac requested a review from ancazamfir September 10, 2024 07:22
@romac romac marked this pull request as ready for review September 10, 2024 07:22
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Are there any tests left for push mode?

@@ -123,7 +123,7 @@ grpc_addr = 'http://127.0.0.1:9090'

# Specify the WebSocket address and port where the chain WebSocket server
# listens on. Required
event_source = { mode = 'push', url = 'ws://127.0.0.1:26657/websocket', batch_delay = '500ms' }
event_source = { mode = 'pull', interval = '500ms', max_retries = 3 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment above needs update.

@ancazamfir
Copy link
Collaborator

As discussed in the IBC meeting:

  • we should have a way to still test the push mode for a few releases at least (can be done in this PR or another)
  • determine if pull mode only is enough for most operators
  • make a decision to deprecate (or not) support for push mode.

Copy link
Contributor

@ljoss17 ljoss17 left a comment

Choose a reason for hiding this comment

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

What is the reason we recommend pull over push? I think we should write something either in a new section of the Configuration section, https://hermes.informal.systems/documentation/configuration/index.html, or if this is only related to performance it can also be added directly in the performance tuning section https://hermes.informal.systems/documentation/configuration/performance.html.


# ...
```

> **Caution:** The "Basic" authentication scheme sends the credentials encoded but not encrypted.
> This would be completely insecure unless the exchange was over a secure connection (HTTPS/TLS).

## Configuring Support for Wasm Relaying
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the entire block removed?

@romac romac marked this pull request as draft October 31, 2024 15:43
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.

3 participants