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 lnbits app #372

Merged
merged 19 commits into from Feb 14, 2021
Merged

Add lnbits app #372

merged 19 commits into from Feb 14, 2021

Conversation

louneskmt
Copy link
Contributor

@louneskmt louneskmt commented Dec 29, 2020

App Submission

App name

LNbits

Version

v0.2.0

One line description of the app

(max 50 characters)

Multi-user wallet management system

Summary of the app

(50 to 200 words)

LNbits is a simple multi-user and account system for Lightning Network that can be used for creating separate Lightning wallets for friends and family members. You can also create multiple accounts for yourself to mitigate the risk of exposing applications to your full balance via unique API keys for each wallet.

LNbits is packaged with tools to help manage funds, such as a table of transactions, line chart of spending, export to CSV, and more to come. It provides an extendable platform for expanding Lightning Network functionality via LNbits extension framework, and can also be used as a fallback wallet for the LNURL scheme.

Developed by

LNbits

Developer website

https://github.com/lnbits/lnbits

Source code repository

https://github.com/lnbits/lnbits

Support link

(Link to your Telegram support channel, GitHub issues/discussions, support portal, or any other place where users could contact you for support.)

https://t.me/lnbits

Uses

  • Bitcoin Core
  • Electrum server
  • LND

256x256 SVG icon

(GitHub doesn't allow uploadig SVGs directly. Upload your file to an alternate service, like https://svgur.com, and paste the link below.)

icon

App screenshots

(Upload 3 to 5 high-quality screenshots (at least 1280x800px) of your app in PNG format.)

image
image
image

I have tested my app on:

@louneskmt louneskmt marked this pull request as ready for review February 4, 2021 21:34
mayankchhabra
mayankchhabra previously approved these changes Feb 13, 2021
Copy link
Member

@mayankchhabra mayankchhabra left a comment

Choose a reason for hiding this comment

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

Really nice job on packaging LNbits, @louneskmt! Works flawlessly.

@louneskmt
Copy link
Contributor Author

@mayankchhabra
Copy link
Member

mayankchhabra commented Feb 13, 2021

cc @arcbtc: LNbits looks ready to be shipped in the next Umbrel release! Can you please quickly eyeball this once?

Also, everything in the PR description above is what the user will see in the LNbits app description on the Umbrel App Store, along with the app icon and gallery images here (getumbrel/umbrel-apps-gallery#4). So feel free to suggest any edits if you'd like.

# App
LNBITS_SITE_TITLE: "LNbits - Umbrel"
LNBITS_DEFAULT_WALLET_NAME: "LNbits wallet"
LNBITS_DISABLED_EXTENSIONS: "amilk"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this extension disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It allows an user to automatically claim sats from an LNURLw. For example, a LNURL to claim 1000 sats every minute. I found this pretty unfair as the delay is here to allow more people to claim sats (e.g. a test LNURL in a Bitcoin bar, for newbies to discover Lightning) and not just have a single person claiming the whole balance.

If necessary I can enable it.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmn, I agree that doesn't sound like very nice behaviour but I'm not sure it should be our decision to blacklist it.

It's not doing anything bad by default, right? A user would have to manually enable it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A user would have to manually enable it?

Each extension has to be activated from the extensions page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But of the extension is disabled in this env var, it cannot be installed from the web page. So yeah, if it shouldn't be our decision to blacklist it, we should remove this line.

Copy link

Choose a reason for hiding this comment

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

Keep it disabled until we manage to improve it for future LNbits release

LNBITS_SITE_TITLE: "LNbits - Umbrel"
LNBITS_DEFAULT_WALLET_NAME: "LNbits wallet"
LNBITS_DISABLED_EXTENSIONS: "amilk"
LNBITS_ALLOWED_USERS: ""
Copy link
Member

Choose a reason for hiding this comment

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

Why is this empty?

Are there any docs for these settings? I had a quick look at the LNbits repo but didn't find any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukechilds
Copy link
Member

lukechilds commented Feb 13, 2021

@louneskmt
Copy link
Contributor Author

Is the app being built in development mode?

https://github.com/louneskmt/docker-lnbits/blob/4767453412e8c70249940586fe5cddc0f0fbb396/Dockerfile#L10

I don't think so, this is the default value. Maybe LNbits needs Quark in dev mode, idk.

https://lnbits.org/guide/installation.html

LNBITS_DEFAULT_WALLET_NAME: "LNbits wallet"
LNBITS_DISABLED_EXTENSIONS: "amilk"
LNBITS_ALLOWED_USERS: ""
LNBITS_FORCE_HTTPS: "true"
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what this option does?

It doesn't appear to be using HTTPS at all for me.

Copy link
Contributor Author

@louneskmt louneskmt Feb 14, 2021

Choose a reason for hiding this comment

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

Not really. I think we can get ride of these 3 lines (disabled extensions, force http and allowed users).

Copy link
Member

@lukechilds lukechilds left a comment

Choose a reason for hiding this comment

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

Looking good, nice job!

@lukechilds
Copy link
Member

No point blocking the merge for this but the Docker image is very large:

$ docker images louneskmt/lnbits:v0.2.0
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
louneskmt/lnbits    v0.2.0              27290287067a        6 weeks ago         412MB

The final image is including all of the build dependencies. It could probably be reduced in size significantly by using multistage builds and only including runtime deps in the resulting image.

It also isn't checksumming any external files it pulls in, which can make the images non-deterministic and hard to verify.

See https://github.com/getumbrel/umbrel/tree/master/apps#a-good-dockerfile for how to resolve this.

@lukechilds lukechilds closed this Feb 14, 2021
@lukechilds lukechilds reopened this Feb 14, 2021
@lukechilds lukechilds merged commit 09bfeef into getumbrel:master Feb 14, 2021
@louneskmt
Copy link
Contributor Author

Thanks, will try to improve this for the next version.

The dojo-whirlpool image is also 600 MB, but I used their Dockerfile. Maybe I will modify it to make a smaller final image.

@arcbtc
Copy link

arcbtc commented Feb 14, 2021

Wow, ace work:

  • Ditch "Developer website" as we are not really maintaining .org, directing folks to github repo is best
  • Add https://t.me/lnbits to support links
  • Keep amilk disabled, its too buggy/resource intensive to have easy access to
  • For one line description, it would be nice to get the extensions in there "Multi-user and wallet system for Lightning Network, with extension system for adding additional functionality such as LNURL"

@arcbtc
Copy link

arcbtc commented Feb 14, 2021

Only if I am not too late that is!

@mayankchhabra
Copy link
Member

Only if I am not too late that is!

You're in luck, we just realized that it's Valentines so we're delaying the release by a day lol.

The changes sound good. Regarding the one-line description, there's a 50 character limit on it. I know you can't really fit in the the entire description in 50 characters, but think of it more like a tagline for LNbits, rather than a description. Here's where it shows up in the app store:

Screen Shot 2021-02-14 at 4 52 20 PM

Currently it's set to "Multi-user wallet management system", but let us know if you'd like to change it to something else.

@arcbtc
Copy link

arcbtc commented Feb 14, 2021

Oh yeah, that's nice 👍, leave it like that

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.

None yet

4 participants