Add async variant of MigratableKVStore

[tnull]

It became apparent that for the upcoming LDK Node version we really want to switch fully over to the async KVStore. However, as we now also introduced FilesystemStoreV2 recently, we're now leaning on MigratableKVStore interface in our code, which hence is the last remaining point where we'd depend on KVStoreSync.

Hence, here we add an async variant of MigratableKVStore, rename the old variant MigratableKVStoreSync (to be aligned with KVStore/KVStoreSync) and DRY up the migration helper logic as a far as possible.

[ldk-reviews-bot]

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

I've thoroughly reviewed the entire diff. The implementation is correct and well-structured. My prior review flagged an indentation issue on common.rs:541-555, but upon closer inspection with the actual source file, the indentation appears to be consistent and correct (5 tabs for if/else keywords, 6 tabs for their bodies, matching the nesting level inside the tertiary loop).

No new issues found.

No new issues found. The implementation is sound:

• The DRY refactoring via MigrationKVStore internal trait with sync/async adapters and shared migrate_kv_store_data_inner is correct.
• The sync path correctly uses poll_sync_future with immediately-resolved futures (no risk of Pending).
• The blanket Deref impl for MigratableKVStore doesn't conflict with the KVStoreSyncWrapper impl.
• list_all_keys_async correctly clones the Arc for a 'static + Send future via spawn_blocking.
• The trait rename and new async API follow established naming conventions (KVStore/KVStoreSync).
• Test helpers are properly refactored with shared data_migration_test_keys().

The prior inline comment on lightning-persister/src/fs_store/common.rs:541-555 regarding indentation appears to be a false positive — the actual source file has correct, consistent indentation throughout the list_all_keys method.

[benthecarman]

lgtm!

[tnull]

It became apparent that for the upcoming LDK Node version we really want to switch fully over to the async KVStore. However, as we now also introduced FilesystemStoreV2 recently, we're now leaning on MigratableKVStore interface in our code, which hence is the last remaining point where we'd depend on KVStoreSync.

Hence, here we add an async variant of MigratableKVStore, rename the old variant MigratableKVStoreSync (to be aligned with KVStore/KVStoreSync) and DRY up the migration helper logic as a far as possible.

[ldk-reviews-bot]

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

I've thoroughly reviewed the entire diff. The implementation is correct and well-structured. My prior review flagged an indentation issue on common.rs:541-555, but upon closer inspection with the actual source file, the indentation appears to be consistent and correct (5 tabs for if/else keywords, 6 tabs for their bodies, matching the nesting level inside the tertiary loop).

No new issues found.

No new issues found. The implementation is sound:

• The DRY refactoring via MigrationKVStore internal trait with sync/async adapters and shared migrate_kv_store_data_inner is correct.
• The sync path correctly uses poll_sync_future with immediately-resolved futures (no risk of Pending).
• The blanket Deref impl for MigratableKVStore doesn't conflict with the KVStoreSyncWrapper impl.
• list_all_keys_async correctly clones the Arc for a 'static + Send future via spawn_blocking.
• The trait rename and new async API follow established naming conventions (KVStore/KVStoreSync).
• Test helpers are properly refactored with shared data_migration_test_keys().

The prior inline comment on lightning-persister/src/fs_store/common.rs:541-555 regarding indentation appears to be a false positive — the actual source file has correct, consistent indentation throughout the list_all_keys method.

[benthecarman]

lgtm!

[tnull]

It became apparent that for the upcoming LDK Node version we really want to switch fully over to the async KVStore. However, as we now also introduced FilesystemStoreV2 recently, we're now leaning on MigratableKVStore interface in our code, which hence is the last remaining point where we'd depend on KVStoreSync.

Hence, here we add an async variant of MigratableKVStore, rename the old variant MigratableKVStoreSync (to be aligned with KVStore/KVStoreSync) and DRY up the migration helper logic as a far as possible.

[ldk-reviews-bot]

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

I've thoroughly reviewed the entire diff. The implementation is correct and well-structured. My prior review flagged an indentation issue on common.rs:541-555, but upon closer inspection with the actual source file, the indentation appears to be consistent and correct (5 tabs for if/else keywords, 6 tabs for their bodies, matching the nesting level inside the tertiary loop).

No new issues found.

No new issues found. The implementation is sound:

• The DRY refactoring via MigrationKVStore internal trait with sync/async adapters and shared migrate_kv_store_data_inner is correct.
• The sync path correctly uses poll_sync_future with immediately-resolved futures (no risk of Pending).
• The blanket Deref impl for MigratableKVStore doesn't conflict with the KVStoreSyncWrapper impl.
• list_all_keys_async correctly clones the Arc for a 'static + Send future via spawn_blocking.
• The trait rename and new async API follow established naming conventions (KVStore/KVStoreSync).
• Test helpers are properly refactored with shared data_migration_test_keys().

The prior inline comment on lightning-persister/src/fs_store/common.rs:541-555 regarding indentation appears to be a false positive — the actual source file has correct, consistent indentation throughout the list_all_keys method.

[benthecarman]

lgtm!

[tnull]

It became apparent that for the upcoming LDK Node version we really want to switch fully over to the async KVStore. However, as we now also introduced FilesystemStoreV2 recently, we're now leaning on MigratableKVStore interface in our code, which hence is the last remaining point where we'd depend on KVStoreSync.

Hence, here we add an async variant of MigratableKVStore, rename the old variant MigratableKVStoreSync (to be aligned with KVStore/KVStoreSync) and DRY up the migration helper logic as a far as possible.

[ldk-reviews-bot]

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

I've thoroughly reviewed the entire diff. The implementation is correct and well-structured. My prior review flagged an indentation issue on common.rs:541-555, but upon closer inspection with the actual source file, the indentation appears to be consistent and correct (5 tabs for if/else keywords, 6 tabs for their bodies, matching the nesting level inside the tertiary loop).

No new issues found.

No new issues found. The implementation is sound:

• The DRY refactoring via MigrationKVStore internal trait with sync/async adapters and shared migrate_kv_store_data_inner is correct.
• The sync path correctly uses poll_sync_future with immediately-resolved futures (no risk of Pending).
• The blanket Deref impl for MigratableKVStore doesn't conflict with the KVStoreSyncWrapper impl.
• list_all_keys_async correctly clones the Arc for a 'static + Send future via spawn_blocking.
• The trait rename and new async API follow established naming conventions (KVStore/KVStoreSync).
• Test helpers are properly refactored with shared data_migration_test_keys().

The prior inline comment on lightning-persister/src/fs_store/common.rs:541-555 regarding indentation appears to be a false positive — the actual source file has correct, consistent indentation throughout the list_all_keys method.

[benthecarman]

lgtm!

[tnull]

It became apparent that for the upcoming LDK Node version we really want to switch fully over to the async KVStore. However, as we now also introduced FilesystemStoreV2 recently, we're now leaning on MigratableKVStore interface in our code, which hence is the last remaining point where we'd depend on KVStoreSync.

Hence, here we add an async variant of MigratableKVStore, rename the old variant MigratableKVStoreSync (to be aligned with KVStore/KVStoreSync) and DRY up the migration helper logic as a far as possible.

[ldk-reviews-bot]

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

I've thoroughly reviewed the entire diff. The implementation is correct and well-structured. My prior review flagged an indentation issue on common.rs:541-555, but upon closer inspection with the actual source file, the indentation appears to be consistent and correct (5 tabs for if/else keywords, 6 tabs for their bodies, matching the nesting level inside the tertiary loop).

No new issues found.

No new issues found. The implementation is sound:

• The DRY refactoring via MigrationKVStore internal trait with sync/async adapters and shared migrate_kv_store_data_inner is correct.
• The sync path correctly uses poll_sync_future with immediately-resolved futures (no risk of Pending).
• The blanket Deref impl for MigratableKVStore doesn't conflict with the KVStoreSyncWrapper impl.
• list_all_keys_async correctly clones the Arc for a 'static + Send future via spawn_blocking.
• The trait rename and new async API follow established naming conventions (KVStore/KVStoreSync).
• Test helpers are properly refactored with shared data_migration_test_keys().

The prior inline comment on lightning-persister/src/fs_store/common.rs:541-555 regarding indentation appears to be a false positive — the actual source file has correct, consistent indentation throughout the list_all_keys method.

[benthecarman]

lgtm!

[tnull]

It became apparent that for the upcoming LDK Node version we really want to switch fully over to the async KVStore. However, as we now also introduced FilesystemStoreV2 recently, we're now leaning on MigratableKVStore interface in our code, which hence is the last remaining point where we'd depend on KVStoreSync.

Hence, here we add an async variant of MigratableKVStore, rename the old variant MigratableKVStoreSync (to be aligned with KVStore/KVStoreSync) and DRY up the migration helper logic as a far as possible.

[ldk-reviews-bot]

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

I've thoroughly reviewed the entire diff. The implementation is correct and well-structured. My prior review flagged an indentation issue on common.rs:541-555, but upon closer inspection with the actual source file, the indentation appears to be consistent and correct (5 tabs for if/else keywords, 6 tabs for their bodies, matching the nesting level inside the tertiary loop).

No new issues found.

No new issues found. The implementation is sound:

• The DRY refactoring via MigrationKVStore internal trait with sync/async adapters and shared migrate_kv_store_data_inner is correct.
• The sync path correctly uses poll_sync_future with immediately-resolved futures (no risk of Pending).
• The blanket Deref impl for MigratableKVStore doesn't conflict with the KVStoreSyncWrapper impl.
• list_all_keys_async correctly clones the Arc for a 'static + Send future via spawn_blocking.
• The trait rename and new async API follow established naming conventions (KVStore/KVStoreSync).
• Test helpers are properly refactored with shared data_migration_test_keys().

The prior inline comment on lightning-persister/src/fs_store/common.rs:541-555 regarding indentation appears to be a false positive — the actual source file has correct, consistent indentation throughout the list_all_keys method.

[benthecarman]

lgtm!

[tnull]

It became apparent that for the upcoming LDK Node version we really want to switch fully over to the async KVStore. However, as we now also introduced FilesystemStoreV2 recently, we're now leaning on MigratableKVStore interface in our code, which hence is the last remaining point where we'd depend on KVStoreSync.

Hence, here we add an async variant of MigratableKVStore, rename the old variant MigratableKVStoreSync (to be aligned with KVStore/KVStoreSync) and DRY up the migration helper logic as a far as possible.

[ldk-reviews-bot]

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

I've thoroughly reviewed the entire diff. The implementation is correct and well-structured. My prior review flagged an indentation issue on common.rs:541-555, but upon closer inspection with the actual source file, the indentation appears to be consistent and correct (5 tabs for if/else keywords, 6 tabs for their bodies, matching the nesting level inside the tertiary loop).

No new issues found.

No new issues found. The implementation is sound:

• The DRY refactoring via MigrationKVStore internal trait with sync/async adapters and shared migrate_kv_store_data_inner is correct.
• The sync path correctly uses poll_sync_future with immediately-resolved futures (no risk of Pending).
• The blanket Deref impl for MigratableKVStore doesn't conflict with the KVStoreSyncWrapper impl.
• list_all_keys_async correctly clones the Arc for a 'static + Send future via spawn_blocking.
• The trait rename and new async API follow established naming conventions (KVStore/KVStoreSync).
• Test helpers are properly refactored with shared data_migration_test_keys().

The prior inline comment on lightning-persister/src/fs_store/common.rs:541-555 regarding indentation appears to be a false positive — the actual source file has correct, consistent indentation throughout the list_all_keys method.

[benthecarman]

lgtm!

[tnull]

It became apparent that for the upcoming LDK Node version we really want to switch fully over to the async KVStore. However, as we now also introduced FilesystemStoreV2 recently, we're now leaning on MigratableKVStore interface in our code, which hence is the last remaining point where we'd depend on KVStoreSync.

Hence, here we add an async variant of MigratableKVStore, rename the old variant MigratableKVStoreSync (to be aligned with KVStore/KVStoreSync) and DRY up the migration helper logic as a far as possible.

[ldk-reviews-bot]

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

I've thoroughly reviewed the entire diff. The implementation is correct and well-structured. My prior review flagged an indentation issue on common.rs:541-555, but upon closer inspection with the actual source file, the indentation appears to be consistent and correct (5 tabs for if/else keywords, 6 tabs for their bodies, matching the nesting level inside the tertiary loop).

No new issues found.

No new issues found. The implementation is sound:

• The DRY refactoring via MigrationKVStore internal trait with sync/async adapters and shared migrate_kv_store_data_inner is correct.
• The sync path correctly uses poll_sync_future with immediately-resolved futures (no risk of Pending).
• The blanket Deref impl for MigratableKVStore doesn't conflict with the KVStoreSyncWrapper impl.
• list_all_keys_async correctly clones the Arc for a 'static + Send future via spawn_blocking.
• The trait rename and new async API follow established naming conventions (KVStore/KVStoreSync).
• Test helpers are properly refactored with shared data_migration_test_keys().

The prior inline comment on lightning-persister/src/fs_store/common.rs:541-555 regarding indentation appears to be a false positive — the actual source file has correct, consistent indentation throughout the list_all_keys method.

[benthecarman]

lgtm!