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

laruh · 2023-09-04T13:06:27Z

In this PR exclude_spam and exclude_phishing params were added for get_nft_list and get_nft_transfers RPCs.

Db in native target was refactored. It doesnt use details_json with all rust struct fields anymore. To get nft and transfers from db, there is need to fetch all fields from the table. However update process some specific fields from became easier.

If exclude_spam is true, then all nfts or transfers where possible_spam is true will be excluded from response.
If exclude_phishing is true, then all nfts or transfers where possible_phishing is true will be excluded from response.

Link to antispam API docs.

Second sprint iteration from commit 410330a

send req to camo proxy to get UriMeta from json. It protects from phishing and solves cors problem.
impl update of possible_phishing (add requests to /api/blocklist/domain/scan endpoint).
use contains_disallowed_url check for possible spam urls during update process, so we will have already checked nfts in db, which can be excluded.
ignore error from moralis Get NFT metadata response. If we get the error, then return Nft with empty metadata and possible_spam:true.
use cross tests.
impl async wrapper for sql Connection (future sprint).

onur-ozkan

Great work! Some notes from first review iteration:

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

onur-ozkan · 2023-09-05T14:58:53Z

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

+        let nfts: Vec<Nft> = self.get_nfts_by_token_address(chain, token_address.clone()).await?;
+        let locked_db = self.lock_db().await?;


You should lock the db before reading nfts

UPD if we move lock_db before other db methods which access db it breaks it. well bcz another storage method tries to lock db while it already was locked.

Oooh, I see. Hmm, that shouldn't be that way but let's not deal with this issue in this PR. You can revert it to previous state.

You can revert it to previous state.

Already did it.

I noticed that something is wrong bcz couldn't see the rest of tests.
Didnt notice the related error message, they are almost the same as usual.

Will compare errors tomorrow, just to be sure

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

mm2src/coins/nft/storage/sql_storage.rs

…ashSet, passes filter funcs, lock_db at the beginning

onur-ozkan

These are my last notes on this PR. Thank you for patience on fixing the previous ones.

As an extra note, if this PR doesn't need to be rushed, I would highly suggest testing and re-designing the usage of db locks if necessary. Right now as far as I understand, almost every function starts with locking, so if caller and callee locks the db, there is very high chance if being end up with deadlocks. Like it happened #1959 (comment) here.

mm2src/coins/nft/nft_tests.rs

mm2src/coins/nft/storage/mod.rs

mm2src/coins/nft.rs

onur-ozkan

Some notes starting from 6f33a11

mm2src/coins/nft.rs

mm2src/coins/nft/nft_tests.rs

onur-ozkan

Minor notes:

mm2src/mm2_db/src/indexed_db/indexed_db.rs

onur-ozkan

thank you for the fixes!

shamardy

First review iteration from my side! I will review the rest of the code in the next and last review iteration!

shamardy · 2023-10-12T12:13:25Z

mm2src/coins/nft.rs

+        storage
+            .update_transfer_spam_by_token_address(chain, address_hex, true)
+            .await?;


This call is not needed if possible_spam was true before, also can possible_spam change from true to false? meaning that an NFT that was flagged as possible spam passes some criteria and gets reconsidered as not being a spam.

This call is not needed if possible_spam was true before,

oh, yeah agree in refresh_possible_spam there is no need to send spam request again if possible spam is already true bcz of nft_db.common.possible_spam = moralis_meta.common.possible_spam;

also can possible_spam change from true to false

Well we dont know how moralis marks their nft spam or not spam.
But yeah it could be a situation that they changed possible_spam from true to false
But since we use separate db with spamy addresses I suggest to mark nft as spam if contact address has true in our db.

Another but :D I got your idea. lets check in refresh_possible_spam function whether the spam value for contract address has changed in our database or not. If moralis returns false and our spam api returns false for contract in refresh functionality then lets leave this as false.

But yeah it could be a situation that they changed possible_spam from true to false

But since we use separate db with spamy addresses I suggest to mark nft as spam if contact address has true in our db.

I consider this an edge case, but NFT collections can gain traction after sometime and some of the false positive possible spam NFTs due to the criteria used to identify spams can become popular in the future. I see that we don't change true to false anywhere, we can keep it this way for now until we get real cases from users feedback.

mm2src/coins/nft.rs

shamardy · 2023-10-12T12:27:52Z

mm2src/coins/nft.rs

-            if let Ok(token_uri_meta) = serde_json::from_value(response_meta) {
-                uri_meta = token_uri_meta;
-            }
+        if let Some(url) = construct_camo_url_with_token(token_uri, url_antispam) {


I am interested to know how camo will be used for image_url/animation_url/etc.. , will the url be constructed from the GUI side? or will it not be used at all?

@smk did /url/decode/{url_hex} endpoint capable to return you json object or if link returns image type content you will download picture, if request was successful of course(camo doesnt allow redirections). So yeah, GUI also should use camo to download pictures

So yeah, GUI also should use camo to download pictures

Should we provide them the constructed camo url or will they construct it from their side? It can be an option in NftMetadataReq, if requested, we would then convert the urls after we retrieve them from database to camo format.

Should we provide them the constructed camo url or will they construct it from their side?

GUI needs to convert image_url string to hex format and send get req to /url/decode/{url_hex}. There is no need to build the whole complex camo url, proxy does it on its side. All you need is to provide hex format. I think GUI can do it on their side.

It can be an option in NftMetadataReq, if requested, we would then convert the urls after we retrieve them from database to camo format.

As far as I know they send get nft list req with max flag to see all nft info and then try to send req to image_url to download picture for all items. But of course we can speak about it and if they want we can add ready hex format.

shamardy

🔥

shamardy · 2023-10-13T21:37:06Z

@laruh please provide @KomodoPlatform/qa team examples of changes in this PR for docs and testing if needed.

laruh · 2023-10-14T09:08:02Z

@laruh please provide @KomodoPlatform/qa team examples of changes in this PR for docs and testing if needed.

Sure. Actually we already in the process of adding up to date changes in the current doc PR

ca333

secure code reviewed

shamardy · 2023-11-06T09:46:01Z

@laruh @KomodoPlatform/qa can this PR be merged or are there still some work to be done on the QA/docs side?

laruh · 2023-11-06T10:16:10Z

@laruh @KomodoPlatform/qa can this PR be merged or are there still some work to be done on the QA/docs side?

Hi, I think we can merge it. Docs will be updated to match this PR, if we missed something from the current PR we also can add it.
I don't know if qa tested this pull request, but they can test the next one with async wrapper and tx fees & confirmations and also re check exclude spam/phishing.

laruh added 11 commits August 28, 2023 18:34

impl functions to update spam in native, unit tests added.

6102d90

Merge remote-tracking branch 'origin/dev' into spam-phishing-nft

40c5739

update_spam func, db impls for wasm, additional unit tests

d31ca2a

use separated fields in nft sql tables

a1f8655

Merge remote-tracking branch 'origin/dev' into spam-nft-refactor-db

2e4a24a

add path_to_address

79a468c

split antispam tests

e3526c3

exclude spam phishing impls

7d55445

check update_spam logs

e1f2b1c

add more doc comments

e7b9162

Merge remote-tracking branch 'origin/dev' into spam-nft-refactor-db

7fea1e2

laruh added under review 2.1.0-beta labels Sep 4, 2023

onur-ozkan self-requested a review September 5, 2023 07:59

doc comments

f5a142b

onur-ozkan requested changes Sep 5, 2023

View reviewed changes

laruh added in progress Changes will be made from the author and removed under review labels Sep 6, 2023

laruh added 4 commits September 6, 2023 18:00

impl to_table_name fncs, filter_nfts and filter_transfers,use H…

0ba06ef

…ashSet, passes filter funcs, lock_db at the beginning

remove Chain ref in some db methods

3ae16b8

move common consts in wasm_storage

c91fccb

use iter

d698708

laruh added under review in progress Changes will be made from the author and removed in progress Changes will be made from the author under review labels Sep 6, 2023

lock db after getting items

220d672

laruh added under review and removed in progress Changes will be made from the author labels Sep 6, 2023

onur-ozkan requested changes Sep 7, 2023

View reviewed changes

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

mm2src/coins/nft/storage/mod.rs Outdated Show resolved Hide resolved

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

onur-ozkan reviewed Oct 2, 2023

View reviewed changes

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

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

laruh added 2 commits October 3, 2023 18:34

Merge remote-tracking branch 'origin/dev' into spam-nft-refactor-db

e228715

url.path_segments_mut().ok(), move cross_test! to common.rs

9275544

onur-ozkan reviewed Oct 5, 2023

View reviewed changes

mm2src/mm2_db/src/indexed_db/indexed_db.rs Outdated Show resolved Hide resolved

mm2src/mm2_db/src/indexed_db/indexed_db.rs Outdated Show resolved Hide resolved

onur-ozkan mentioned this pull request Oct 5, 2023

feat(zcoin): ARRR WASM implementation #1957

Merged

8 tasks

use const DB_NAME for trait DbInstance

932364e

shamardy added 1.1.0-beta and removed 2.1.0-beta labels Oct 6, 2023

Merge remote-tracking branch 'origin/dev' into spam-nft-refactor-db

92e088e

onur-ozkan previously approved these changes Oct 9, 2023

View reviewed changes

shamardy reviewed Oct 12, 2023

View reviewed changes

dont send scan spam/phishing req in refresh funct if value is true

34229a9

laruh dismissed onur-ozkan’s stale review via 34229a9 October 13, 2023 08:02

laruh mentioned this pull request Oct 13, 2023

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

Merged

shamardy approved these changes Oct 13, 2023

View reviewed changes

shamardy added QA sec-review docs no-deps labels Oct 13, 2023

Merge remote-tracking branch 'origin/dev' into spam-nft-refactor-db

b147b43

ca333 approved these changes Nov 5, 2023

View reviewed changes

shamardy removed sec-review no-deps labels Nov 6, 2023

shamardy merged commit 30c5869 into dev Nov 6, 2023
30 of 32 checks passed

shamardy mentioned this pull request Dec 15, 2023

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

Merged

smk762 removed the docs label Nov 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

laruh commented Sep 4, 2023 •

edited

Loading

onur-ozkan left a comment

onur-ozkan Sep 5, 2023

laruh Sep 6, 2023

laruh Sep 6, 2023 •

edited

Loading

onur-ozkan Sep 6, 2023

laruh Sep 6, 2023 •

edited

Loading

onur-ozkan left a comment

onur-ozkan left a comment

onur-ozkan left a comment

onur-ozkan left a comment

shamardy left a comment

shamardy Oct 12, 2023

laruh Oct 12, 2023 •

edited

Loading

shamardy Oct 13, 2023

shamardy Oct 12, 2023

laruh Oct 12, 2023 •

edited

Loading

shamardy Oct 13, 2023

laruh Oct 14, 2023

shamardy left a comment

shamardy commented Oct 13, 2023

laruh commented Oct 14, 2023

ca333 left a comment

shamardy commented Nov 6, 2023

laruh commented Nov 6, 2023

		let nfts: Vec<Nft> = self.get_nfts_by_token_address(chain, token_address.clone()).await?;
		let locked_db = self.lock_db().await?;

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

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

Conversation

laruh commented Sep 4, 2023 • edited Loading

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

shamardy commented Oct 13, 2023

laruh commented Oct 14, 2023

ca333 left a comment

Choose a reason for hiding this comment

shamardy commented Nov 6, 2023

laruh commented Nov 6, 2023

laruh commented Sep 4, 2023 •

edited

Loading

laruh Sep 6, 2023 •

edited

Loading

laruh Sep 6, 2023 •

edited

Loading

laruh Oct 12, 2023 •

edited

Loading

laruh Oct 12, 2023 •

edited

Loading