refactor: add unified storage abstraction and SQLite backend support #270
Draft
quake wants to merge 39 commits intonervosnetwork:developfrom
Draft
refactor: add unified storage abstraction and SQLite backend support #270quake wants to merge 39 commits intonervosnetwork:developfrom
quake wants to merge 39 commits intonervosnetwork:developfrom
Conversation
This commit refactors the storage layer to reduce code duplication and adds SQLite as an alternative storage backend to RocksDB. Changes: - Add unified StorageIterator trait for all storage backends - Extract shared query/filter logic to service_helpers module - Implement SQLite storage backend with feature flag - Define LightClientStorage trait for future Service layer refactoring - Update RPC and WASM to use shared helper functions Benefits: - Eliminates ~210 lines of duplicate code between RPC and WASM - Supports three storage backends: RocksDB (default), SQLite (feature), IndexedDB (wasm) - All backends expose identical APIs via unified abstractions - Lays groundwork for complete Service layer refactoring Files added: - light-client-lib/src/service_helpers.rs (123 lines) - light-client-lib/src/storage/db/iterator.rs (107 lines) - light-client-lib/src/storage/db/sqlite.rs (948 lines) - light-client-lib/src/storage/storage_trait.rs (149 lines) Testing: - All three compilation targets pass (RocksDB, SQLite, WASM) - All clippy checks pass with --deny warnings Note: Complete Service layer refactoring with LightClientStorage trait implementation will be done in a follow-up PR.
- Implement LightClientStorage trait for RocksDB backend - Implement LightClientStorage trait for SQLite backend - Create new service_impl module with LightClientService struct - Implement get_cells() method in LightClientService - Add REFACTORING_PLAN.md documenting the refactoring progress This is part of the ongoing refactoring to eliminate code duplication between RPC and WASM implementations (~500 lines of duplicate logic). Benefits: - Single source of truth for business logic - Storage backend abstraction via LightClientStorage trait - Type-safe generic implementation over storage backends - Foundation for completing service layer refactoring
- Add get_transactions() method with grouped/ungrouped modes - Add get_cells_capacity() method to calculate total capacity - Both methods support full filtering (script, block range, etc.) - All business logic now unified in LightClientService This completes the core service layer implementation. Next step is to refactor RPC and WASM to use these unified methods, eliminating ~600 lines of duplicate code.
- Replace get_cells/get_transactions/get_cells_capacity with service calls - Delete 551 lines of duplicate business logic - Add thin wrapper layer that delegates to LightClientService - Net reduction: 530 lines This eliminates ~60% of the target duplicate code.
…htClientStorage for browser - Implement LightClientStorage trait for browser IndexedDB storage - Migrate get_cells/get_transactions/get_cells_capacity in WASM to use LightClientService - Delete 697 lines of duplicate business logic from WASM - Add 41 lines of thin wrapper code - Add 160 lines for LightClientStorage trait implementation - Net reduction: 513 lines from WASM module This completes the service layer refactoring, eliminating ~850 lines of duplicate code between RPC and WASM implementations.
…lter_map - Changed worker's collect_iterator to pass both key and value to filter_map closures - Updated filter_map signature from Fn(&[u8], ObjectStore) to Fn(&[u8], &[u8], ObjectStore) - Added FilterMapType enum in browser.rs to handle both Iterator (key+value) and IteratorKey (key only) commands - Fixed unused variable warning and added TODO comment about direction handling bug - This change enables the service layer to access both key and value without redundant storage lookups
…chain and transaction methods - Added LightClientChainService for operations requiring StorageWithChainData - Implemented transaction methods: send_transaction, get_transaction, fetch_transaction - Implemented chain/header methods: get_tip_header, get_genesis_block, get_header, fetch_header - Implemented estimate_cycles method - Migrated RPC implementation to use unified service methods - Migrated WASM implementation to use unified service methods - Eliminated ~294 lines of duplicate code between RPC and WASM - Added 270 lines of unified business logic in service layer - Net reduction: 24 lines, significant reduction in duplication
- Removed #[allow(unused_imports)] from storage/db/mod.rs (types are actually used) - Removed #[allow(clippy::type_complexity)] from service_helpers.rs - Introduced type aliases QueryOptions and FilterOptions for better readability - All builds pass without warnings
…k methods to service layer Add set_scripts, get_scripts, local_node_info, and get_peers to the unified service layer: - Add set_scripts and get_scripts to LightClientChainService - Both methods handle matched_blocks clearing and storage updates - Use blocking_write() for tokio::sync::RwLock access - Create new LightClientNetworkService for network operations - Implements local_node_info with configurable max_addrs - Implements get_peers with platform-specific Instant handling - Handles peer state, protocols, and connection info - Update RPC implementations to use service layer - Add consensus field to BlockFilterRpcImpl - Replace ~100 lines of duplicate code with service calls - NetRpc now uses LightClientNetworkService - Update WASM implementations to use service layer - Replace ~90 lines of duplicate code with service calls - Use Arc::clone for proper Arc<Peers> handling Impact: - Eliminated ~190 lines of duplicate code - Added 153 lines of unified service logic - Net reduction: ~37 lines - All script, network, chain, and cell operations now unified
Move service-related files into a dedicated service/ directory for better organization: Structure changes: - service.rs → service/types.rs (type definitions) - service_impl.rs → service/impls.rs (service implementations) - service_helpers.rs → service/helpers.rs (helper functions) - Added service/mod.rs to organize exports Benefits: - Better code organization with related files grouped together - Clear separation: types, implementations, and helpers - Cleaner imports from external modules - Follows Rust module organization best practices Module exports: - Public: All types, LightClientService, LightClientChainService, LightClientNetworkService - Internal: Helper functions only used by service implementations All builds pass without warnings: - ✅ Native RPC - ✅ Library - ✅ WASM
…s in RPC structs Optimize service usage by pre-creating service instances instead of creating them on every method call: RPC layer changes: - Store LightClientService and LightClientChainService in BlockFilterRpcImpl - Store LightClientChainService in TransactionRpcImpl and ChainRpcImpl - Store LightClientNetworkService in NetRpcImpl - Create all services once during initialization - Share single LightClientChainService instance across multiple RPC impls WASM layer changes: - Add helper functions: get_chain_service(), get_network_service(), get_cell_service() - All exported functions now call these helpers instead of creating services inline - Eliminates repeated service instantiation code Service layer changes: - Add #[derive(Clone)] to LightClientChainService for sharing across RPC impls Benefits: - Reduced code duplication by ~69 lines - Cleaner method implementations (less boilerplate) - Services created once instead of per-call - Easier to maintain and test Performance impact: - Minimal: Service creation is lightweight (just wrapping Arc references) - But eliminates unnecessary repeated cloning All builds pass: - ✅ Native RPC - ✅ Library - ✅ WASM
Plan to reduce ~2500 lines of duplicated code across three storage backends (RocksDB, SQLite, IndexedDB) by: - Introducing StorageBackend trait for low-level KV operations - Adding default implementations to LightClientStorage trait - Unifying iterator signatures to (key, value) two-parameter form - Consolidating duplicate type definitions
- Add StorageBackend trait with get/put/delete/batch/collect_iterator - Add BatchWriter trait for atomic batch operations - LightClientStorage now extends StorageBackend - Implement StorageBackend for both native (RocksDB) and browser (IndexedDB) - Unified iterator signature to (key, value) two-parameter form - Add get_block_hash and get_transaction methods to LightClientStorage This is Phase 1 of the storage layer refactoring to reduce code duplication.
- Add default filter_block implementation in LightClientStorage trait - Remove duplicated filter_block from native, browser, and sqlite backends - Fix collect_iterator to return transformed value instead of original - Fix get_cells and get_transactions to pass through tx_hash correctly - Fix collect_iterator argument order (skip, limit were swapped) - Add RPC impl constructors and test helpers
quake
force-pushed
the
quake/refactor-and-sqlite
branch
from
f103a02 to
02cb340
Compare
- Remove REFACTORING_PLAN.md and temporary design doc - Add comprehensive STORAGE_REFACTORING.md documenting the new architecture - Describes StorageBackend, LightClientStorage traits, and service layer
Previously the direction parameter was ignored and always iterated forward. Now uses iter_from and direction correctly for get_earliest/get_latest_matched_blocks.
Remove unnecessary proxy method calls in StorageBackend implementations. Directly call underlying storage operations (RocksDB/IndexedDB) instead of delegating to Storage::get/put/delete methods.
…lementations Move all 30+ methods from LightClientStorage trait to have default implementations in storage_trait.rs. The methods now use StorageBackend trait methods (get, put, delete, batch, collect_iterator). This dramatically simplifies the backend implementations: - native.rs: Reduced from ~1100 to ~180 lines - browser.rs: Reduced from ~1400 to ~400 lines Backends now only need to implement StorageBackend (5 methods), then get all LightClientStorage methods for free via default implementations. Also adds LightClientStorage trait imports to all files that call trait methods on Storage, and fixes platform-specific HeaderProvider disambiguation (native uses HeaderProvider::get_header, wasm uses LightClientStorage::get_header).
…implementations Remove all redundant inherent methods from sqlite.rs. The SQLite backend now implements only StorageBackend (5 methods) and gets all LightClientStorage methods for free via default implementations. sqlite.rs reduced from 1077 lines to 291 lines (~73% reduction).
quake
force-pushed
the
quake/refactor-and-sqlite
branch
from
db99741 to
52b3c11
Compare
- Change StorageBackend::batch() to return Self::Batch associated type instead of Box<dyn BatchWriter> to avoid unnecessary heap allocation - Update BatchWriter::commit() signature from Box<Self> to self - Update all three backends (RocksDB, SQLite, IndexedDB) to use the new pattern - Add BatchWriter import to storage_trait.rs for default implementations - Add SQLite CI test job for ubuntu, macos, and windows - Add make clippy-sqlite and test-sqlite targets - Forward sqlite feature from light-client-bin to light-client-lib - Fix TempDir lifetime issue in test helpers to keep temp directory alive
…er impls from backends - Remove CellDataProvider impl from native.rs and sqlite.rs (always unreachable) - Remove HeaderProvider impl from native.rs and sqlite.rs - Unify CellDataProvider and HeaderProvider implementations in StorageWithChainData - Remove conditional compilation, align all backends with browser approach - Reduces code by ~60 lines while maintaining identical behavior
- Replace CursorDirection with IteratorDirection in db-common - Simplify to single conversion function iterator_direction_to_idb() - Remove redundant Next/Prev variants (only NextUnique/PrevUnique were used) - Unify IteratorDirection definition across all platforms
- On wasm32, re-export IteratorDirection from light-client-db-common - Remove redundant type conversion in browser.rs StorageBackend impl - Keep native definition for non-wasm platforms (RocksDB/SQLite)
- Rename KV to KVPair in light-client-db-common for consistency - On wasm32, re-export KVPair from db-common in iterator.rs - Remove redundant type conversion in browser.rs - Keep native KVPair definition for non-wasm platforms
The trait was never implemented - iterator functionality is provided by StorageBackend::collect_iterator method instead.
- Remove From<Direction> for IteratorDirection (never used) - Remove From<(Vec<u8>, Vec<u8>)> for KVPair (never used)
- Remove unused FilterMapType enum (Single variant never constructed) - Use TakeWhileFn and FilterMapFn type aliases from backend.rs - Remove unnecessary #[allow(dead_code)] from WrappedBlockView - Remove unnecessary #[allow(clippy::type_complexity)] where type aliases used
Type aliases TakeWhileFn and FilterMapFn already simplify the types enough.
- Move clippy-sqlite check to the clippy job - Remove redundant toolchain installation from test-sqlite job - Each job runs on independent runner, so consolidating lint checks in one place is more efficient
quake
force-pushed
the
quake/refactor-and-sqlite
branch
from
c7bf729 to
88fc3c2
Compare
The collect_iterator method was causing a deadlock when filter_map_fn called storage.get() because std::sync::Mutex is not reentrant. The method held the mutex lock during iteration, and nested get() calls would try to acquire the same lock, causing tests to hang indefinitely. Fix by collecting all matching rows first while holding the lock, then releasing the lock before processing rows with filter_map_fn.
- Make rocksdb dependency optional in both lib and bin crates - Add feature flags to properly control backend selection - Update conditional compilation to check for rocksdb feature - Add proper error variants for each backend (rocksdb, sqlite, indexdb) - Disable default features for ckb-light-client-lib dependency This allows building with --features sqlite --no-default-features to completely exclude rocksdb from the dependency tree.
The From<IteratorDirection> for Direction impl is only used in the RocksDB backend (native.rs), so move it there for better locality.
- Add IteratorStart enum to light-client-db-common for worker protocol - Update DbCommandRequest::Iterator and IteratorKey to use IteratorStart - Re-export IteratorStart from db-common on wasm32 to avoid duplication - Simplify browser.rs by removing collect_iterator_internal - Update worker's collect_iterator and collect_iterator_keys functions - Unify the API from StorageBackend trait through to worker protocol
The test was failing because the TempDir was being dropped at the end of new_storage(), causing the temporary directory to be deleted while the SQLite database was still in use. Fix by returning (Storage, TempDir) tuple so the temp directory lives as long as the storage is being used in tests.
Officeyutong
previously approved these changes
Feb 9, 2026
eval-exec
approved these changes
Feb 9, 2026
This comment was marked as resolved.
This comment was marked as resolved.
Use block_in_place to wrap blocking_write() call on tokio::sync::RwLock. This prevents panic when set_scripts RPC is called from HTTP worker threads running inside the tokio runtime.
block_in_place requires tokio's rt-multi-thread feature which is not available on wasm32 target. Use cfg attributes to call blocking_write() directly on wasm (safe in single-threaded environment) while keeping block_in_place for native builds.
This comment was marked as resolved.
This comment was marked as resolved.
The collect_iterator function was incorrectly assigning the filter_map result (transformed value) to the key field and using the original value in the value field. This caused panics when parsing keys in functions like get_filter_scripts() because they received value data instead of actual keys. Fixed to match native/RocksDB behavior: - key: original key from database - value: transformed value from filter_map
|
Change browser batch commit to delete first, then put. This ensures correct 'replace' semantics when the same key appears in both delete and add lists (e.g., when setScripts is called with same content). Native (RocksDB) and SQLite implementations don't have this issue because they execute operations in the order they were added.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.