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

Import liquidjs interfaces in index.d.ts #38

Open
altafan opened this issue Mar 8, 2023 · 9 comments
Open

Import liquidjs interfaces in index.d.ts #38

altafan opened this issue Mar 8, 2023 · 9 comments
Assignees

Comments

@altafan
Copy link
Collaborator

altafan commented Mar 8, 2023

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

@louisinger
Copy link
Contributor

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
Copy link
Member

tiero commented Mar 8, 2023

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

This also works

@tiero
Copy link
Member

tiero commented Mar 8, 2023

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

@nicofuccella
Copy link

@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
Copy link
Contributor

@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:

  • tiny-secp256k1 does not define any interface.
  • bip32 defines an interface with some methods.
  • slip77 defines an interface with some other methods.

@nicofuccella
Copy link

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:

  • we can version the interface (one package may use the v1 and still have to migrate to v2, already in use by the other package, and that would be clear from the package.json dependencies section)
  • we have to write the interface only once
  • each package can only import a small dependency, the one with just the types

@louisinger what do you think?

@altafan
Copy link
Collaborator Author

altafan commented Mar 9, 2023

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

@tiero
Copy link
Member

tiero commented Mar 9, 2023

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

ACK

@louisinger
Copy link
Contributor

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

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

No branches or pull requests

4 participants