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

signet support #1820

Closed
nicolasburtey opened this issue Jun 26, 2022 · 5 comments
Closed

signet support #1820

nicolasburtey opened this issue Jun 26, 2022 · 5 comments

Comments

@nicolasburtey
Copy link

signet support is missing from the library

(this is generating typescript error here: https://github.com/GaloyMoney/galoy/blob/main/src/domain/bitcoin/onchain/tx-decoder.ts @openoms @dolcalmi)

@junderw
Copy link
Member

junderw commented Jun 26, 2022

Adding signet to this library will only temporarily fix the issue of how networks are handled by galoy.

  1. The list of networks depends directly on the version of bitcoinjs-lib they are using, and yet they define the list explicitly, separately in their library.
  2. They have a list of address regexes which is based off of known address prefix results which does not depend at all on bitcoinjs-lib.
  3. They define an implicit alias of mainnet -> bitcoin at the point of usage (not at the point of definition of this list)

At each point where BtcNetwork comes into play, it has varying levels of depending on bitcoinjs-lib, which will cause problems.

It would be better to gather everything into one place in their codebase, import the existing network objects from bitcoinjs-lib, add whichever networks are missing + create any aliases needed.

Because they did not do this, and the code was written with the implicit misunderstanding that "bitcoinjs-lib already has signet as a default network object" which causes the problem.


That being said, I am not opposed to adding signet as a named network here. It's basically like another testnet, so having it as a named network should be fine.

Either way you will need galoy to update to a new version, so why not fix their network usage on a more fundamental level instead? ie. Remove assumptions from the code instead of making the library mold to the assumptions?

To make my concerns easier to understand, I will pose this rhetorical question: "Every time galoy adds some new network (signet2 or signet3 or signetTheEmpireStrikesBack) are we going to be required to add that network as well?"

The fact that I have to ask that question points to there being a fundamental flaw in the way galoy is handling the network object usage, and I think the issue should be fixed there first.


Again, I am not against adding signet, though.

@nicolasburtey
Copy link
Author

we could indeed have a proper mapping from our domain object to bitcoinjs-lib, but signet is part of the default network in bitcoin-core so I think that would make sense to have it here also?

@nicolasburtey
Copy link
Author

PR to have proper mapping on the galoy side: GaloyMoney/blink@b474369

I guess we can run with this for now

@h76oeI6pMxU9g4p8aCpc6Q
Copy link

h76oeI6pMxU9g4p8aCpc6Q commented Jul 24, 2022

I used bitcoinjs-lib with signet with no problem.
I just setting the network to "testnet", then bitcoinjs-lib can generate address and transaction using on signet.
The signet address seems like same as testnet address.

@jasonandjay
Copy link
Contributor

I agree that signet and testnet are almost exactly the same at the application level.

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