db: introduce sqldbv2 scaffolding#235
Conversation
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new database layer for Faraday, supporting both SQLite and PostgreSQL. Key changes include the integration of sqlc for generating Go models from SQL schemas and golang-migrate for database migrations. The main Faraday daemon has been significantly refactored to encapsulate RPC server setup, macaroon service management, and lnd client connections, leading to a simplified frdrpcserver package. Supporting changes include new Makefile targets, a sqlc.yaml configuration, and updated dependencies. Review comments primarily address incorrect usage of btclog.Logger methods (Infof, Errorf, Error with format strings) which should be corrected to Info or Error without format strings. Further feedback suggests improving code clarity and maintainability by making DefaultPostgresFixtureLifetime a constant, updating an outdated comment regarding the Postgres version, adding comments for temporary go.mod replace directives, and exploring automation for LatestMigrationVersion to reduce manual errors.
| // NOTE: This MUST be updated when a new migration is added. | ||
| LatestMigrationVersion = 1 |
There was a problem hiding this comment.
The comment NOTE: This MUST be updated when a new migration is added. indicates a manual step that is prone to human error. Consider implementing a mechanism to automatically derive or validate the LatestMigrationVersion during the build or test process to prevent inconsistencies.
There was a problem hiding this comment.
Will add a test in a later PR that checks this.
bitromortac
force-pushed
the
2603-fwd-prep-6
branch
from
ed4a3ab to
c30afe7
Compare
|
Updated to latest sqldbv2 in lnd side branch. |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Nice, this will definitely be useful. Leaving a few comments below 😃
| @$(call print, "Generating sql models and queries in Go") | ||
| ./scripts/gen_sqlc_docker.sh | ||
|
|
||
| sqlc-check: sqlc |
There was a problem hiding this comment.
Should add this check to the CI workflow.
There was a problem hiding this comment.
Right, I'll add that when we have the migrations since it would fail at the moment: sed: can't read db/sqlc/migrations/*.up.sql: No such file or directory
bitromortac
force-pushed
the
2603-fwd-prep-6
branch
from
c30afe7 to
0196bf3
Compare
bitromortac
force-pushed
the
2603-fwd-prep-6
branch
from
0196bf3 to
cb204ae
Compare
|
Thanks for the great review @ViktorT-11, I could remove a lot of code 🎉 |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks for the changes, LGTM 🔥!
bitromortac
merged commit 69a4a32
into
lightninglabs:faraday-forwarding-ability
3 participants
Based on #234.
This PR introduces a SQL database to Faraday, which will later be used to track channel state changes.
Uses the exact sqldbv2 version of https://github.com/lightninglabs/lightning-terminal/tree/sql-migration-base and the custom migration library. The scaffolding was copied from this (already reviewed) branch, which should make it easy to compare against.
Corresponding lnd PR: lightningnetwork/lnd#10175