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

[WIP] sqldb: InvoiceDB interface implementation #7357

Conversation

positiveblue
Copy link
Contributor

  • We need to be careful with the zero value of some types that can be null: []myType{} vs var myVar []myType by default what I did is if it is nullable and the "empty" value has no meaning do not store it in the db. However, I set it to that value when unmarshaling data from the db.

  • In sqlc, different queries return different types, even if they have the same fields. For example GetAMPPaymentBySetID and GetAMPPaymentsByInvoiceIDRow are not the same type.

  • LookupInvoice does not load the custom records yet. They will be serialized in a similar way features are.

Currently the PR is organized with each commit implementing one of the InvoiceDB interface methods. The commit includes some tests (form the ones in channeldb/invoices_test.go) to ensure that we pass the current suite. Before promoting the draft I will split in one method per commit + one commit per functionality test so it is easier to check that we cover all the old functionalities.

NOTE: This PR is the 3 out of X PRs for fixing #6288
It builds on top of #7354, relevant commits from sqldb: implement AddInvoice

@saubyk saubyk added database Related to the database/storage of LND invoices sql labels Jan 26, 2023
@saubyk saubyk added this to the v0.16.0 milestone Jan 26, 2023
@saubyk saubyk modified the milestones: v0.16.0, v0.16.1 Feb 14, 2023
@Roasbeef Roasbeef requested a review from sputn1ck February 24, 2023 04:47
@saubyk saubyk modified the milestones: v0.16.1, v0.17.0 Mar 13, 2023
@positiveblue positiveblue force-pushed the invoice-sql-implementation branch 3 times, most recently from 7adfde6 to 68fdd28 Compare June 8, 2023 00:14
@positiveblue positiveblue force-pushed the invoice-sql-implementation branch 2 times, most recently from 146daec to 9474081 Compare June 10, 2023 00:12
@saubyk
Copy link
Collaborator

saubyk commented Jun 13, 2023

Concept Ack

@bhandras bhandras self-requested a review August 3, 2023 06:05
@bhandras
Copy link
Collaborator

bhandras commented Aug 3, 2023

Needs rebase. Ready for first look?

@saubyk saubyk modified the milestones: v0.17.0, v0.18.0 Aug 4, 2023
@positiveblue positiveblue force-pushed the invoice-sql-implementation branch from 9474081 to 9e3d112 Compare September 22, 2023 19:06
sqlc has now an official dockerhub organization (sqlc). The old org was
the personal dev account, and new images are not uploaded there anymore.

It looks like they formed a company (Riza, Inc) and are dedicating 100%
of their time to the project (which is great news!)

[sqlc v1.21.0](sqlc-dev/sqlc#2709) has a bug
generating unvalid go code (files with unused imports) so we use v1.20.0
By default `INTEGER` translates to 64 bits long int in sqlite. For
postgres we are translating `INTEGER PRIMARY KEY` to `SERIAL PRIMARY KEY`.
The last piece is that we use `uint64` in Go for many identifiers (ex:
`AddIndex` in Invoices).

Given that SQL does not have uint of any size, we will use
`BIGSERIAL PRIMARY KEY` in postgres, `INTEGER PRIMARY KEY` in sqlite
and parse it as uint64 in Go.

NOTE: sqlite creates a shadow rowid column for each row in a table. If
the primary key of that table is defined as `INTEGER PRIMARY KEY` the
column will be an alias for the rowid column. That IS NOT the case when
we use `BIGINT PRIMARY KEY`, even when both are int64 in sqlite. SQLite
does not have autoincrement for BIGINT, so we need to replace it for
INTEGER while making sure sqlc uses int64 for that field.
Reuse the test suite from the channeldb implementation
The test for this functionality has been updated to make sure that we do
not delete settled invoices with this method.
@bhandras
Copy link
Collaborator

bhandras commented Oct 2, 2023

Closing this due to takeover in #8052

@bhandras bhandras closed this Oct 2, 2023
@saubyk saubyk removed this from the v0.18.0 milestone Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Related to the database/storage of LND invoices sql
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants