-
Notifications
You must be signed in to change notification settings - Fork 39
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
MTG-404 Skip batch mint indexing feature #207
Conversation
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.
Several questions to clarify:
- entries in batch_mint_to_verify are never dropped, right?
- account_worker method has some hardcoded false in the list of arguments sent to ProgramTransformer::new. Is this intended?
- the batch_mint_to_verify entries are always created, without regards to the value from config, am I getting it right?
nft_ingester/src/account_updates.rs
Outdated
@@ -35,6 +35,7 @@ pub fn account_worker<T: Messenger>( | |||
pool, | |||
create_download_metadata_notifier(bg_task_sender), | |||
false, | |||
false, |
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.
This value should be coming from the same config, we don't want to always index the batch minted trees even when the flag to set it is set on the config
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.
Here account_worker is created and both cl_audits
and skip_batch_minted_trees
flags don't make any sense. During accounts handling they are not used
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'd put that in a comment, as it's not crystal clear
nft_ingester/src/account_updates.rs
Outdated
@@ -35,6 +35,7 @@ pub fn account_worker<T: Messenger>( | |||
pool, | |||
create_download_metadata_notifier(bg_task_sender), | |||
false, |
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.
Don't we need to pass the cl_audits value from the config as well
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.
not sure that we have to pass value from the config for account worker such as this flag is related to transaction processing only
if let Some(batched_trees) = batched_trees { | ||
let mut b_trees = batched_trees.write().await; | ||
b_trees.insert(tree_id.clone()); | ||
drop(b_trees); |
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.
Isn't this drop unnecessary? The lock will be dropped as soon as we leave the context of if
anyway
program_transformers/src/lib.rs
Outdated
let batched_trees = batched_trees.read().await; | ||
|
||
if let Some(_tree) = batched_trees.get(&change_log.id) { | ||
drop(batched_trees); |
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.
this drop may be removed as well
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.
- right, there only status is updated
- intended, because for account_worker both
cl_audits
andskip_batch_minted_trees
flags don't make any sense. During accounts handling they are not used - yes because we create entry there during
FinalizeTreeWithRoot
instruction processing and that instruction will be processed even if we setskip_batch_minted_trees
to true because at the moment indexer receive instruction it doesn't have tree ID in a hashSet yet
What
This PR extends/improves previously added feature - skip batch minted trees indexing.
Why
Code with previous version was not indexing batch mint instructions, if specific flag was set, but if there were any changes in a tree after it created like mint, transfer and so on all that updates were indexed. It means that API would try to return proofs for that assets but it would be incorrect because most of the updates were missed such as initiall instruction was not parsed. Now indexer checks if merkle tree update we received is related to batched minted tree and if so it will not save its update, of course if skip indexing flag is set to true.
How
There is hashSet added to the program transformer. On start that hashSet is populated with all the batched minted trees from the batch_mint_to_verify table and then it's updating during its work. When we get cNFT update we check if tree ID is in hashSet and if so - skip this update.