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

Gifting #421

Merged
merged 5 commits into from
Oct 3, 2023
Merged

Gifting #421

merged 5 commits into from
Oct 3, 2023

Conversation

benthecarman
Copy link
Collaborator

Used for testing the gifting feature

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 14, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 97515a8
Status: ✅  Deploy successful!
Preview URL: https://b9ffc7f8.mutiny-web.pages.dev
Branch Preview URL: https://gifting.mutiny-web.pages.dev

View logs

@futurepaul futurepaul force-pushed the gifting branch 2 times, most recently from 0ccf359 to cc7263c Compare August 18, 2023 21:22
@futurepaul futurepaul force-pushed the gifting branch 6 times, most recently from 84cf96a to 6ccebb7 Compare August 22, 2023 18:27
@futurepaul
Copy link
Collaborator

futurepaul commented Aug 22, 2023

needs MutinyWallet/mutiny-node#711 merged and then this is ready for review

@futurepaul futurepaul changed the title Gifing demo Gifting Aug 22, 2023
@futurepaul
Copy link
Collaborator

things to test:
not enough funds on sender
too small amount for receiver (if it's a fresh wallet)
trying to redeem twice
loading from url
loading from scanner

@futurepaul
Copy link
Collaborator

rebase achieved, haven't re-tested yet. todo list here: #540

@benalleng
Copy link
Collaborator

When using the qr-scanner I am sent to this page
https://gifting.mutiny-web.pages.dev/gifting.mutiny-web.pages.dev

@futurepaul
Copy link
Collaborator

When using the qr-scanner I am sent to this page https://gifting.mutiny-web.pages.dev/gifting.mutiny-web.pages.dev

I think I fixed this, can't test right now though. (it was only a problem because I was looking for the string /gift and our staging url includes /gift because //gifting..

@futurepaul futurepaul marked this pull request as ready for review September 22, 2023 23:25
@TonyGiorgio
Copy link
Collaborator

This warning appears on the amount dialog for the gift amount editor. You should probably default to these types of warnings not being displayed so we quit running into it for anything besides the receive flow.

Screenshot from 2023-09-23 13-26-49

@TonyGiorgio
Copy link
Collaborator

  1. The lightning fee warning is appearing but it wasn't needed. I redeemed 1 gift for 10k sats which opened up my first channel + 100k sats. Then I went to redeem a gift of 50k sats, which it warned I needed another channel for. This was incorrect.
  2. Could the timeout warning be better here? In this case, I closed the browser on the gifter side but then redeemed on the receiver side. It should say that the gift is in progress becauase it sent the NWC message and it will be sent whenever the gifter comes back online.
  3. There's nothing on the gifter side warning them that they need to be online for the gift to be redeemed.

Screenshot from 2023-09-23 13-32-17

@TonyGiorgio
Copy link
Collaborator

In general it's feeling pretty solid! Just a few edge cases and smaller feature requests. Nice work!

@benalleng
Copy link
Collaborator

benalleng commented Oct 3, 2023

Tested on firefox mobile web

  • Not enough funds on sender error only happens on the receiver side (should probably check the available lightning balance at the time of gift creation)
  • below 50k sats shows an appropriate warning on sender and an error on receiver side
  • scanner and url both work as expected
  • redeeming for a second time returns a "Payment timed out." no specific error that it had been redeemed already

@TonyGiorgio
Copy link
Collaborator

Should this feature be available to anyone that is self hosting?

@TonyGiorgio
Copy link
Collaborator

I hardcoded URL on both the sending and receiving side and as long as these match up properly, android works great. Even opens in app and/or scans in app properly. So just detecting android and switching URL should work.

@benthecarman
Copy link
Collaborator Author

Should this feature be available to anyone that is self hosting?

yeah it should

@futurepaul
Copy link
Collaborator

okay I think warnings are well behaved now and have a better error for timeout

Screenshot 2023-10-03 at 12 57 39 PM Screenshot 2023-10-03 at 1 55 30 PM

also hardcoded urls

only comment I didn't get to is this one, didn't feel like it's 100% necessary

Not enough funds on sender error only happens on the receiver side (should probably check the available lightning balance at the time of gift creation)

Copy link
Collaborator Author

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

ACK

@TonyGiorgio TonyGiorgio merged commit 7a962c5 into master Oct 3, 2023
4 checks passed
@TonyGiorgio TonyGiorgio deleted the gifting branch October 3, 2023 19:59
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.

4 participants