aptos-labs / aptos-indexer-processors

Set of core processors that index data on the Aptos blockchain
https://aptos.dev/indexer/indexer-landing
60 stars 73 forks source link

[Processor] Simplify some logic to reduce unnecessary duplicated work. #501

Closed grao1991 closed 2 weeks ago

grao1991 commented 2 months ago

This PR is trying to move some common logic/type/constant to resources.rs, avoid duplicate/unnecessary parsing/deserialization logic when converting between generic write resource and a specific resource type.

The main motivation is to improve the performance, no other behavior change is expected. To give an idea on the performance impact, when running simple token minting workload, the processing time of token_v2_processor is 5x less than before.

This PR only handles FA and TokenV2 resources, leave other resources like Coin and Token for future PRs.

bowenyang007 commented 2 months ago

This looks really good! Did we test this? Also don't forget to deploy before merging. For testing, maybe @rtso and @yuunlimm can help? We should at least compare the before and after, but it could also be a great case for an integration test looking for diffs.

grao1991 commented 2 months ago

This looks really good! Did we test this? Also don't forget to deploy before merging. For testing, maybe @rtso and @yuunlimm can help? We should at least compare the before and after, but it could also be a great case for an integration test looking for diffs.

I don't have a good way to get a full coverage for everything. I only manually sampled some rows and didn't see any difference before and after.

rtso commented 2 months ago

The general code structure looks good to me, but I'm hesitant to stamp this until we do adequate testing because it's such a big change. We can either write some integration tests (@yuunlimm or @larry-aptos do either of you have bandwidth to write these tests?) or we can deploy to devnet and manually check each resource is indexed.

grao1991 commented 2 weeks ago

Rebased my PR on top of tests. PTAL @rtso @yuunlimm @larry-aptos