-
Notifications
You must be signed in to change notification settings - Fork 94
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
[WASM] Fixing, refactoring, and implementing P2P, IndexedDB, SwapLock, SavedSwap, MySwaps, MyOrdersStorage #1007
Conversation
* Move the low-level Indexed DB interface to the separate 'db_driver' module * Remove channel logic from tx_history_db (tx_history_db communicates with Indexed DB directly)
* Change type of index_value from &str to Json * Move the logic of initializing database from 'CoinsContext::tx_history_db()' to 'indexed_db::db_lock::get_or_initialize_db' * Remove 'TxHistoryOps' and native 'TxHistoryDb' * Replace 'TxHistoryError', 'TxHistoryResult' to lp_coins * Return 'tx_history_path' to MmCoin * Add 'FileLockError', 'FileLockResult' to 'common::file_lock'
* Add DbNamespaceId
* Add 'SwapsContext::swap_db' * Add 'SwapLockTable'
* Remove 'SwapLock::last_timestamp'
* Add 'NodeType::LightInMemory' and 'NodeType::RelayInMemory' variants * Refactor parsing relay addresses
* Do not add relays to 'PeersExchange' in WASM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I have a couple of questions 🙂
I will take one more look at this when we discuss them.
* Refactor 'MarketMakerIt' * Add 'common::log_tag' macro * Configure the 'trade_test_rick_and_morty' and 'test_qrc20_history' tests to run in WASM
# Conflicts: # mm2src/mm2_tests.rs
* Move 'SavedSwap' to the separate 'lp_swap/saved_swap' module * Add 'get_item_by_unique_index', 'replace_item_by_unique_index', 'delete_item_by_unique_index' methods to 'DbTable' * Refactor 'for_tests::register_wasm_log'
* Add 'common::wasm_http' mod with 'FetchRequest'
* Add driver/cursor.rs, Idb<Single/Multi><Bound>KeyCursor * Add indexed_cursor.rs, Db<Single/Multi><Bound>KeyCursor * Add TableUpgrader::create_multi_index * Add IdbTableImpl::cursor_builder * Add DbTable::open_cursor
* Add 'spawn_highload_future' to dispatcher_legacy.rs
* Move common::file_lock to common::fs::file_lock * Move slurp, safe_slurp, file_lock, remove_file, write, read_dir_async, read_dir, json_dir_entries to common::fs
* Use MyOrdersStorage everywhere instead of working with fs/Db manually * Rename swap_db::MySwapsTable to swap_db::MySwapsFiltersTable * Move fs functions from lp_swap::saved_swap to common::fs * Add error_if, warn_if logging macros * Remove OrdermatchContext::my_cancelled_orders
* Add 'OrdermatchContext::ordermatch_db' * Add 'MyActive[Maker/Taker]OrdersTable', 'MyHistoryOrdersTable', 'MyFilteringHistoryOrdersTable' * Kick-start active maker/taker orders on restart
* Enable 'order_status' RPC call in WASM
* Rename 'my_swaps_db::MySwaps' to 'my_swaps_storage::MySwapsStorage' * Rename swap_db.rs to swap_wasm_db.rs
@artemii235 @shamardy PR is ready for the review 🥳 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work!
I've reviewed this PR partially, might have more comments when we solve the questions below.
@sergeyboyko0791 Could you please also solve git conflicts? |
* Remove excessive trait bounds of the `load_history_from_file_impl` function * Refactor error messages
This PR consists of critical changes in many different modules such as The test cases: Transaction history (check in Native and WASM):
Swaps (in Native only):
Orders (in Native only):
|
* Add the 'LogOnError' trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I have left a couple of non-blocker possible refactoring ideas. We can discuss them later. Thanks!
//! | ||
//! # Important | ||
//! | ||
//! Please make sure all keys of the index are specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that created indexes should be represented in the type system somehow to avoid invalid cursor creation. E.g
enum SwapIndexes {
BaseRelStartedAt,
...
}
It might be a part of the TableSignature trait to pin indexes to a specific table: associated type or another way, just thinking out loud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the advice! It makes sense and it's nice to have :)
We can consider integrating it later as you said or if we move it to a public crate
use serde_json::Value as Json; | ||
|
||
#[derive(Debug, Deserialize, Eq, PartialEq, Serialize)] | ||
pub struct MyActiveMakerOrdersTable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an intuitive feeling that it might be worth splitting a Table and it's item into separate structs. They can be pinned to each other using generics or associated types of the TableSignature trait. It's just an idea, I'm unsure whether it's possible to do so without major refactoring. Maybe it doesn't make much sense in general 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I didn't understand why it's needed :D
The only reason I see is the confusing names of the structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason I see is the confusing names of the structures.
Actually yes, it's confusing because a specific struct represents table and table's item at the same time 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great Work 🔥
# Conflicts: # mm2src/lp_native_dex.rs # mm2src/lp_ordermatch.rs # mm2src/mm2_tests.rs
IndexedDB
API to make it easier to add new databases and tablesdb_driver
moduletx_history_db
(tx_history_db
has to communicate with Indexed DB directly)TxHistoryOps
and nativeTxHistoryDb
tx_history_path
toMmCoin
DbNamespaceId
toMmCtx
to allow running multipleMmCtx
at the same timeSwapLock
in WASMSwapsContext::swap_db
SwapLockTable
FileLockError
,FileLockResult
tocommon::file_lock
SwapLock
to maker/taker swapsNodeType::LightInMemory
andNodeType::RelayInMemory
variantsatomicdex_tests
IdbCursor
in RustMyOrdersStorage