-
Notifications
You must be signed in to change notification settings - Fork 81
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
Conversation
ed283ac
to
c03e91a
Compare
82a9a0f
to
fa05aff
Compare
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why delete this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
fa05aff
to
fffce36
Compare
fffce36
to
6f04cb7
Compare
@@ -2,5 +2,18 @@ version: v1 | |||
managed: | |||
enabled: true | |||
plugins: | |||
- name: ts | |||
out: typescript/ | |||
- plugin: buf.build/community/stephenh-ts-proto |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
6f04cb7
to
938b976
Compare
938b976
to
4f3b4d0
Compare
Summary
This PR does the following:
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:
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.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):