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

Model more transaction history details #1111

Merged
merged 13 commits into from
Sep 4, 2023
Merged

Model more transaction history details #1111

merged 13 commits into from
Sep 4, 2023

Conversation

Restioson
Copy link
Contributor

@Restioson Restioson commented Aug 16, 2023

Addresses #872 by modelling more history details for different kinds of events (transactions, trades, fees, etc).

I don't think this PR should address the Figma mock beyond adding all the info that it has to the display and that should rather be done in a followup, but happy to change this if desired.

Questions:

  • What should we do about unpaid invoices in the database?

Related: #1103

@Restioson
Copy link
Contributor Author

Restioson commented Aug 23, 2023

With ec9b7de, I insert the invoice into the payments table upon creation. This is required to know the description once the payment has been claimed. However, it also leads to the payment just sticking around in the DB if it's unpaid. I'm not sure what we want to happen here from a UX perspective if the user creates an invoice and it isn't paid (expiry date & periodic sweep to remove? notify user?).

@Restioson Restioson requested review from luckysori and holzeis August 23, 2023 16:21
@Restioson Restioson marked this pull request as ready for review August 23, 2023 16:22
@Restioson
Copy link
Contributor Author

This is more-or-less ready, aside from the above comment

@holzeis
Copy link
Contributor

holzeis commented Aug 23, 2023

With ec9b7de, I insert the invoice into the payments table upon creation. This is required to know the description once the payment has been claimed. However, it also leads to the payment just sticking around in the DB if it's unpaid. I'm not sure what we want to happen here from a UX perspective if the user creates an invoice and it isn't paid (expiry date & periodic sweep to remove? notify user?).

I would remove it once it has expired. I think our invoices are already created with an expiry.

Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

LGTM, except for the preimage. I would leave that out completely :D

Can we get some screenshots too? 👀

crates/ln-dlc-node/src/lib.rs Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/node/invoice.rs Outdated Show resolved Hide resolved
mobile/lib/main.dart Outdated Show resolved Hide resolved
mobile/native/src/ln_dlc/mod.rs Show resolved Hide resolved
@Restioson
Copy link
Contributor Author

Can we get some screenshots too? 👀

I can make some, although it isn't pretty as this is just the modeling side of things and not the display

@luckysori
Copy link
Contributor

Can we get some screenshots too? 👀

I can make some, although it isn't pretty as this is just the modeling side of things and not the display

Fair enough, we can look at the screenshots once that's nicer.

@Restioson
Copy link
Contributor Author

Restioson commented Aug 25, 2023

I would remove it once it has expired. I think our invoices are already created with an expiry.

See these lines in native::ln_dlc::create_invoice:

node.inner.create_interceptable_invoice(
amount_sats,
0,
"Fund your 10101 wallet".to_string(),
final_route_hint_hop,
)
})

How should we interpret this? Apparently lightning (the crate) adds two hours (gleaned by reading the code):

https://github.com/lightningdevkit/rust-lightning/blob/3dffe54258a374d15571d4ec72f5faa02477b770/lightning/src/ln/inbound_payment.rs#L237-L243

I'm not sure if we can rely on the +2 hours, but maybe we just should?

Also, there is a default expiry time: (1 hour).

@Restioson
Copy link
Contributor Author

image
image
image
image

@luckysori some screenshots! Missing on chain because it's a pain 😆 but I can take one if we need. It requires restarting the app to show up.

@Restioson
Copy link
Contributor Author

Restioson commented Aug 25, 2023

I would remove it once it has expired. I think our invoices are already created with an expiry.

Actually, I'm not so sure of this. I think an expired invoice is semantically a failed payment (or maybe we can add a new HTLCStatus for this called expired), or something similar.

User stories for this (proposed) feature:

  • As a wallet user,
    • I want to check on the history of invoices I've sent
    • so I know if the transfer was paid, expired, or failed
  • As a 10101 sysadmin,
    • I want to check if any invoices from the coordinator are expiring unexpectedly
    • so I can monitor the health of the coordinator

If the user creates an invoice, sends it off, and it isn't paid, they should be able to go back and see that it wasn't paid instead of silently disappearing, imo.

@Restioson
Copy link
Contributor Author

Restioson commented Aug 25, 2023

I've pushed 720fc36 to implement the above feature, but it's untested as I still need to wait the hour for it to expire (I could make one expire on purpose but since it's Friday, I may as well wait till Monday 😉)

Similarly, 3892963 fixes #1140, but I'm not sure how to force a failed transaction.

@Restioson
Copy link
Contributor Author

Example of a failed payment

image

@Restioson
Copy link
Contributor Author

I think this is ready, pending review. That is to say, I am happy with it now.

@Restioson Restioson requested a review from luckysori August 28, 2023 14:54
@Restioson Restioson requested review from bonomat and holzeis August 29, 2023 19:49
@Restioson
Copy link
Contributor Author

Are we happy with this? If so, I can resolve conflicts for merge

Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Contributor

@bonomat bonomat left a comment

Choose a reason for hiding this comment

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

I guess we can make it look nice in a follow-up PR :) Thank you Cael.

@Restioson
Copy link
Contributor Author

Restioson commented Aug 30, 2023

I guess we can make it look nice in a follow-up PR :) Thank you Cael.

That's the idea - I wanted to stick to the backend side (hence 'model more') here and implement just the minimum amount of frontend to test it. I figured that the looking nice could be up to more debate.

I'll rebase this soon for merge

@Restioson
Copy link
Contributor Author

Ah, maybe I should have a crack at displaying the fees for lightning transactions

@klochowicz
Copy link
Contributor

klochowicz commented Sep 1, 2023

@Restioson could we try to merge this soon to avoid the need for rebasing? we can always keep improving in a follow-up, and these changes can be useful for others :)

@Restioson
Copy link
Contributor Author

@Restioson could we try to merge this soon to avoid the need for rebasing? we can always keep improving in a follow-up, and these changes can be useful for others :)

Sure, I'll do that today!

This is in preparation for adding more fields to different types of history items
Specifically, fee and confirmation time
Otherwise, the description is not kept.

However, if the payment is not paid, it will stay in the database. I'm not sure what we "should" do here. Add an expiry date and sweep to remove expired invoices?
The old name doesn't make sense in view of the current usage
@Restioson Restioson enabled auto-merge September 4, 2023 10:35
@Restioson Restioson added this pull request to the merge queue Sep 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 4, 2023
@Restioson Restioson added this pull request to the merge queue Sep 4, 2023
Merged via the queue into main with commit 6a9bb51 Sep 4, 2023
7 checks passed
@Restioson Restioson deleted the model-more-history branch September 4, 2023 13:15
@Restioson Restioson mentioned this pull request Sep 7, 2023
4 tasks
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.

Show failed transactions in transaction history
5 participants