chainlink icon indicating copy to clipboard operation
chainlink copied to clipboard

Adding gRPC dial capability for mercury

Open patrickhuie19 opened this issue 11 months ago • 4 comments

Summary This adds a grpc (over HTTP/2) mode to the mercury client, in addition to the currently in production wsrpc mode. We will need to support both modes for ~4-8 weeks while we learn to operate the service with grpc.

Discussion After lots of iteration, I chose to implement this change by manually changing the autogenerated proto client so that I could add grpc handlers. I found this to be by far the most efficient way to accomplish this - both generics and introducing a new higher level grpc client result in loads of duplicated and unnecessarily complicated code.

What do you need to review?

  • Proper test coverage to ensure no regressions to wsrpc behavior
  • Review the related WSRPC changes: https://github.com/smartcontractkit/wsrpc/pull/59. Includes the update to the autogeneration script
  • Metadata key naming. The key naming of the KV pairs end up in HTTP headers.

Key material in this PR is for testing only and is fine to be leaked.

patrickhuie19 avatar Mar 25 '24 06:03 patrickhuie19

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

  • #added For any new functionality added.
  • #breaking_change For any functionality that requires manual action for the node to boot.
  • #bugfix For bug fixes.
  • #changed For any change to the existing functionality.
  • #db_update For any feature that introduces updates to database schema.
  • #deprecation_notice For any upcoming deprecation functionality.
  • #internal For changesets that need to be excluded from the final changelog.
  • #nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
  • #removed For any functionality/config that is removed.
  • #updated For any functionality that is updated.
  • #wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

github-actions[bot] avatar Mar 25 '24 06:03 github-actions[bot]

Quality Gate failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

Quality Gate failed Quality Gate failed

Failed conditions
8 New Major Issues (required ≤ 5)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

We should separate the logic for gRPC and WS more clearly, especially when authentication is involved. Also, we shouldn't use the presence of a config tlsCertFile to decide on the control flow between gRPC and WS. We need something more explicit.

timweri avatar Jul 19 '24 17:07 timweri

We should separate the logic for gRPC and WS more clearly, especially when authentication is involved.

Do you have ideas? If its by separating the higher level abstractions, like Pool, I'm very hesitant to revisit that - I've already tried that and the code is an absolute mess. Most maintainers should not even be aware of this low level detail, so the code is isolated about as cleanly as I could make it.

Also, we shouldn't use the presence of a config tlsCertFile to decide on the control flow between gRPC and WS. We need something more explicit.

It also requires the env var CHAINLINK_MERCURY_USE_GRPC

patrickhuie19 avatar Sep 06 '24 17:09 patrickhuie19