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

Loop integration (revised) #231

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

Loop integration (revised) #231

wants to merge 39 commits into from

Conversation

wbobeirne
Copy link
Member

Revision of #226 from @callmekurisu

Description

This is a rework of a previous PR to integrate Lightning Labs' Loop functionality into Joule. Here are the main changes I've made from that original PR:

  • Reworked the UI to have a dedicated screen for setting URL. Adds some instructions and an introduction to Loop, so that newcomers won't be lost.
  • Refactored the reducers to have explicit loading and error fields for each request type, and to consolidate a lot of code between loop out and in, since they're basically the same APIs and responses.
  • Refactored the sagas to have one method for each action that's shared between loop in and out.
  • Fixed incorrect conf argument. This is supposed to be block confirmations, but was just using the amount field instead. This now allows for (much) lower chain fees.
  • Only sync config on LOOP_SET_URL action.
  • Mild UI improvements all around.

There are still some possible improvements to be done, but overall this feels pretty polished at this point with what's available. Unfortunately the monitor endpoint present in the gRPC API isn't available to the REST API, so we can't keep users informed about their loops-in-progress.

I want to do a bunch more testnet testing though just to make sure it's fully robust before merging.

Things to do

  • Add a warning for users on older versions of Loop that don't support Loop In or its REST API. This isn't yet in a released version of Loop so this'll be busted for lots of folks.

Steps to Test

  1. Do a loop in, confirm it works
  2. Do a loop out, confirm it works
  3. Attempt to connect to an invalid / down loop server, confirm you get a useful error message
  4. Attempt to loop with invalid arguments, confirm they all get corrected

Screenshots

Empty form

Screen Shot 2019-09-08 at 7 51 28 PM

Advanced Fields

Screen Shot 2019-09-08 at 7 52 31 PM

Fee breakdown

Screen Shot 2019-09-08 at 7 51 46 PM

@reemuru
Copy link
Contributor

reemuru commented Sep 9, 2019

The UI is hella polished now. This looks damn good man! Nice job on the refactoring too. It was super hacky before. I don't have anything to add. I plan to continue testing this on mainnet to see how it goes but should be gtg as soon as the new Loop API changes are released. I was actually working on loop monitor rest API changes but it is taking longer than expected. Could make that a goal upon next release to get some more notifications to the user as loop monitor progresses.

@Roasbeef
Copy link

Roasbeef commented Sep 9, 2019

Loop there it is!!!

@wbobeirne wbobeirne mentioned this pull request Apr 17, 2020
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.

3 participants