vulpemventures / secp256k1-zkp

JavaScript bindings of libsecp256k1-zkp with support for pedersen commitments and range proofs.
MIT License
15 stars 10 forks source link

Import liquidjs interfaces in index.d.ts #38

Open altafan opened 1 year ago

altafan commented 1 year ago

In lib/index.d.ts we should import and export liquidjs interfaces rather than redefining them.

louisinger commented 1 year ago

nACK to import a liquidjs-lib dep in this repo in my opinion. As far as I remember, we add that index.d.ts to avoid errors while importing it in our Typescript libraries. So it's just a way to typedef the index.js file.

The right way would be to migrate the project to full typescript and let tsc compile the .d.ts (so the interface) file for us. Note that it's also the solution choosen by bitcoinjs/tinysecp256k1.

cc/ @altafan @tiero

tiero commented 1 year ago

and let tsc compile the .d.ts (so the interface)

This also works

tiero commented 1 year ago

that being said, there is way to not import the full liquidJS but only the type that was suggested by @nicofuccella

nicofuccella commented 1 year ago

@louisinger ideally you'd do something like this in your index.d.ts file

import ZKPInterface from "liquidjs-lib"

export ZKPInterface

and should work just fine. By doing this you don't have to maintain the same interface (thus exposing yourself to misalignments between the two) in both places (liquidjs-lib and secp256k1-zkp)

louisinger commented 1 year ago

@louisinger ideally you'd do something like this in your index.d.ts file

import ZKPInterface from "liquidjs-lib"

export ZKPInterface

In my mind this is not common and add a dependency on liquidjs.

By doing this you don't have to maintain the same interface (thus exposing yourself to misalignments between the two) in both places (liquidjs-lib and secp256k1-zkp)

If we want to be compliant with bitcoinjs philosophy, each libs using the secp256k1 implementations must define (and export) its own interface. Thus I think it's fine to keep liquidjs-lib ZKPInterface there and only use it while importing liquidjs. I you plan to use secp256k1-zkp in another project which does not depend on liquidjs, you could define your own interface with the methods you need. Then it's up to the implementation to fit it or not.

If we look at bitcoinjs world:

nicofuccella commented 1 year ago
In my mind this is not common and add a dependency on liquidjs.

I agree. You may want to use only secp256k1-zkp without liquidjs. However, we're developing for example a RN secp256k1-zkp version, which interface should we refer to in that case?

I think we should create a third package, @types/secp256k1-zkp, with the agreed interface. Then all the libs (secp256k1-zkp, rn-secp256k1-zkp, liquidjs-lib) can all import that. The advantages would be:

@louisinger what do you think?

altafan commented 1 year ago

ACK for me, seems like a nice trade-off between 2 discordant but legit philosophies.

tiero commented 1 year ago

I think we should create a third package, @types/secp256k1-zkp, with the agreed interface

ACK

louisinger commented 1 year ago

42 should solve this and compile the .d.ts file from typescript