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

Add ability to decode signet #3128

Merged
merged 16 commits into from
May 6, 2024
Merged

Add ability to decode signet #3128

merged 16 commits into from
May 6, 2024

Conversation

thebrandonlucas
Copy link
Contributor

@thebrandonlucas thebrandonlucas commented Apr 12, 2024

Describe the changes you have made in this PR

This PR begins the process of properly decoding signet payments with Alby as mentioned by #3213. The issue is Alby doesn't seem to have signet enabled anywhere that I could find in the app, and I'm not familiar enough with the codebase to know where to conditionally retrieve the network. The bolt11 library Alby is using to decode invoices takes an optional second parameter that allows specification of a custom network, i.e.:

  const signet = {
    bech32: "tbs",
    pubKeyHash: 0x6f,
    scriptHash: 0xc4,
    validWitnessVersions: [0],
  };
  const paymentRequestDetails = lightningPayReq.decode(paymentRequest, signet);

Right now, the changes aren't very DRY and make payments only work with signet, so if someone more knowledgeable could show me how to grab the network dynamically and place the signet constant in the appropriate file, I would be happy to finish implementing.

Link this PR to an issue [optional]

Fixes #3213

Type of change

  • feat: New feature (non-breaking change which adds functionality)

Screenshots of the changes [optional]

Note that this is the signet Mutiny app

Screen.Recording.2024-04-12.at.6.37.10.PM.mov

How has this been tested?

Checklist

  • Self-review of changed code
  • Manual testing
  • Added automated tests where applicable
  • Update Docs & Guides
  • For UI-related changes
  • Darkmode
  • Responsive layout

@rolznz
Copy link
Contributor

rolznz commented Apr 21, 2024

@thebrandonlucas awesome, thanks for looking into this!

Actually, ideally the bolt11 package would actually support mutinynet. It already supports testnet/mainnet so it could also do this check internally. What do you think?

@thebrandonlucas
Copy link
Contributor Author

@rolznz So the library actually does support signets (see here), but only if an extra parameter is manually provided with the details of the custom network, as shown above. There are many different roads we could take with this to varying levels of complexity:

  1. Fork the bolt11 library (since it seems unmaintained), and modify the code to parse signet by default, then install this package from that fork or see if we can get it merged.
  2. For each current call to lightningPayReq.decode(), we check if the network is equal to signet. Not very DRY.
  3. Create a wrapper function that checks if the network being used for the current account is signet, if it is, parse with the custom parameters for mutinynet, if it isn't, parse without them.
  4. Create a wrapper function that just attempts to parse, if it receives the Unknown coin bech32 prefix error, try to parse it again with mutinynet parameters. If it did receive an error but it wasn't Unknown coin bech32 prefix, then it just re-throws.

I've gone with Option (4) as the DRYest solution that required the least refactoring, although it seems a bit suboptimal to be unnecessarily throwing/catching/rethrowing an error. I'm open to suggestions, but the way around this is for me to have a way to tell what the network is (preferably outside of a React component).

@thebrandonlucas
Copy link
Contributor Author

thebrandonlucas commented Apr 22, 2024

@rolznz I've updated the code and taking it out of draft. Let me know if there is anything else I need to do/fix.

@thebrandonlucas thebrandonlucas marked this pull request as ready for review April 22, 2024 20:39
@rolznz
Copy link
Contributor

rolznz commented Apr 23, 2024

Hi @thebrandonlucas

After looking at your changes, your first suggestion (forking the bolt11 library and using that until the PR gets merged) seems like the best option to me, as adding mutinynet there would be a very minor change and more consistent (it's the same checks as it already does for e.g. regtest). Then we just change the dependency in our package.json.

Copy link

socket-security bot commented Apr 24, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/bolt11-signet@1.4.1 None +3 1.97 MB bslucas
npm/braces@3.0.2 None +3 98.1 kB doowb

🚮 Removed packages: npm/bolt11@1.4.1, npm/colorette@2.0.20, npm/rxjs@7.5.7, npm/string-width@5.1.2, npm/wif@2.0.6, npm/wrap-ansi@8.1.0, npm/yaml@2.3.3

View full report↗︎

Copy link

socket-security bot commented Apr 24, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

@thebrandonlucas
Copy link
Contributor Author

After looking at your changes, your first suggestion (forking the bolt11 library and using that until the PR gets merged) seems like the best option to me, as adding mutinynet there would be a very minor change and more consistent (it's the same checks as it already does for e.g. regtest). Then we just change the dependency in our package.json.

@rolznz Done. I modified the bolt11 library to bolt11-signet, opened a PR to the original bolt11 library just in case that gets merged, and updated Alby's package to use it. Signet should now parse out-of-the-box now without any additional changes. Let me know if I need to do anything else. Cheers!

yarn.lock Outdated Show resolved Hide resolved
@rolznz
Copy link
Contributor

rolznz commented Apr 25, 2024

I tested this with https://signet-app.mutinywallet.com/ to create an invoice and NWC-next using mutinynet configuration connected to the Alby Extension to pay it - looks good!

@rolznz rolznz requested a review from bumi April 25, 2024 03:40
@thebrandonlucas
Copy link
Contributor Author

@rolznz updated!

Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

would be nice to get it into the official package. until then let's fix the version

package.json Outdated Show resolved Hide resolved
Co-authored-by: Michael Bumann <hello@michaelbumann.com>
@thebrandonlucas
Copy link
Contributor Author

would be nice to get it into the official package. until then let's fix the version

@bumi Agree 100%. If you want to bump here perhaps we can get it in sooner: bitcoinjs/bolt11#73.

Committed your suggestion in the meantime.

@bumi
Copy link
Collaborator

bumi commented Apr 26, 2024

@thebrandonlucas thanks for making the PR there! -and here obviously :)

@thebrandonlucas
Copy link
Contributor Author

@thebrandonlucas thanks for making the PR there! -and here obviously :)

@bumi happy to! let me know if I can do anything else here

yarn.lock Outdated Show resolved Hide resolved
@reneaaron reneaaron added the next-release To be included in the next release label Apr 29, 2024
Copy link
Contributor

@rolznz rolznz left a comment

Choose a reason for hiding this comment

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

tACK

@rolznz rolznz merged commit f2f25e2 into getAlby:master May 6, 2024
6 checks passed
@rolznz
Copy link
Contributor

rolznz commented May 6, 2024

Thanks @thebrandonlucas !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-release To be included in the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants