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

normalized wallet logs #1826

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

normalized wallet logs #1826

wants to merge 4 commits into from

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Jan 18, 2025

Description

We want to normalize the wallet logs. They currently don't link to invoices or withdrawals via foreign keys but store all the data in the JSON context column.

However, afaict, we can't get rid of context entirely since either a) the data we want to show is unrelated to an invoice or b) the invoice was not created yet.

update: Decided to drop the context column anyway to keep it simple. If we really need some information, we can add it back or save it in a relational way.

update: Realized we would lose created invoice for X sats log messages. We log them when we just fetched a user invoice before they get wrapped. Since wrapping can fail, this would mean we would show an error without having shown that we created an invoice.

There are and will most likely be more cases like this so I decided to keep the context column.

So all this PR does is to add the link to the invoice/withdrawal if we already have an invoice/withdrawal id and resolve the context from this link instead of from the JSON column.

TODO:

  • resolve context fields required by frontend
  • refactor context in frontend? will completely remove client logs in the future and move them to server, too

Regression tests:

  • test wallet status derived from logs
  • test log messages still show same information in frontend

Additional Context

To not use the JSON column if not necessary, I now also resolve context.status and context.recv (needed for wallet status) from the existing information instead of saving it.

In a future PR, I will replace the logic to derive status with two status column on the Wallet table anyway.

Checklist

Are your changes backwards compatible? Please answer below:

maybe, but I didn't test and we don't care so we just truncate the table

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:

8. Tested successful/failed autowithdrawals, successful/failed p2p zaps (forward error, wrap error) and sending payments (even though sender logs were not changed at all)

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

n/a

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

no

@ekzyis ekzyis added the wallets label Jan 18, 2025
@ekzyis ekzyis marked this pull request as draft January 18, 2025 20:47
@ekzyis ekzyis force-pushed the normalized-wallet-logs branch 2 times, most recently from 891cdf0 to fa46bd3 Compare January 18, 2025 20:57
@ekzyis ekzyis force-pushed the normalized-wallet-logs branch from fa46bd3 to 05037d5 Compare February 11, 2025 17:05
@ekzyis ekzyis force-pushed the normalized-wallet-logs branch 3 times, most recently from 8c7e3c8 to c0a55b4 Compare March 14, 2025 20:45
@ekzyis ekzyis force-pushed the normalized-wallet-logs branch 4 times, most recently from fe3a2c3 to 9332b54 Compare March 20, 2025 00:18
@ekzyis ekzyis marked this pull request as ready for review March 20, 2025 01:54
@ekzyis ekzyis force-pushed the normalized-wallet-logs branch from 8778dae to 863282f Compare March 20, 2025 01:54
@ekzyis ekzyis requested a review from huumn March 20, 2025 01:54
Copy link
Member

@huumn huumn left a comment

Choose a reason for hiding this comment

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

QA for this went fine. As discussed in chat, I'd recommend refactoring this to not require 2N+1 DB dips where N = # log lines.

I'd also recommend thinking about preventing context from (in the future ... perhaps by accident) leaking sender details to receivers and receiver details to senders. Perhaps wallet logs for receiving should never have an invoiceId and for sending should never have a withdrawalId.

@ekzyis ekzyis force-pushed the normalized-wallet-logs branch from a0a9903 to 0d5c035 Compare March 21, 2025 00:57
@ekzyis
Copy link
Member Author

ekzyis commented Mar 21, 2025

I'd recommend refactoring this to not require 2N+1 DB dips where N = # log lines.

Done in d204a70

leaking sender details to receivers and receiver details to senders.

I realized I was actually indeed leaking a sender's invoice to the receiver. 👀 Fixed that in 257f48b. I now never return an invoice as the context for a receiver's log.

Perhaps wallet logs for receiving should never have an invoiceId and for sending should never have a withdrawalId.

Mhh, wouldn't we miss out on interesting relationships that way again?

But if we want to do this, we would just need to remove this and this line.

We could also completely drop the invoiceId column and add it back when we actually have sender logs on the server.

@ekzyis ekzyis force-pushed the normalized-wallet-logs branch from 0d5c035 to 257f48b Compare March 21, 2025 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants