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
Epic for Bindings Rebuild #88
Comments
@philknows @dapplion I have a question about the process above. There is little overlap from old to new code now that everything is hand built instead of SWIG. I am intending to put the new code in with the old code to test for regression and for performance validation. We have some of that stuff already on the POC branches but figured it should be formalized during the merge. There is one DX question as both new and old code cannot sit at the root of the repo together and still build correctly. The old bindings do not have a The I can either put all the new code in a folder like The other option is to move all the old code into a |
@wemeetagain @g11tech @nazarhussain @tuyennhv @nflaig was curious if you guys had any thoughts about the process |
i think this can be closed since 1.0 has been released :) |
Rebuilding
blst-ts
Adding this issue as a tracker for rebuilding the bindings using
N-API
. There is not much code that is shared between the new and old versions so the ease the review process I am going to break stuff up into pieces. I am thinking about usingmkeil/rebuild
as the trunk branch and will merge all the pieces below into it. I think once everything is merged there may be a a PR to organize a bit and remove the stuff related to swig before mergingmkeil/rebuild
tomaster
.Review/Merge Hierarchy
Most of these PR's cascade. I was attempting to do the base Classes and then keep all the functions independent but there were a few changes in
verifyMultipleAggregateSignatures
that should cascade to avoid difficult merge conflicts for theverify
suite. Fixed the branch in PR#97 and merged the aggregates into the verify branch to testfastAggregateVerify
. Any remaining PR's will follow from #97 to avoid similar confusion.#89 -> #90 -> #91 -> #94 -> this is where there is a branch.
branch 1) #94 -> #92 -> #93
branch 2) #94 -> #95 -> #96
branches get merged together in) #97
#97 -> #98 -> #99 -> #100->#101
All unit tests and all specs are passing. The bulk of the code is substantially complete. Just the PR review changes and ancillary code (CI/CD etc) needs to be completed.
The implementation for this code in Lodestar can be found here:
Reviewer Notes
There is a lot of code in here and the first few pieces are mostly boilerplate to make the rest work. It may be easier to take a peek at #92 to see how everything comes together and gets implemented. Then peek at #91 and #94 to get an overview of the pki classes. Then circle back and start reviewing #90.
To view the entire lot at once,
mkeil/fix-agg-spec
has all of the pieces and is 100% passing specs and unit tests.Base
Classes
Addon
,BlstBase
,BlstAsyncWorker
,Uint8ArrayArg
,Uint8ArrayArgArray
SecretKey
,PublicKey
andSignature
PublicKeyArg
,PublicKeyArgArray
,SignatureArg
andSignatureArgArray
SecretKey.sign
aggregatePublicKey
andaggregateSignatures
verifyMultipleAggregateSignatures
verifyMultipleAggregateSignatures
function with unit testsverify
andaggregateVerify
fastAggregateVerify
fastAggregateVerify
unit and Spec tests (also covers merge of all function PR's)Cross-Compatability
Things to double check
HandleScope
is applied correctly everywhereconst &
everywhere applicableWishlist
Signature.deserialize
PublicKey.deserialize
Testing
valgrind
leak check testDevOps
node-pre-gyp
orprebuild
for popular platformsIntegration/E2E Testing
Audit
Documentation
HandleScope
N-API
and/ornode-addon-api
Docs Clarification and Submission Offer nodejs/node#47504The text was updated successfully, but these errors were encountered: