Conversation
zoedberg
left a comment
There was a problem hiding this comment.
This is a quick review, will do a more detailed one after the following points have been addressed:
-
Why did you put the entities in an
entitydirectory at the project root? If there's no particular reason please generate the entities in asrc/database/directory. Please also check the structure we used inrgb-lib, if possible I would appreciate if we could use the same naming/structure. -
Please create a single
init_dbmigration, it seems overkill to define the DB with multiple separate migrations. -
Please avoid async-std since it's deprecated.
-
Please replace the auto-generated README in the migration crate with the one in rgb-lib, I think you can copy that and make only small changes.
-
Please do not refactor the test code, the PR should make only the changes necessary for its scope.
-
Please drop the
src/test/revoked_token_persistence.rsfile and just add some checks to the already existingauthenticationtest. -
I see no DB transaction usage, is this because there are no cases where they are required?
-
Please explain the purpose of
SeaOrmKvStore. -
Please explain or revert the changes to
.gitignore. -
Please use the latest version of sea-orm.
|
All comments have been addressed, here some comments on the sepecific points:
In short; no, there are no cases where they are required directly. RGB transactions are stored in rgb-lib's database and lightning payments are stored via LDK's KVStore (now in
SeaOrmKvStore serves the same purpose as ldk-node's By default LDK uses filesystem storage, but ldk-node moved to SQLite for better reliability. We follow the same pattern here. Since RLN now uses sea-orm,
Changed to
Tried changing to tokio runtime but tests are not even running, there is a deeper issue involved. Let me know your thoughts on this I'm not so experienced with async rust and any help is greatly appreciated.
Bumped version to 1.1.19. I see there is a new version 2.x.x but I can only find release candidates and this version introduces breaking changes on the API. I'd say staying in 1.1.19 is safer for now until a stable v2 is realeased. I'm happy to explore the upgrade to v2 when the time comes. It introduces a sync crate that could be a good fit but currently only supports rusqlite afaics. sea-orm-sync |
9942266 to
6981244
Compare
|
Re-running the nightly build on windows should solve the failing check. I already experienced this before, seems like a network error while fetching nightly rust. https://github.com/RGB-Tools/rgb-lightning-node/actions/runs/21513442756/job/62004512971?pr=92 |
|
I just pushed the tokio runtime integration in a separate commit. Since I'm not sure this is the right approach I'll wait for the review to merge the commits into a single one if this is ok. See implementation of runtime and comments here https://github.com/dcorral/rgb-lightning-node/blob/20a811ecdaad8b4cf21fcb1c5f03c512a10a2b73/src/runtime.rs With this all comments have been addressed. 🙏 UpdateI realized the implementation was using a different rbg-lib version that is already using tokio runtime. |
20a811e to
c81e5b3
Compare
Summary
Replaces file-based persistence with SQLite database using sea-orm ORM.
Changes
New database tables:
mnemonic- encrypted mnemonic storagekv_store- LDK persistence (channel manager, payments, swaps, network graph, scorer)config- configuration key/value pairs (indexer_url, proxy_url, etc.)revoked_token- revoked auth token IDschannel_peer- channel peer pubkey/address mappingMigrated to database:
Architecture:
DatabaseConnectionviaArcSeaOrmKvStoreimplements rust-lightning'sKVStoreSynctrait