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

refactor: update naming and API #15

Merged
merged 4 commits into from
Jan 29, 2024
Merged

Conversation

gabrocheleau
Copy link
Contributor

@gabrocheleau gabrocheleau commented Jan 27, 2024

Resolves #11

This PR unifies the naming and gives more explicit names to some classes/helpers.

It also makes some improvements to types and constructor methods for the Point and ScalarField classes.

I will being writing some in-code documentation for all the exposed methods. I will do this in another PR, as I want to cross-check my understanding of some of the classes/methods with Kev.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Hi there, thanks for the PR! 🙂

Thinking about naming and the whole structure we have right now while reviewing this.

I will merge for now, getting more and more the impression that we might want to continue to relatively strongly evolve this while our understanding deepens.

Some narrow things I had which I think deserve to get continued thought.

  1. More expressive names: that's not such a big one, but for my taste the names are still very much too generic, something like Point e.g. (that feels a bit as if we would name our keccak256() method hash() or even: function() 🙂 ).

So these names should give much stronger pointers to what the things actually contain/do/.... Not sure if this specific one is the correct choice, but something like BanderwagonPoint, ...

  1. API feels arbitraty. The API in its current bundling feels very (very) arbitrary and puzzled together.
  • Point
  • commitToPoly()
  • batchMapToScalarField()

Hmm. 🤔 That might be partly due to naming. But I am starting to think more and more that we think too narrow about API design here. Instead of thinking what our very very specific use case is exactly needing we should rather be more focused on the question "what functionality is the underlying library providing?". And then expose this in a consistent and graspable way, so that people can take this for their use cases and we (as just one consumer) can use a subset of the exposed functionality too.

End of 2. 🙂

So in the context of 2. I stumbled for the first time more consciously about the fact that yeah there are actually these divided and separte libraries over on Kevs side (late on the party, I know).

  • banderwagon (the curve)
  • ipa-multipoint (the commitment)
  • verkle-spec (not completely getting this one)
  • verkle-trie (higher level verkle trie implementation)

I wonder if it wouldn't be possible and at the end cleaner and clearer to also do the exposures here along the lines of these libraries? Or at least limit to the lower level ones? Also stumbled upon this issue crate-crypto/rust-verkle#72 from Kev talking about taking out verkle from his repository which somewhat fits in here to the topic.

So might it be possible to just use the first two (banderwagon and ipa-multipoint) libs and expose the crypto there in a more generic way? 🤔

These feels like the more correct line, since we have (work on) our verkle package as well, which seems to be somewhat the equivalent to the verkle-trie package from Kev (Kev, if you are reading along, you are of course invited to comment as well!). A big practical point here is - to already mention - also the package/WASM size (on our side), which is likely too much blown up for production if we aribtrarily take the verkle-trie package from Kev in.

So this all is rather mid-term thinking (I guess?), but at least wanted to write this down once now already, so that things can evolve in a good direction. For now it would likely be also ok to just use the stuff we have. BUT if this is structurally not TOO difficult to switch over, it might also be worth it to do relatively early in the process.

@holgerd77 holgerd77 merged commit 2ea95d9 into master Jan 29, 2024
3 checks passed
@holgerd77 holgerd77 deleted the refactor/improve-naming branch January 29, 2024 09:59
@holgerd77
Copy link
Member

holgerd77 commented Jan 29, 2024

(various edits on the previous post, please read on side 🙂 )

Subsequent notes here: maybe this is not so "mid-term" after all, but rather now is really the time to think about these things. If we go this changes path this would come with a very different package structure rather publishing something likebanderwagon-ipa-wasm or however this will be structured.

So rather something to take the time for now and NOT later on. 🙂

@gabrocheleau
Copy link
Contributor Author

Hi there, thanks for the PR! 🙂

Thinking about naming and the whole structure we have right now while reviewing this.

I will merge for now, getting more and more the impression that we might want to continue to relatively strongly evolve this while our understanding deepens.

Some narrow things I had which I think deserve to get continued thought.

  1. More expressive names: that's not such a big one, but for my taste the names are still very much too generic, something like Point e.g. (that feels a bit as if we would name our keccak256() method hash() or even: function() 🙂 ).

So these names should give much stronger pointers to what the things actually contain/do/.... Not sure if this specific one is the correct choice, but something like BanderwagonPoint, ...

  1. API feels arbitraty. The API in its current bundling feels very (very) arbitrary and puzzled together.
  • Point
  • commitToPoly()
  • batchMapToScalarField()

Hmm. 🤔 That might be partly due to naming. But I am starting to think more and more that we think too narrow about API design here. Instead of thinking what our very very specific use case is exactly needing we should rather be more focused on the question "what functionality is the underlying library providing?". And then expose this in a consistent and graspable way, so that people can take this for their use cases and we (as just one consumer) can use a subset of the exposed functionality too.

End of 2. 🙂

So in the context of 2. I stumbled for the first time more consciously about the fact that yeah there are actually these divided and separte libraries over on Kevs side (late on the party, I know).

  • banderwagon (the curve)
  • ipa-multipoint (the commitment)
  • verkle-spec (not completely getting this one)
  • verkle-trie (higher level verkle trie implementation)

I wonder if it wouldn't be possible and at the end cleaner and clearer to also do the exposures here along the lines of these libraries? Or at least limit to the lower level ones? Also stumbled upon this issue crate-crypto/rust-verkle#72 from Kev talking about taking out verkle from his repository which somewhat fits in here to the topic.

So might it be possible to just use the first two (banderwagon and ipa-multipoint) libs and expose the crypto there in a more generic way? 🤔

These feels like the more correct line, since we have (work on) our verkle package as well, which seems to be somewhat the equivalent to the verkle-trie package from Kev (Kev, if you are reading along, you are of course invited to comment as well!). A big practical point here is - to already mention - also the package/WASM size (on our side), which is likely too much blown up for production if we aribtrarily take the verkle-trie package from Kev in.

So this all is rather mid-term thinking (I guess?), but at least wanted to write this down once now already, so that things can evolve in a good direction. For now it would likely be also ok to just use the stuff we have. BUT if this is structurally not TOO difficult to switch over, it might also be worth it to do relatively early in the process.

Completely agree with these points overall.

Concerning what we want to expose, I'd be tempted to stick to the following API, described by Kev, which should provide all the necessary handlers for a full verkle implementation: https://hackmd.io/@6iQDuIePQjyYBqDChYw_jg/H1xXvMatq?utm_source=preview-mode&utm_medium=rec

Concerning the naming & docs, I will schedule a call with Kev (he's traveling this week, so tentatively next week) where we will go over the methods and write out docs, which will also clarify what each method does (I'm not clear on what all of them do, and I feel like that's a pre-requisite to come up with good naming)

@holgerd77
Copy link
Member

Concerning what we want to expose, I'd be tempted to stick to the following API, described by Kev, which should provide all the necessary handlers for a full verkle implementation: https://hackmd.io/@6iQDuIePQjyYBqDChYw_jg/H1xXvMatq?utm_source=preview-mode&utm_medium=rec

I'd like to go a bit deeper here, guess best to exchange on this in some broader way on the call with Kev next week.

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.

Rename classes & methods
2 participants