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

Add aptos-indexer-protos package for TS #112

Merged
merged 2 commits into from
Sep 1, 2023
Merged

Conversation

banool
Copy link
Collaborator

@banool banool commented Aug 30, 2023

Summary

This PR does the following:

  • Creates the aptos-indexer-protos package, which contains all the code generated from the protos, basic instructions for using them, a guide explaining how to publish the package, etc.
  • Switches all the code under typescript/ to use this package. I modernize the packaging / tsc stuff as part of this.

I will update the proto generation stuff in the GH workflows in a later PR.

Callouts

Some things worth calling out that we might want to discuss:

  • I have configured the generated code to represent u64s using bigint. Using string isn't very clear to the user and using number is dangerous.
  • You must refer to types in the generated code using the absolute "path" through the modules, e.g. aptos.indexer.v1.GetTransactionsRequest). Through a long discussion with some TS experts this seemed like the best option while maintaining compatibility with various downstream development environments while also maintaining our own sanity: https://aptos-org.slack.com/archives/C03JJF9DJ4V/p1693414593545719.
  • The getter and setter methods are gone from the generated code, it is just properties now. Imo this is cleaner and more modern but one downside is you have to null check yourself now, like transaction.user!. This is how it works in Rust too though so it's more standard this way.

Test Plan

I have tested that the library can be used from both commonjs and es code.

Testing our own example (commonjs):

$ cat /tmp/config.yaml
grpc_data_stream_endpoint: grpc.testnet.aptoslabs.com:443
grpc_data_stream_api_key: <token>
db_connection_uri: "postgresql://postgres:postgres@localhost:5432/example_event"
health_port: 8080
indexer_grpc_http2_ping_interval_in_secs: 30
indexer_grpc_http2_ping_timeout_in_secs: 5
starting_version: 0
chain_id: 2
cd typescript
pnpm build
node dist/processors/example-write-set-change-processor/processor.js process --config /tmp/config.yaml
{
  message: 'Successfully processed transaction',
  last_success_transaction_version: 39000n
}
{ message: 'Response received', starting_version: 40000n }
{
  message: 'Successfully processed transaction',
  last_success_transaction_version: 40000n
}
{ message: 'Response received', starting_version: 41000n }
{
  message: 'Successfully processed transaction',
  last_success_transaction_version: 41000n
}

@banool banool changed the base branch from main to banool/protos-package-python August 30, 2023 14:23
@banool banool force-pushed the banool/protos-package-ts branch 2 times, most recently from ed283ac to c03e91a Compare August 30, 2023 16:19
Base automatically changed from banool/protos-package-python to main August 30, 2023 17:10
@banool banool force-pushed the banool/protos-package-ts branch 8 times, most recently from 82a9a0f to fa05aff Compare August 31, 2023 10:31
@banool banool marked this pull request as ready for review August 31, 2023 10:35
@banool banool requested review from rtso and geekflyer August 31, 2023 10:43
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need this patch to fix an issue with the grpc-js library. More disucussion in https://aptos-org.slack.com/archives/C04PRP1K1FZ/p1683760992469519?thread_ts=1683670305.272199&cid=C04PRP1K1FZ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay good to know, let me add it back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sweet, can we add a comment in this explaining what it's for? So we don't accidentally delete it later on

} from "@/processors/example-write-set-change-processor/event-parser";
import { NextVersionToProcess } from "@/utils/common_models/NextVersionToProcess";

// A hack to override the http2 settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why delete this?

Copy link
Collaborator Author

@banool banool Aug 31, 2023

Choose a reason for hiding this comment

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

Well I'm not 100% sure why we had this in the first place but I think it was related to using an endpoint without TLS. We now have endpoints with TLS, so it's not necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be to make it work with grpc-js patch. @jjleng would know better!

Copy link
Contributor

Choose a reason for hiding this comment

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

no, this was not related to TLS. This is the settings we needed for the http2 flow control. Without these settings, the grpc link is very slow. With this, the link has similar performance as Python

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay got it, I'll try to make this work with TLS then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm having a bit of trouble getting this working, asking about it here: grpc/grpc-node#2565.

@banool banool force-pushed the banool/protos-package-ts branch from fa05aff to fffce36 Compare August 31, 2023 17:14
@rtso rtso requested a review from jjleng August 31, 2023 17:59
@banool banool force-pushed the banool/protos-package-ts branch from fffce36 to 6f04cb7 Compare September 1, 2023 17:01
@@ -2,5 +2,18 @@ version: v1
managed:
enabled: true
plugins:
- name: ts
out: typescript/
- plugin: buf.build/community/stephenh-ts-proto
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have documentation on how to regenerate the protobuf types somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah in the top level README. I clean that up a bit further in the PR after this.

@banool banool force-pushed the banool/protos-package-ts branch from 6f04cb7 to 938b976 Compare September 1, 2023 17:25
@banool banool force-pushed the banool/protos-package-ts branch from 938b976 to 4f3b4d0 Compare September 1, 2023 17:34
@banool banool merged commit 9945f0d into main Sep 1, 2023
@banool banool deleted the banool/protos-package-ts branch September 1, 2023 17:48
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