search

ethereum / c-kzg-4844

A minimal implementation of the Polynomial Commitments API for EIP-4844 and EIP-7594, written in C.
Apache License 2.0
112 stars 105 forks source link

Replace `size_t` with `uint64_t` #478

Closed jtraglia closed 1 month ago

jtraglia commented 1 month ago

Opening as a draft. Not entirely sure I want this. I've had some mild concerns with size_t being a dynamic size. I'm not sure if my concerns are justified. This PR replaces all instances of size_t with uint64_t.

asn-d6 commented 1 month ago

Can you elaborate on what you think is the issue here? It would be weird if we were accepting size_t as input on a public method, since there could be cross-platform disagreements, but I don't see anything wrong with using the type in internal functions.

jtraglia commented 1 month ago

Hmm I don't have a strong opinion on what internal types we should use. I thought it might be good to use uint64_t everywhere for consistency, but that's not a great reason. Maybe I should revert that, and only do uint64_t for the public API functions, like you suggested. This is originally what I had in mind, but I'm on the fence as to what I think is best.

Screenshot of the task list:

image
jtraglia commented 1 month ago

I will make these changes sometime next week. Doesn't feel like a high priority right now.

asn-d6 commented 1 month ago

This looks good! Thanks!

As a question, is it possible to replace the ulong of the C# bindings to uint64 as well? https://github.com/ethereum/c-kzg-4844/blob/b58cd9d00a0d1ee6362d6bec6d99299b0a7cad65/bindings/csharp/Ckzg.Bindings/Ckzg.Bindings.cs#L67-L68