feat(nft): move db lock, add tx fee and confirmations #1989

laruh · 2023-10-11T09:34:30Z

continuation of this PR.
solves comment and issue.

nft_cache_db was added in NftCtx for non wasm target.
Changelog of Async sqlite wrapper gist
AsyncConnection structure was created in mm2src/db_common/src/async_sql_conn.rs. It can be used as async wrapper for sqlite connection.
async_sqlite_connection field was added in MmCtx.

note:

According to this transaction_receipt(hash) can return None for "old" transactions.
transaction_receipt should work fine for swaps, as we need info about the recently broadcasted transaction.

deps added

mm2src/db_common/Cargo.toml
tokio = { version = "1.20", default-features = false, features = ["macros"] }
crossbeam-channel = "0.5.1"
futures = "0.3.1"

artemii235

Thanks for the PR! I have few minor notes and one question.

mm2src/coins/nft.rs

artemii235 · 2023-11-02T05:50:24Z

mm2src/coins/nft/storage/wasm/wasm_storage.rs

                .into_iter()
                .map(|(_item_id, transfer)| transfer);
-            let filtered = Self::filter_transfers(transfer_tables, filters)?;
+            let filtered = filter_transfers(transfer_tables, filters)?;


As I understand, we fetch all transfers from IndexedDB and apply filters thereafter. Can cursor be used instead?

Yeah, we can build and open cursor and then use while let Some((_item_id, item)) = cursor.next() to get next item and apply filters. But seems like cursor.next() doesnt work properly.
I did such impl previously in wasm storage method to filter transfers only with empty metadata. However, GUI got runtime error mm2lib.js:1731 Uncaught Error: closure invoked recursively or after being dropped. I will send you full logs in dm.
PR where I started to use cursor collect method.

Did you look for a way to fix the issue with next? Using collect or requesting all items from the table will cause excessive memory usage. Also, cursor is used in Zcoin WASM implementation, so we have a possible problem with it too.

I tried to use spawn_local_abortable function instead of spawn_local, but to be honest I dont remember where. Didnt work, I think bcz my idea was wrong. As we needed to do hotfix asap we decided to come back to this issue later.

ps: as I can see now collect method utilizes next and collect works fine.

I propose to apply the following plan:

Use next instead of collect where applicable.

Check if Uncaught Error: closure invoked recursively or after being dropped happens again.

If yes, create an issue in the DeFi framework repo.

Troubleshoot and fix the problem.

cc @shamardy

I suppose it can be done in the next iteration 🙂

Thanks for the detailed plan @artemii235 , I totally agree with it :)

shamardy · 2023-11-06T10:26:27Z

@laruh please rebase this PR to dev and then we need to change the base branch in this page.

Edit: After discussions with @laruh, there is no need to rebase now to continue the review process without interrupting it with force pushing.

artemii235

Next iteration 🙂

mm2src/coins/nft.rs

mm2src/coins/nft/nft_structs.rs

mm2src/coins/nft/storage/sql_storage.rs

mm2src/db_common/Cargo.toml

artemii235

Minor notes and question 🙂

mm2src/coins/nft/storage/wasm/wasm_storage.rs

shamardy

Thank you for the PR! First review iteration with only small comments at this stage of the review.

mm2src/coins/nft.rs

mm2src/mm2_core/src/mm_ctx.rs

shamardy · 2023-11-07T13:43:45Z

mm2src/db_common/src/async_sql_conn.rs

Is this code ported? In the future it would be good to follow these guidelines for ported code #1861 (review) #1861 (comment) so that we can track the changes in the commit history, and it will also be easy for reviewers to check the changes to the original code. It's not required for this PR though as it will be hard to do that.

Oh, I see, Yes, its ported from https://github.com/programatik29/tokio-rusqlite/blob/master/src/lib.rs

In PR description I can provide gist with the info about changes in ported files and in deps for this lib.
Also I will provide the list of commits where I added async con first time and then changed it.

Added Changelog of Async sqlite wrapper gist

artemii235

No more notes from my side 🙂

shamardy

Next Review Iteration :)

mm2src/coins/nft/nft_errors.rs

mm2src/mm2_core/src/mm_ctx.rs

shamardy

Only one last comment!

mm2src/db_common/src/async_sql_conn.rs

onur-ozkan

Great work!

Can we have different PRs for per scope next time? It was quite hard to review (e.g., trying to understand which changes relate each other) for me as the changes contain changes on different logics/scopes.

Some notes from my side:

mm2src/coins/nft.rs

mm2src/coins/nft/nft_structs.rs

mm2src/mm2_core/src/mm_ctx.rs

mm2src/db_common/src/async_sql_conn.rs

laruh · 2023-11-10T05:47:04Z

Can we have different PRs for per scope next time? It was quite hard to review (e.g., trying to understand which changes relate each other) for me as the changes contain changes on different logics/scopes.

yeah, sorry for this. Lets try this strategy in next sprint(s). we can create a chain of pull requests, where the base branch of subsequent RP is the feature branch from prev PR.

The base branch was changed.

laruh · 2023-11-21T04:35:05Z

@laruh now you need to change the base branch to dev as discussed on DM some time ago

Done!

shamardy · 2023-11-21T18:18:03Z

@laruh if there are any changes to docs for @KomodoPlatform/qa please provide these changes.

smk762 · 2023-11-27T17:46:07Z

Docs r2r at GLEECBTC/komodo-docs-mdx#42

laruh · 2023-12-11T06:03:03Z

@smk762 @shamardy added commit 4ba539f which fixes redundant moralis reqs during update_nft call.

smk762 · 2023-12-11T06:07:17Z

@smk762 @shamardy added commit 4ba539f which fixes redundant moralis reqs during update_nft call.

tested in isolation and confirmed working

Alrighttt · 2023-12-11T13:40:35Z

~~@laruh please provide a brief justification for each dependency added.~~

edit: Never mind, all the "deps added" are already used within the project.

ca333

secure code reviewed

* dev: (22 commits) chore(config): remove vscode launchjson (GLEECBTC#2040) feat(trading-proto-upgrade): wasm DB, kickstart, refund states, v2 RPCs (GLEECBTC#2015) feat(UTXO): balance event streaming for Electrum clients (GLEECBTC#2013) feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins (GLEECBTC#1930) fix(p2p): handle encode_and_sign errors (GLEECBTC#2038) chore(release): add changelog entries for v2.0.0-beta (GLEECBTC#2037) chore(network): write network information to stdout (GLEECBTC#2034) fix(price_endpoints): add cached url (GLEECBTC#2032) deps(network): sync with upstream yamux (GLEECBTC#2030) fix(config): accept a string as rpcport value (GLEECBTC#2026) feat(nft): move db lock, add tx fee and confirmations (GLEECBTC#1989) chore(network): update seednodes for netid 8762 (GLEECBTC#2024) chore(network): add todo on peer storage behaviour (GLEECBTC#2025) chore(network): exclude `168.119.236.249` from the seednode list (GLEECBTC#2021) feat(network): deprecate 7777 network (GLEECBTC#2020) chore(release): bump mm2 version to 2.0.0-beta (GLEECBTC#2018) feat(UTXO swaps): kmd burn plan impl (GLEECBTC#2006) chore(docs): fix the link to simple market maker in README.md (GLEECBTC#2011) refactor(cli): cli dependency updates and warn on bad config perm (GLEECBTC#1956) chore(containers and docs): update docs and container images (GLEECBTC#2003) ... # Conflicts: # mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs # mm2src/mm2_test_helpers/src/for_tests.rs

* dev: (24 commits) chore(release): bump mm2 version to 2.1.0-beta (GLEECBTC#2044) feat(trezor): add segwit support for withdraw with trezor (GLEECBTC#1984) chore(config): remove vscode launchjson (GLEECBTC#2040) feat(trading-proto-upgrade): wasm DB, kickstart, refund states, v2 RPCs (GLEECBTC#2015) feat(UTXO): balance event streaming for Electrum clients (GLEECBTC#2013) feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins (GLEECBTC#1930) fix(p2p): handle encode_and_sign errors (GLEECBTC#2038) chore(release): add changelog entries for v2.0.0-beta (GLEECBTC#2037) chore(network): write network information to stdout (GLEECBTC#2034) fix(price_endpoints): add cached url (GLEECBTC#2032) deps(network): sync with upstream yamux (GLEECBTC#2030) fix(config): accept a string as rpcport value (GLEECBTC#2026) feat(nft): move db lock, add tx fee and confirmations (GLEECBTC#1989) chore(network): update seednodes for netid 8762 (GLEECBTC#2024) chore(network): add todo on peer storage behaviour (GLEECBTC#2025) chore(network): exclude `168.119.236.249` from the seednode list (GLEECBTC#2021) feat(network): deprecate 7777 network (GLEECBTC#2020) chore(release): bump mm2 version to 2.0.0-beta (GLEECBTC#2018) feat(UTXO swaps): kmd burn plan impl (GLEECBTC#2006) chore(docs): fix the link to simple market maker in README.md (GLEECBTC#2011) ...

laruh added the in progress label Oct 11, 2023

laruh changed the title ~~feat(nft): move db lock~~ feat(nft): move db lock, add tx fee and confirmations Oct 25, 2023

laruh added under review and removed in progress labels Oct 29, 2023

artemii235 self-requested a review October 30, 2023 04:42

artemii235 suggested changes Nov 2, 2023

View reviewed changes

laruh added in progress and removed under review labels Nov 2, 2023

laruh mentioned this pull request Nov 6, 2023

feat(nft): exclude spam and phishing, refactored db #1959

Merged

6 tasks

shamardy changed the base branch from spam-nft-refactor-db to dev November 6, 2023 10:25

shamardy changed the base branch from dev to spam-nft-refactor-db November 6, 2023 10:25

artemii235 suggested changes Nov 7, 2023

View reviewed changes

laruh added in progress and removed under review labels Nov 7, 2023

artemii235 suggested changes Nov 7, 2023

View reviewed changes

mm2src/coins/nft/storage/wasm/wasm_storage.rs Outdated Show resolved Hide resolved

mm2src/coins/nft/storage/wasm/wasm_storage.rs Outdated Show resolved Hide resolved

mm2src/coins/nft/storage/wasm/wasm_storage.rs Show resolved Hide resolved

shamardy reviewed Nov 7, 2023

View reviewed changes

artemii235 previously approved these changes Nov 8, 2023

View reviewed changes

laruh added under review and removed in progress labels Nov 8, 2023

shamardy reviewed Nov 8, 2023

View reviewed changes

mm2src/coins/nft/nft_errors.rs Outdated Show resolved Hide resolved

mm2src/mm2_core/src/mm_ctx.rs Outdated Show resolved Hide resolved

mm2src/mm2_core/src/mm_ctx.rs Show resolved Hide resolved

shamardy reviewed Nov 9, 2023

View reviewed changes

mm2src/db_common/src/async_sql_conn.rs Show resolved Hide resolved

onur-ozkan requested changes Nov 9, 2023

View reviewed changes

onur-ozkan previously approved these changes Nov 10, 2023

View reviewed changes

laruh added in progress and removed under review labels Nov 12, 2023

laruh removed the in progress label Nov 20, 2023

laruh force-pushed the nft-move-db-lock branch from 44d93f8 to c9127ff Compare November 21, 2023 03:48

laruh changed the base branch from spam-nft-refactor-db to dev November 21, 2023 03:49

use match when we canonicalize the sqlite_file_path

2560e37

shamardy previously approved these changes Nov 21, 2023

View reviewed changes

shamardy added sec-review labels Nov 21, 2023

laruh mentioned this pull request Nov 28, 2023

[r2r] Add nft docs GLEECBTC/komodo-docs-mdx#42

Merged

8 tasks

don't update spam transfers with empty meta

4ba539f

laruh dismissed shamardy’s stale review via 4ba539f December 11, 2023 06:01

shamardy added the 2.0.0-beta label Dec 11, 2023

shamardy approved these changes Dec 11, 2023

View reviewed changes

shamardy merged commit 52dab22 into dev Dec 11, 2023

shamardy deleted the nft-move-db-lock branch December 11, 2023 18:08

ca333 reviewed Dec 12, 2023

View reviewed changes

ca333 requested review from Alrighttt and DeckerSU December 12, 2023 06:48

shamardy mentioned this pull request Dec 15, 2023

chore(release): v2.0.0-beta #1968

Merged

feat(nft): move db lock, add tx fee and confirmations #1989

feat(nft): move db lock, add tx fee and confirmations #1989

Uh oh!

Conversation

laruh commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

note:

deps added

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemii235 Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

laruh commented Oct 11, 2023 •

edited

Loading

artemii235 Nov 7, 2023 •

edited

Loading

shamardy commented Nov 6, 2023 •

edited

Loading

laruh Nov 8, 2023 •

edited

Loading

Alrighttt commented Dec 11, 2023 •

edited

Loading