Use minSendable and maxSendable values from LNURLp response to guide user input #1697

fanismichalakis · 2022-11-02T09:23:11Z

Describe the changes you have made in this PR

Use the minSendableand maxSendablefields in the LNURLp response to display better suggestions in the SatButtons component, and disable the "Confirm" button if the input amount is out of the [min, max] range.
The values suggested by SatsButtonstill default to 100, 1K, 5K and 10K (current behavior) if the minSendableand maxSendable are missing or if they are "standard" (eg min below 100 sats and max above 10k sats).

Link this PR to an issue [optional]

Fixes #1569

Type of change

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

Screenshots of the changes [optional]

How has this been tested?

Tested manually:

on web pages where minSendable and maxSendable values are "standard". For example: https://twitter.com/getAlby ;
on web pages where minSendable and maxSendable values are a bit more exotic. For example: https://fanismichalakis.fr, where the LN Markets Lightning Address requires a minSendable amount of 1000 sats ;
by changing the code in the LnurlPay component to pass arbitrary values to the SatButtonscomponent. For example: <SatButtons onClick={setValueSat} disabled={loadingConfirm} min={42000} max={44000} />
to assert that the same amount isn't suggested twice when the minSendable and maxSendable values are close to each other.

I also ran test e2e and unit tests:

all e2e passed, except 2 skipped
for unit tests: Test Suites: 32 failed, 14 passed, 46 total ; Tests: 21 passed, 21 total (which is exactly the same as when running tests on the master of Alby even before any change).

Checklist

My code follows the style guidelines of this project and performed a self-review of my own code
New and existing tests pass locally with my changes
I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

bitcoinuser · 2022-11-02T12:13:09Z

Hi, In my opinion would be better show only two buttons with the min and max values. Something like what Zebedee wallet does:

bitcoinuser · 2022-11-02T12:16:42Z

Or maybe show a message if the user type an amount less the minimum or more than maximum. Not only disable the confirm button.

github-actions · 2022-11-02T13:45:03Z

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.

This build is brought to you by: James Charlesworth (who recently dropped 100 sats):

Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

bitcoinuser · 2022-11-02T13:49:15Z

I think inside the amount field should have a text like: Send between x sats and y sats.
Obs. The amount field should have no prefilled amount.

fanismichalakis · 2022-11-02T14:11:13Z

Thanks @bitcoinuser for your comments!

Hi, In my opinion would be better show only two buttons with the min and max values. Something like what Zebedee wallet does

I guess the "best solution" depends on what people use Alby for. What I like about the 100, 1K, 5K et 10K suggested values that are in place right now is that it makes perfect sense (at least to me) for tipping, Value4Value, etc. That's why I went for amounts that stayed quite close to this values, while taking the minSendable and maxSendable values into account.

Or maybe show a message if the user type an amount less the minimum or more than maximum. Not only disable the confirm button.

I agree. I noticed that there already is a help text displayed when hovering on the input that does just that, although I think that it would be better to display it under the input field.

I think inside the amount field should have a text like: Send between x sats and y sats.
Obs. The amount field should have no prefilled amount.

I think I prefer a prefilled amount (makes less clicks and helps decision, hence reducing transaction friction). Like for the help text on hover, I decided to let it like it was and focus on the SatButtons component. Happy to discuss it further and see what people think! :)

reneaaron

Thanks for your contribution! I just tested the PR and noticed that the algorithm for creating the sats buttons creates some odd distribution for different min / max combinations.

I've prepared a list of LNURLs that we can use for testing here:
https://github.com/getAlby/lightning-browser-extension/wiki/%F0%9F%A4%A0-Alby-in-the-wild

I think we should aim for the following behaviour:

Minimum is never lower than min
Maximum is never higher than max
Have the steps between min & max distributed evenly (but rounded to a number that depends on the size of the available range)

reneaaron · 2022-11-05T09:12:00Z

src/app/components/SatButtons/index.tsx

 };

-function SatButtons({ onClick, disabled }: Props) {
+function roundThousands(number: number) {
+  return number < 1000 ? Math.round(number) : Math.floor(number / 1000) * 1000;


I think we need to round here but take into account whether this is the min / max value.

Let's say a service sets 1.4 as their minimum value, we would currently round (which would set the minimum to 1) which will then fail because it is lower than the min amount.

Probably it would make sense to add some unit tests for this behavior as well.

cc @escapedcat

Happy to help with unit-tests if wanted once this is fixed.

escapedcat · 2022-12-02T14:07:11Z

Hey @fanismichalakis , should we finish this up or do you want to? I assume you're busy and that's understandable. Happy to jump in here!

fanismichalakis · 2022-12-09T17:35:09Z

Hey @escapedcat, sorry for all the delays! Last few weeks have been a bit challenging indeed, but I'm still willing to finish up if it's okay on your side.

I'll have some time next week to work on this, and I have already thought of some ways to address the aforementioned issues.

Thanks and sorry again for the delay!

escapedcat · 2022-12-09T17:47:22Z

Alright, awesome. No worries. I just wanted to check in. Thanks for your reply!

escapedcat · 2022-12-21T13:29:58Z

src/app/components/SatButtons/index.tsx

+  return integralFunction(rounded / Math.pow(10, tens)) * Math.pow(10, tens);
+}
+
+function amountsArray(min: number, max: number) {


Looking at this is leaving afraid and confused :P
Not sure how others feel. Is there a nice way to makes this easier to digest?

bitcoinuser · 2023-03-11T14:48:05Z

Hi @reneaaron was this fixed by #2152?

If yes, I think this issue can be closed.

reneaaron · 2023-03-13T14:29:31Z

@fanismichalakis I think with #2152 we've found a elegant (and less complex) solution to this kind of issue. Thanks for your work on this issue. Although this PR hasn't been merged, it brought up some important improvements! 🙌

@bitcoinuser Yep, thanks for the hint!

fanismichalakis and others added 4 commits October 29, 2022 10:53

feat: use minSendable and maxSendable amounts for input suggestions

bf49652

Merge branch 'getAlby:master' into master

69481cc

feat: round sub-1k numbers and dedup suggestions when range is tight

d1daf68

fix: typo in comment

9884913

escapedcat requested review from lisabaut, bumi, reneaaron, escapedcat and im-adithya November 2, 2022 13:34

reneaaron requested changes Nov 6, 2022

View reviewed changes

bumi removed request for bumi and lisabaut November 11, 2022 11:42

bumi assigned escapedcat Nov 11, 2022

Merge branch 'getAlby:master' into master

88459bc

Merge remote-tracking branch 'upstream/master'

b542ecd

fanismichalakis and others added 2 commits December 17, 2022 09:14

Merge branch 'getAlby:master' into master

b61290d

fix: better rounding and amount suggestion

9efa98d

escapedcat reviewed Dec 21, 2022

View reviewed changes

Merge branch 'getAlby:master' into master

007ff0f

escapedcat mentioned this pull request Mar 1, 2023

feat: improve LNURL ux #2152

Merged

3 tasks

reneaaron closed this Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use minSendable and maxSendable values from LNURLp response to guide user input #1697

Use minSendable and maxSendable values from LNURLp response to guide user input #1697

fanismichalakis commented Nov 2, 2022

bitcoinuser commented Nov 2, 2022

bitcoinuser commented Nov 2, 2022 •

edited

Loading

github-actions bot commented Nov 2, 2022 •

edited

Loading

bitcoinuser commented Nov 2, 2022 •

edited

Loading

fanismichalakis commented Nov 2, 2022

reneaaron left a comment

reneaaron Nov 5, 2022

reneaaron Nov 6, 2022

escapedcat Nov 6, 2022

escapedcat commented Dec 2, 2022

fanismichalakis commented Dec 9, 2022

escapedcat commented Dec 9, 2022 •

edited

Loading

escapedcat Dec 21, 2022

bitcoinuser commented Mar 11, 2023

reneaaron commented Mar 13, 2023

Use minSendable and maxSendable values from LNURLp response to guide user input #1697

Use minSendable and maxSendable values from LNURLp response to guide user input #1697

Conversation

fanismichalakis commented Nov 2, 2022

Describe the changes you have made in this PR

Link this PR to an issue [optional]

Type of change

Screenshots of the changes [optional]

How has this been tested?

Checklist

bitcoinuser commented Nov 2, 2022

bitcoinuser commented Nov 2, 2022 • edited Loading

github-actions bot commented Nov 2, 2022 • edited Loading

bitcoinuser commented Nov 2, 2022 • edited Loading

fanismichalakis commented Nov 2, 2022

reneaaron left a comment

Choose a reason for hiding this comment

reneaaron Nov 5, 2022

Choose a reason for hiding this comment

reneaaron Nov 6, 2022

Choose a reason for hiding this comment

escapedcat Nov 6, 2022

Choose a reason for hiding this comment

escapedcat commented Dec 2, 2022

fanismichalakis commented Dec 9, 2022

escapedcat commented Dec 9, 2022 • edited Loading

escapedcat Dec 21, 2022

Choose a reason for hiding this comment

bitcoinuser commented Mar 11, 2023

reneaaron commented Mar 13, 2023

bitcoinuser commented Nov 2, 2022 •

edited

Loading

github-actions bot commented Nov 2, 2022 •

edited

Loading

bitcoinuser commented Nov 2, 2022 •

edited

Loading

escapedcat commented Dec 9, 2022 •

edited

Loading