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

[WIP] Implement allowances that auto-pay invoices #222

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

wbobeirne
Copy link
Member

@wbobeirne wbobeirne commented Jun 13, 2019

Closes #42

Description

This adds a new module, appconf that can be used to add customizations to each webapp. The one implemented here is allowances, the idea of setting up rules that allow automatic payments to happen without user intervention.

This also adds a status bar that gives you quick actions you can take on the current page you're on.

A more thorough write-up of this PR is up on Medium: https://medium.com/p/2b08bec75e3a

TODOs

  • Make more chain agnostic (hard-coded use of sats in a few places)
  • Find good places to provide fiat equivalents
  • ???

Steps to Test

  1. Go to a WebLN page that you've enabled
  2. Confirm you see the enabled status bar
  3. Click the disable button, confirm it disables WebLN for app
  4. Re-enable it, and click on the piggy bank, confirm it created a default allowance for you
  5. Attempt to make a payment that fits the constraints, confirm it happened automatically for you
  6. Attempt to make a payment that does not fit the constraints, confirm you were prompted instead
  7. Configure the settings and make sure they all work as expected
  8. Add a new allowance directly from the allowances UI, confirm it correctly works when manually entered

Screenshots

Status Bar

Screen Shot 2019-06-11 at 4 05 02 PM

Allowance page

Screen Shot 2019-06-06 at 5 42 29 PM

Notifications

Screen Shot 2019-06-06 at 5 40 31 PM

Screen Shot 2019-06-06 at 5 40 38 PM

@wbobeirne wbobeirne requested a review from jamaljsr June 13, 2019 17:45
@bumi
Copy link
Contributor

bumi commented Jun 26, 2019

I was experimenting a bit with this PR. First of all: the UX is great! Super intuitive and nice to use. Can not wait to get this released :)

Here are a few things that I've noticed:

send payment for allowances is done in the context of the website

It seems currently the send payment call (POST /v1/channels/transactions) is done in a content script thus in the context of the current website. This does not work for me because of blocked cross origin calls. (calling the rest API of a lnd node without CORS). (debug screenshot)
Somehow I feel like the check if it is a "allowed" domain and handling the request could be done in the same "handlePrompt" receiver?

failed browser notifications

For whatever reason browser.notifications is undefined and thus browser.notifications.create and browser.notifications.clear fails. (screenshot
I could not find out why this is the case. (I was using Chrome 75.0.3 on Linux)

bolt11 (and coinfo/bitcoinjs-lib) currently does not support simnet network

And the bolt11 package adds some dependencies and I was wondering if we can use the /v1/payreq API call to decode the payment request.

@wbobeirne
Copy link
Member Author

send payment for allowances is done in the context of the website

Yep, this is definitely a hacked together version. I think I'm going to have to setup a communication channel with the background script and do all node interactions in there. I definitely don't want node communications outside of the extension sandbox. Thanks for noting this.

failed browser notifications

This is possibly a permissions issue? I'll have to dig a little deeper to see why it might be missing. But I should definitely be handling cases where notifications go wrong.

I was wondering if we can use the /v1/payreq API call to decode the payment request.

I wanted to avoid this for latency reasons, my remote node can sometimes respond a little slow and I didn't want to add 500ms+ delays to payment prompts to decode the invoice. I'd rather contribute to bolt11 and make that library more robust than require a network request.

@bumi
Copy link
Contributor

bumi commented Jul 1, 2019

fyi: I was experimenting a bit with this and also moving parts into the background script.
My changes are in my fork - in case you want to follow: autopay...bumi:autopay (includes loads of debug stuff for me to understand what is going on and get more familiar with the code)

@bumi
Copy link
Contributor

bumi commented Jul 4, 2019

it seems the state of crypto.isRequestingPassword is always true screenshot - do you have an idea why that is the case? Shouldn't that be false once one payment is done and the checkbox is set to store the password for the session?

@wbobeirne
Copy link
Member Author

wbobeirne commented Jul 4, 2019

In that screenshot, password is still null so it would seem that the password hasn't yet been entered or fetched from the background script. The hasSetPassword indicates that they have ever set a password, not that they've entered it that session.

The password is only filled into the store state again when you've checked the remember checkbox once it's needed. You can see that logic over in the crypto saga.

@bumi
Copy link
Contributor

bumi commented Jul 4, 2019

ok, thanks for the hint.
somehow it seems this is always the case. inspecting the state here or here gives me an empty password.

Though I don't have to enter the password when I confirm the prompt.
Is this related to the autopay changes? or some bug? I am currently stuck there any hints would be awesome :) - sadly I don't know the redux-saga concept yet.

update: after some more debugging I don't find/understand how the password should be stored. also I failed to try to add it. any hints are welcome :) thx.

@wbobeirne
Copy link
Member Author

wbobeirne commented Jul 19, 2019

Hey @bumi, sorry for leaving you hanging so long on this one. The password saving behavior is definitely a little tricky, and sagas are doubly tricky. The way it works is once you enter your password with the save password checkbox set, it send a message to the background script (sending code here, saving code here) to hold onto it in memory.

Whenever a password request happens, we first check in with the background script if it has the password cached (Request from background here, background sending back here). Now, my understanding is that when that happens, it should fire the ENTER_PASSWORD action that sets it all in state. I'm not entirely sure why you're not seeing what you're seeing in your logs, I'll do a little investigating of my own to see if I can reproduce it.

@jamaljsr
Copy link
Collaborator

@wbobeirne I see you requested a review on this PR. I'm looking forward to playing with it. Is it ready? I should have time over the weekend.

@wbobeirne
Copy link
Member Author

Haha, oh man I totally forgot I had tagged you ages ago. Finally scratching out some free time to work on Joule again.

Planning on getting this in a stable state and trying to entice people to run it as a beta for me for some sats in exchange. Still has some work to go after the refactor in #244, but it should be pretty close.

Happily using Polar to do my testing with this right now.

@jamaljsr jamaljsr removed their request for review February 29, 2020 07:39
@jamaljsr
Copy link
Collaborator

No worries. I just saw a notification and wasn't sure if that was new or old. Feel free to let me know if you need any help with anything.

Once you go Polar, it's hard to go back to the terminal. Lol

@bumi
Copy link
Contributor

bumi commented Mar 2, 2020

<3 I am also jumping back on this. let me know if I can help you with something.

@wbobeirne
Copy link
Member Author

Unfortunately in spite of moving the requests to the background context, we still get stopped with ERR_SSL_PROTOCOL_ERROR errors when trying to fetch to self signed certificate nodes. This is doubly bad because unlike the current workaround of going to the URL with the self signed cert and manually approving it, the background script will error out no matter what.

Unfortunately this type of automatic node communication may be dead in the water unless a native bridge is built (#106).

@bumi
Copy link
Contributor

bumi commented Mar 31, 2020

hmm. does that mean current master also has that issue?
I could try working on a native proxy for a showcase. But native extension can not be bundled with the extension, can they?
I think it should be prevented as official requirement as much as possible.

@wbobeirne
Copy link
Member Author

Yeah, that issue is currently present in develop (master is always the latest release, develop is the working branch.) I may revert those commits given these findings if I start working on something else, so that a new release doesn't break people's existing installations.

Native programs can't be bundled with extensions, they have to be downloaded and installed separately. This isn't an uncommon setup with some applications, like 1Password, where they have a version of the extension that requires a native application to be paired with it.

Not having native access has been a blocker for a ton of issues, such as switching to use the gRPC API, dealing with invalid TLS cers, being able to implement channel close, being able to handle lightning: URIs, and a ton of other things. While I've tried very hard to avoid going native, it definitely seems like it unlocks a ton of possibilities for Joule.

bumi added 2 commits May 24, 2020 17:28
The lastPaymentAttempt is needed to verify the cooldown period for
autopayments.
Store last payment attempt in the allowance config
@wbobeirne wbobeirne mentioned this pull request Aug 16, 2020
@bumi
Copy link
Contributor

bumi commented Sep 29, 2020

fyi. this is the proxy app I've built to work around the invalid cert error: https://github.com/bumi/lnd-proxy
it runs a local proxy and proxies requests from localhost to the remote LND node.

@wbobeirne
Copy link
Member Author

Dude, this is very slick! I'd be happy to throw something in the readme after I look it over. Nice work.

@bumi
Copy link
Contributor

bumi commented Sep 30, 2020

it is a pretty early version and I have to learn some Golang for it :) And I am wondering about the UX and the usefulness of it.
If the plan is to move towards a native bridge, then that could be a first step maybe.

What is your plan with the allowance feature? As the communication through the background script should get reverted do you see any chance to implement the allowances? Could it maybe be optional?

@curtcurt87
Copy link

webpack.config.js

Copy link

@curtcurt87 curtcurt87 left a comment

Choose a reason for hiding this comment

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

webpack.config.js

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

Successfully merging this pull request may close these issues.

Allow user to setup automatic payments per lapp
4 participants