feat: refactor protobufs and utils to remove grpc-js dependency #798

PangZhi · 2023-04-03T20:55:46Z

Motivation

Part of #573, removed grpc-js dependency from protobufs (thus utils).

Change Summary

separate rpc.proto to request_response.proto and rpc.proto (only have service definition), remove grpc-js dependency from protobufs (thus utils)
move all grpc-js code generation to hub-nodejs
refactor hubble app to use hub-nodejs
add protobufs & utils as dependency for hub-web and add new exports

Merge Checklist

Choose all relevant options below by adding an x now or at any time before submitting for review

PR title adheres to the conventional commits standard
PR has a changeset
PR has been tagged with a change label(s) (i.e. documentation, feature, bugfix, or chore)
PR does not require changes to the protocol

Additional Context

…nodejs

changeset-bot · 2023-04-03T20:55:49Z

🦋 Changeset detected

Latest commit: f95eed5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@farcaster/protobufs	Minor
@farcaster/hub-web	Minor
@farcaster/utils	Minor
@farcaster/hub-nodejs	Patch
@farcaster/hubble	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

PangZhi · 2023-04-03T20:57:02Z

.changeset/honest-news-own.md

@@ -0,0 +1,10 @@
+---
+'@farcaster/protobufs': minor


ideally it's major change for protobufs and utils (interface not backward compatible). However, it seems that we haven't released major version of protobufs & utils yet, so I made it minor.

socket-security · 2023-04-03T20:57:18Z

New dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package	Capability Access	`+/-` Transitive Count	Publisher
@farcaster/[email protected]	None	`+1`	varunsrin

⬆️ Updated Package	Version Diff	Added Capability Access	`+/-` Transitive Count	Publisher
@grpc/[email protected]	1.8.7...1.8.13	None	`+0/-0`	murgatroid99

codecov · 2023-04-03T21:02:24Z

Codecov Report

Patch coverage: 85.58% and project coverage change: -0.43 ⚠️

Comparison is base (71d6494) 73.03% compared to head (a043e15) 72.61%.

❗ Current head a043e15 differs from pull request most recent head f95eed5. Consider uploading reports for the commit f95eed5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #798      +/-   ##
==========================================
- Coverage   73.03%   72.61%   -0.43%     
==========================================
  Files          61       62       +1     
  Lines        5359     5379      +20     
  Branches     1216     1212       -4     
==========================================
- Hits         3914     3906       -8     
- Misses       1351     1373      +22     
- Partials       94      100       +6

Impacted Files	Coverage Δ
apps/hubble/src/eth/utils.ts	`100.00% <ø> (ø)`
apps/hubble/src/hubble.ts	`1.86% <0.00%> (ø)`
apps/hubble/src/network/p2p/protocol.ts	`100.00% <ø> (ø)`
apps/hubble/src/rpc/adminServer.ts	`1.88% <0.00%> (ø)`
apps/hubble/src/rpc/server.ts	`79.24% <ø> (ø)`
apps/hubble/src/storage/db/hubState.ts	`36.36% <ø> (ø)`
apps/hubble/src/storage/db/idRegistryEvent.ts	`83.87% <ø> (ø)`
apps/hubble/src/storage/db/nameRegistryEvent.ts	`100.00% <ø> (ø)`
apps/hubble/src/storage/db/rocksdb.ts	`90.47% <ø> (ø)`
apps/hubble/src/storage/engine/seed.ts	`100.00% <ø> (ø)`
... and 27 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

PangZhi · 2023-04-03T21:03:36Z

apps/hubble/package.json

@@ -47,6 +47,7 @@
  "dependencies": {
    "@chainsafe/libp2p-gossipsub": "6.1.0",
    "@chainsafe/libp2p-noise": "^11.0.0 ",
+    "@farcaster/hub-nodejs": "^0.6.0",


What's the process for package release here? We need to release and update dependency to new version in order:
protobuf, utils, hub-nodejs & hub-web, hubble app

Do we need to break this PR into multiple PRs for release purpose?

We use changesets for version control, so we can run yarn changeset version and it will update all of these correctly.

PangZhi · 2023-04-03T21:08:49Z

Codecov Report

Patch coverage: 2.56% and project coverage change: -26.23 ⚠️

Comparison is base (d0239f8) 74.04% compared to head (c1a7796) 47.81%.

Additional details and impacted files
☔ View full report in Codecov by Sentry. 📢 Do you have feedback about the report comment? Let us know in this issue.

any config to ignore the coverage for generated code?

pfletcherhill

Exciting! I think the most important question to answer is whether we want hubble to import anything from @farcaster/protobufs or @farcaster/utils rather than getting everything from @farcaster/hub-nodejs. Right now it seems somewhat arbitrary which package a file picks for each import.

pfletcherhill · 2023-04-03T21:33:08Z

apps/hubble/src/rpc/test/bulkService.test.ts

@@ -1,5 +1,6 @@
 import * as protobufs from '@farcaster/protobufs';
-import { Factories, getInsecureHubRpcClient, HubResult, HubRpcClient } from '@farcaster/utils';
+import { Factories, HubResult } from '@farcaster/utils';


What's the reason to continue importing these from @farcaster/utils vs from @farcaster/hub-nodejs since hub-nodejs also exports everything from utils?

After thinking through I think it's cleaner and more consistent to just depend on hub-nodejs and use all imports there, refactored hubble

pfletcherhill · 2023-04-03T21:34:58Z

packages/protobufs/src/schemas/request_response.proto

@@ -0,0 +1,124 @@
+syntax = "proto3";


What's the reason for splitting rpc.proto into two files?

Personally feel this is slightly cleaner pattern: share whatever can be shared.
Where hub-nodejs and hub-web are different is the service/client generation part (exactly the rpc.proto after splitting). After splitting, we will be able to share like SyncIds and Empty from protobufs

pfletcherhill · 2023-04-03T21:36:22Z

packages/protobufs/package.json

@@ -18,7 +18,7 @@
  "scripts": {
    "build": "tsup --config tsup.config.ts",
    "clean": "rimraf ./dist",
-    "protoc": "protoc --plugin=./node_modules/.bin/protoc-gen-ts_proto --ts_proto_out=./src/generated/ --ts_proto_opt=esModuleInterop=true,exportCommonSymbols=false,outputServices=grpc-js,useOptionals=none,unrecognizedEnum=false,removeEnumPrefix=true --proto_path=./src/schemas ./src/schemas/*",
+    "protoc": "protoc --plugin=./node_modules/.bin/protoc-gen-ts_proto --ts_proto_out=./src/generated/ --ts_proto_opt=esModuleInterop=true,exportCommonSymbols=false,outputServices=false,useOptionals=none,unrecognizedEnum=false,removeEnumPrefix=true --proto_path=./src/schemas ./src/schemas/*",


Could we get rid of @farcaster/protobufs entirely and just add a protoc command to @farcaster/utils to generate the non-service protobuf classes for itself?

Yes this is exactly next step: completely merge these two into a single package. Was planning to do it in separate PR.

PangZhi · 2023-04-03T22:20:07Z

@sds do you know how to let codecov/path and codecov/project skip generated code from grpc? I think now they're failing because of generated code.

sds · 2023-04-03T22:25:40Z

I didn't set this tool up, so I'm not familiar with it (honestly didn't even know this existed until now 🙂).

It looks like we could specify the ignore option in codecov.yml to ignore the directories with generated files?

CC @pfletcherhill @varunsrin how much do we care about code coverage requirements being met?

varunsrin · 2023-04-03T22:28:13Z

fine to merge without code coverage requirements, theyre not meant to be blocking just a guide

but it would be nice if we added that to the ignore file to keep the code coverage metrics useful

PangZhi · 2023-04-03T22:31:52Z

codecov.yml

I

I didn't set this tool up, so I'm not familiar with it (honestly didn't even know this existed until now 🙂).

It looks like we could specify the ignore option in codecov.yml to ignore the directories with generated files?

CC @pfletcherhill @varunsrin how much do we care about code coverage requirements being met?

I think the codecov ignored the previous generated code in the protobufs package, but it's not in the codecov.yml. We should definitely fix so the metric is useful.

…actor_protobufs_utils

pfletcherhill

Looks good to me! One minor change and a question, otherwise ready to merge.

pfletcherhill · 2023-04-04T16:03:50Z

apps/hubble/src/console/protobufCommand.ts

@@ -1,5 +1,5 @@
-import * as protobufs from '@farcaster/protobufs';
-import { Factories } from '@farcaster/utils';
+import * as hub_nodejs from '@farcaster/hub-nodejs';


Nit: can you camelCase this variable?

pfletcherhill · 2023-04-04T16:09:26Z

packages/hub-web/src/index.ts


+export * from './generated/rpc';


Do we need to export from ./generated/request_response too? Or does the export of that file from @farcaster/protobufs work with the services exported from ./generated/rpc?

I tried locally using FidRequest (one of the request_response), so the export was from @farcaster/protobufs but things work well when I feed into rpc request.

PangZhi added 3 commits April 3, 2023 13:30

remove grpc-js dependency from protobufs, refactor hubble to use hub-…

29f423c

…nodejs

hub-web to use @farcaster/protobufs and utils

efc483b

add yarn.lock

142d226

PangZhi added the t-feat Add a new feature or protocol improvement label Apr 3, 2023

PangZhi commented Apr 3, 2023

View reviewed changes

pfletcherhill reviewed Apr 3, 2023

View reviewed changes

refactor hubble app to only use hub-nodejs

511ef02

PangZhi force-pushed the jf/refactor_protobufs_utils branch from c1a7796 to 511ef02 Compare April 4, 2023 00:22

update readme

7e9e274

PangZhi closed this Apr 4, 2023

PangZhi reopened this Apr 4, 2023

PangZhi closed this Apr 4, 2023

PangZhi and others added 6 commits April 3, 2023 18:17

update export order

acf3f5a

update export order

494a116

Merge branch 'main' of github.com:farcasterxyz/playground into jf/ref…

92c0c0e

…actor_protobufs_utils

fix protobufs namespace build issue

7810d91

Merge branch 'main' into jf/refactor_protobufs_utils

aed6683

update codecov

a043e15

PangZhi reopened this Apr 4, 2023

pfletcherhill approved these changes Apr 4, 2023

View reviewed changes

fix protobufCommand naming

f95eed5

PangZhi merged commit 1f47906 into farcasterxyz:main Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor protobufs and utils to remove grpc-js dependency #798

feat: refactor protobufs and utils to remove grpc-js dependency #798

PangZhi commented Apr 3, 2023

changeset-bot bot commented Apr 3, 2023 •

edited

Loading

PangZhi Apr 3, 2023

socket-security bot commented Apr 3, 2023 •

edited

Loading

codecov bot commented Apr 3, 2023 •

edited

Loading

PangZhi Apr 3, 2023 •

edited

Loading

pfletcherhill Apr 3, 2023

PangZhi commented Apr 3, 2023

Codecov Report

pfletcherhill left a comment

pfletcherhill Apr 3, 2023

PangZhi Apr 4, 2023

pfletcherhill Apr 3, 2023

PangZhi Apr 3, 2023

pfletcherhill Apr 3, 2023

PangZhi Apr 3, 2023

PangZhi commented Apr 3, 2023

sds commented Apr 3, 2023 •

edited

Loading

varunsrin commented Apr 3, 2023

PangZhi commented Apr 3, 2023

pfletcherhill left a comment

pfletcherhill Apr 4, 2023

pfletcherhill Apr 4, 2023

PangZhi Apr 4, 2023 •

edited

Loading

feat: refactor protobufs and utils to remove grpc-js dependency #798

feat: refactor protobufs and utils to remove grpc-js dependency #798

Conversation

PangZhi commented Apr 3, 2023

Motivation

Change Summary

Merge Checklist

Additional Context

changeset-bot bot commented Apr 3, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

socket-security bot commented Apr 3, 2023 • edited Loading

codecov bot commented Apr 3, 2023 • edited Loading

Codecov Report

PangZhi Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PangZhi commented Apr 3, 2023

Codecov Report

pfletcherhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PangZhi commented Apr 3, 2023

sds commented Apr 3, 2023 • edited Loading

varunsrin commented Apr 3, 2023

PangZhi commented Apr 3, 2023

pfletcherhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PangZhi Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

changeset-bot bot commented Apr 3, 2023 •

edited

Loading

socket-security bot commented Apr 3, 2023 •

edited

Loading

codecov bot commented Apr 3, 2023 •

edited

Loading

PangZhi Apr 3, 2023 •

edited

Loading

sds commented Apr 3, 2023 •

edited

Loading

PangZhi Apr 4, 2023 •

edited

Loading