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

Wallet logger interface, more (responsive) logs #1516

Draft
wants to merge 30 commits into
base: master
Choose a base branch
from
Draft

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Oct 24, 2024

Description

Close #1380

based on #1510, #1511, #1515

TODO:

  • wallet logger interface: replace addWalletLog with logger interface
  • responsive receive logs: don't refetch receive logs after save, poll during save
  • more (receive) logs
    incoming payments; amount of payments; especially info around errors
  • test various errors on unhappy paths

Video

Current status (a610b93):

2024-11-06.03-10-33.mp4

Additional Context

  • we currently only log wallet attached / wallet details updated and wallet enabled / wallet disabled for receives so if you only attach spending credentials, there's no such log. ideally, we would only log these once, no matter if send and/or recv was attached. but how without creating a mess? previously, we logged them separately but that was noisy.

  • I believe there is a bug where we don't store the invoice description of unwrapped invoices since it's empty in the database even though the stored bolt11 does contain a description (see missing invoice description in last receiver log in video).

Checklist

Are your changes backwards compatible? Please answer below:

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

For frontend changes: Tested on mobile? Please answer below:

Did you introduce any new environment variables? If so, call them out explicitly here:

@ekzyis ekzyis added enhancement improvements to existing features wallets labels Oct 24, 2024
@ekzyis ekzyis marked this pull request as draft October 24, 2024 23:19
@ekzyis ekzyis changed the title Wallet logger interface, more and more responsive receive logs, Wallet logger interface, more (responsive) logs Oct 24, 2024
@ekzyis ekzyis force-pushed the wallet-logs branch 3 times, most recently from 6b2a906 to 6161c48 Compare October 31, 2024 12:03
@ekzyis
Copy link
Member Author

ekzyis commented Oct 31, 2024

Current status: 2cb3342

  • on successful autowithdrawal:
1m	[lnbits]	OK	↙ payment received: 1,000,000 msats
1m	[lnbits]	INFO	created invoice for 1,000,000 msats
1m	[lnbits]	INFO	↙ incoming payment: 1,000,000 msats
  • if wallet failed to create invoice:
6s	[lnbits]	ERROR	failed to create invoice: test error
6s	[lnbits]	INFO	↙ incoming payment: 1,000,000 msats
  • if wallet does not support msats:
4m	[lnbits]	OK	↙ payment received: 9,000 msats
4m	[lnbits]	WARN	wallet does not support msats
4m	[lnbits]	INFO	created invoice for 9,000 msats
4m	[lnbits]	INFO	↙ incoming payment: 9,900 msats
  • if withdrawal itself failed:
1m	[lnbits]	ERROR	withdrawal failed: no route found
1m	[lnbits]	INFO	created invoice for 1,000,000 msats
1m	[lnbits]	INFO	↙ incoming payment: 1,000,000 msats

TODO:

  • p2p zap logs on error and success
  • error handling (HTTP vs JSON)
  • rich log: show details of logs like payment hash, link to invoice, info how to fix etc. when clicked/uncollapsed (?)
  • log wallet enabled / wallet details updated only once but also show it if only send is configured ... how ideally?

@ekzyis ekzyis force-pushed the wallet-logs branch 2 times, most recently from 2cb3342 to 7a24ffb Compare November 5, 2024 00:28
@ekzyis
Copy link
Member Author

ekzyis commented Nov 5, 2024

How "rich log" looks so far:

2024-11-05.04-39-23.mp4

(not really rich yet since it's just text, no links to anywhere)

I am not super happy with + and - as indicators that the log can be expanded / collapsed but the SVGs that we use in ContextAwareToggle don't align well in the table. Same for these unicode arrows: 🢓, 🢒 (as you can see the arrow-down is not vertically aligned)

* remove unused logNav class
* rename classes
* the code assignment was broken anyway
* we already log withdrawal errors using .catch on payViaPaymentRequest
@ekzyis
Copy link
Member Author

ekzyis commented Nov 6, 2024

Current status (a610b93):

2024-11-06.03-10-33.mp4

I believe there is a bug where we don't store the invoice description since it's empty in the database even though the stored bolt11 does contain a description (see missing invoice description in last receiver log).

// server implementation of wallet logger interface on client
const log = (level) => async (message, context = {}) => {
try {
await models.walletLog.create({
Copy link
Member Author

Choose a reason for hiding this comment

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

no console.log here since I think wallets logs inside the server logs would be more noise than signal

testCreateInvoice:
walletDef.testCreateInvoice && validateLightning && canReceive({ def: walletDef, config: data })
? (data) => walletDef.testCreateInvoice(data, { me, models })
? (data) => walletDef.testCreateInvoice(data, { logger, me, models })
Copy link
Member Author

Choose a reason for hiding this comment

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

we pass logger to testCreateInvoice of the wallets but we only use the logger for NWC. So far, I saw no need to add wallet-specific logging to the other wallets.

export function assertContentTypeJson (res) {
const contentType = res.headers.get('content-type')
if (!contentType || !contentType.includes('application/json')) {
throw new Error(`POST ${res.url}: ${res.status} ${res.statusText}`)
Copy link
Member Author

Choose a reason for hiding this comment

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

no more gigantic error messages about how we couldn't parse the HTML body as JSON when a reverse proxy throws a gateway timeout

@@ -26,7 +26,7 @@ async function disconnect (lnc, logger) {
}, 50)
logger.info('disconnected')
} catch (err) {
logger.error('failed to disconnect from lnc', err)
logger.error('failed to disconnect from lnc: ' + err)
Copy link
Member Author

Choose a reason for hiding this comment

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

logger no longer supports a variable number of arguments because second argument is now context

@@ -294,7 +292,7 @@ export async function checkWithdrawal ({ data: { hash, withdrawal, invoice }, bo

const fee = Number(wdrwl.payment.fee_mtokens)
const paid = Number(wdrwl.payment.mtokens) - fee
const [{ confirm_withdrawl: code }] = await serialize([
const [[{ confirm_withdrawl: code }]] = await serialize([
Copy link
Member Author

Choose a reason for hiding this comment

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

for some reason the return type changed at some point

Comment on lines -310 to -313
const message = `autowithdrawal of ${
numWithUnits(msatsToSats(paid), { abbreviate: false })} with ${
numWithUnits(msatsToSats(fee), { abbreviate: false })} as fee`
await addWalletLog({ wallet: dbWdrwl.wallet, level: 'SUCCESS', message }, { models })
Copy link
Member Author

Choose a reason for hiding this comment

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

we don't need this because we log success/errors using .then and .catch on payViaPaymentRequest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvements to existing features wallets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Surface all relevant payment problems/successes in wallet logs
1 participant