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

Update nodejs bindings to use runtime field count #366

Merged
merged 12 commits into from
Sep 20, 2023

Conversation

jtraglia
Copy link
Member

This PR updates the Node.js bindings to work with runtime field counts.

Note: the JSON trusted setup was outdated. It contained 4096 G2 points. Pulled in the latest version.

@jtraglia jtraglia changed the base branch from main to runtime-fields September 18, 2023 12:49
@asn-d6
Copy link
Contributor

asn-d6 commented Sep 19, 2023

@matthewkeil would be great if you could take a look here. Thanks!

bindings/node.js/lib/kzg.js Outdated Show resolved Hide resolved
bindings/node.js/src/kzg.cxx Show resolved Hide resolved
jtraglia and others added 2 commits September 19, 2023 07:12
Co-authored-by: George Kadianakis <[email protected]>
Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

You already got most of the items @jtraglia. Just the Error vs TypeError and the question about removing the Blob type. I tend to think that was a cleaner way of handling the array of bytes so its fixed length. What do you think? What about you @asn-d6?

Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

LGTM!! 🚀

Would love to actually run the tests to make sure all is well once #352 is approved and merged.

@jtraglia
Copy link
Member Author

Great. Yes, we'll be sure that all of the tests pass before merging!

@asn-d6
Copy link
Contributor

asn-d6 commented Sep 20, 2023

I'm slightly confused on why the bindings here use Blob in the typescript code, but uint8_t *blob in the C++ code.
From reading the comments above I see this is because "To my knowledge, like in C, it's not very practical in C++ to have a custom Blob type.".

Is this mixed usage of Blob preferable to just using uint8_t everywhere?

bindings/node.js/lib/kzg.d.ts Outdated Show resolved Hide resolved
bindings/node.js/src/kzg.cxx Outdated Show resolved Hide resolved
@jtraglia
Copy link
Member Author

I'm slightly confused on why the bindings here use Blob in the typescript code, but uint8_t *blob in the C++ code.

For the API, I think Blob is a more human friendly name. Keep in mind Blob is a Uint8Array under the hood:

export type Blob = Uint8Array; // Dynamic size

Is this mixed usage of Blob preferable to just using uint8_t everywhere?

For me, yeah. As long as it's Blob everywhere in the TS code & uint8_t * in the C++ code I'm good.

I suppose I don't know what the alternative is in the C++ code. Would you prefer we typedef Blob uint8_t? That would work, but I chose not to do that in the C code. We could also use std::vector<uint8_t> if you think that's a good idea.

Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks all!

@jtraglia jtraglia merged commit a0c26c5 into ethereum:runtime-fields Sep 20, 2023
@jtraglia jtraglia deleted the nodejs-runtime-field-count branch September 20, 2023 18:03
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