Skip to content

fix(nft): fix notes for 1.0.6-beta#1914

Merged
shamardy merged 9 commits intodevfrom
nft_fix_for_1.0.6
Jul 17, 2023
Merged

fix(nft): fix notes for 1.0.6-beta#1914
shamardy merged 9 commits intodevfrom
nft_fix_for_1.0.6

Conversation

@laruh
Copy link
Copy Markdown

@laruh laruh commented Jul 12, 2023

@laruh
Copy link
Copy Markdown
Author

laruh commented Jul 14, 2023

@Alrighttt @ozkanonur please review Address type for nft addresses and prepared stmt.

@shamardy shamardy added the P1 label Jul 14, 2023
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a 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 fixes! First review iteration!

Comment thread mm2src/coins/eth.rs Outdated
}

pub(crate) struct NftCtx {
pub(crate) guard: Arc<AsyncMutex<()>>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of a guard you should have the nft_cache_db in the Nft context https://github.com/KomodoPlatform/komodo-defi-framework/blob/806ac632a2f65ce09e2215af9dc25298458d45e2/mm2src/coins/lp_coins.rs#L2577
then it can be initialized only once when from_ctx first called and then retrieved on subsequent calls and this is done using from_ctx https://github.com/KomodoPlatform/komodo-defi-framework/blob/eaed80ef9e839e6a51812723fbc2cb66aa15d8d9/mm2src/mm2_core/src/mm_ctx.rs#L608
like how it's used in CoinsContext https://github.com/KomodoPlatform/komodo-defi-framework/blob/806ac632a2f65ce09e2215af9dc25298458d45e2/mm2src/coins/lp_coins.rs#L2562
This is not a blocker since guard works right now, but it's better code structure since now we have CoinsContext and NftCtx and nft_cache_db is in CoinsContext. I think you can remove the need to build storage in every nft function also following from the above.

Copy link
Copy Markdown
Author

@laruh laruh Jul 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of a guard you should have the nft_cache_db in the Nft context

without mutex guard, we will have the same mm2 problem due to race condition.

{"mmrpc":"2.0","error":"DB error ErrorSaving(\"Error uploading an item: \\\"DomException { obj: Object { obj: JsValue(ConstraintError: Unable to add key to index 'chain_tx_hash_index': at least one key does not satisfy the uniqueness requirements.\\\\nundefined) } }\\\"\")","error_path":"nft.wasm_storage.object_store","error_trace":"nft:115] wasm_storage:362] object_store:42]","error_type":"DbError","error_data":"ErrorSaving(\"Error uploading an item: \\\"DomException { obj: Object { obj: JsValue(ConstraintError: Unable to add key to index 'chain_tx_hash_index': at least one key does not satisfy the uniqueness requirements.\\\\nundefined) } }\\\"\")","id":null}

PS: I also caught this problem in native target. Sql propagates the error that you are trying to add item with identical primary key.

I mean if we just move nft_cache_db to NftCtx, we will have the same situation. Instead of CoinsContext::from_ctx(ctx) we will just have NftCtx::from_ctx(&ctx) here
image
Also dont forget, that nft_cache_db is for wasm target, for native we use sqlite_connection from mmctx.

We need to lock the whole function before using storage and sending the moralis request. So I believe it doesn't mater, where we have nft_cache_db or build storage, we need to prevent the access to storage and sending identical moralis request before previous RPC is done.

But yeah, it is a good idea, bcz nft_cache_db should be a part of context of related feature. I didnt notice it.

I think you can remove the need to build storage in every nft function also following from the above.

yep, I have it in my plans. Actually I was trying to move the process of NftBuilder creation and NftStorage building to NftCtx as in this example, but I faced with problem during creating of Box with type impl NftListStorageOps + NftTxHistoryStorageOps. Moreover traits have type Error: NftStorageError. So I postponed this idea until the next refactoring.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved nft_cache_db field to struct NftCtx

Comment thread mm2src/coins/nft.rs Outdated
Copy link
Copy Markdown

@Alrighttt Alrighttt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smk762 smk762 self-requested a review July 17, 2023 19:34
Copy link
Copy Markdown

@smk762 smk762 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing NFT database successful:

On first load, nft list and history empty until update_nft called.
Ddid some transfers, from external and to self, and each time no change to list/history, until update called. No errors encountered.
On self send, saw block number go up in list as expected. No errors encountered.
After logging out and then in again, previous stored list/history was visible without needing to call update.

@shamardy shamardy merged commit 1e0cf5b into dev Jul 17, 2023
@shamardy shamardy deleted the nft_fix_for_1.0.6 branch July 17, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants