-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feat: dynamic budgets #226
Conversation
…transactions service
…rors to NIP-47 response also reduce query duplication
) * fix: migrate existing tables to have autoincrementing primary keys * fix: recreate request and response event tables after apps * fix: remove null from request_events app_id * fix: do not crash when reloading app created page * chore: update pragma commands, add busy_timeout * fix: remove unnecessary migration of user_configs table * chore: update comment on autoincrement migration
* fix: db transaction should be passed as pointer * feat(lnd): subscribe for payments and invoices * chore: rearrange check for settled * chore: remove TODO * chore: retry on error and add select * chore: publish payment failed event * chore: use json logging * feat: add lnd notification types * chore: remove sleep
expires_at datetime, | ||
settled_at datetime, | ||
metadata text, | ||
self_payment boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably should have an index on
- app_id
- state
- type
- payment_hash
- created_at / settled_at?
need to check what queries we make, but I think we need something here.
for example for this a combined might be helpful
"app_id = ? AND type = ? AND (state = ? OR state = ?) AND created_at > ?"
app_id = ? AND type = ? AND state = ?
make sure type
is no reserved word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a reserved word. Do we need to add indexes now or would it be better to wait until we hit performance issues so we know which ones actually work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would you want to wait? what is an argument of not adding an index?
you know which queries you do and I am not sure if we can identify slow queries.
Migrate: func(tx *gorm.DB) error { | ||
|
||
if err := tx.Exec(` | ||
CREATE TABLE transactions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transactions is also often a reserved word - esp in the DB context this can lead to strange things. are we good there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works, but I also don't like the same terminology being used for 2 different things. I added a note to #299
} | ||
tx. | ||
Table("transactions"). | ||
Select("SUM(amount_msat + coalesce(fee_msat, 0) + coalesce(fee_reserve_msat, 0)) as sum"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need the coalesce
? isn't a fee_msat default of 0 ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently fee_msat is nullable, because we don't always know the fee (for some connectors) and it seems wrong for it to be zero (I think we chatted about this in the past)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually add zero in our wallet - imo such exceptions because some connectors will make the app complicated and now we have to do things like coalesce
because some connector misses a feature.
and in the next code this coalesce
or similar will be forgotten and just causes bugs.
TLDR:
Fixes #25
Fixes #52
Fixes #122
Partially addresses #284 (no multi-connection setup though)
Partially addresses #105
Breaking Changes
Payments
table is deleted as part of the migration.What this enables
Changes
Architecture
nwc_payment_received
andnwc_payment_sent
has been updated to send entire payment details to the event publisher, which the transactions service will consume.Database
type
incoming or outgoingstate
which can be used to ensure budgets are not exceededmetadata
JSON-encoded transaction metadatafull
orisolated
) - isolated balance is summed from that app's received and sent paymentsfull
orisolated
) - isolated only allows accessing that app's created invoicesisolated
booleanTODOs:
SubscribeInvoices
andSubscribeTransactions
RPC methods and ensure it works for keysend, OR change the method of checking unsettled payments to list all payments from LNClient for a certain period of time. -> implement notifications support in LNDDELETE FROM app_permissions WHERE app_id NOT IN (SELECT id FROM apps);
UI
Unit tests
Testing
To move to new issues: