Skip to content
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

Add ConcurrentFungibleAssetSupply and ConcurrentFungibleAssetBalance #338

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

vusirikala
Copy link
Contributor

@vusirikala vusirikala commented Apr 3, 2024

This PR adds the indexer support for the AIP 70 (aptos-labs/aptos-core#11183).
AIP 70 adds ConcurrentSupply and ConcurrentBalance for fungible assets. Unlike the regular supply and balance, the newly added ConcurrentSupply and ConcurrentBalance use aggregators for improved parallelizability.

This PR indexes ConcurrentSupply and ConcurrentBalance in the indexer. The indexing is done in a way that Supply and ConcurrentSupply, Balance and ConcurrentBalance are treated identically when storing into the database so that the indexer client wouldn't notice a difference whether the aggregators are being used or not.

Backfill

  • Testnet: 1230793516
  • Mainnet: not there yet

@@ -112,7 +124,7 @@ impl FungibleAssetBalance {
asset_type: asset_type.clone(),
is_primary,
is_frozen: inner.frozen,
amount: inner.balance.clone(),
amount: concurrent_balance.unwrap_or_else(|| inner.balance.clone()),
Copy link
Contributor

Choose a reason for hiding this comment

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

like it.

Copy link
Collaborator

@bowenyang007 bowenyang007 left a comment

Choose a reason for hiding this comment

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

I don't think that this'll work? Were you able to get it to show concurrent balance? Not making sense to me tbh.

Comment on lines 98 to 92
let concurrent_balance: Option<BigDecimal> =
ConcurrentFungibleAssetBalance::from_write_resource(
write_resource,
txn_version,
)
.ok()
.and_then(|balance| balance.map(|b| Some(b.balance.value.clone())))
.unwrap_or(None);
Copy link
Collaborator

@bowenyang007 bowenyang007 Apr 23, 2024

Choose a reason for hiding this comment

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

wait how are you able to get the balance given this write resource is actually FungibleAssetStore? I think that you might need to pre-process and store it in object_metadatas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it by pre-processing and storing in object_metadatas.

pub current: AggregatorU128,
}

impl ConcurrentFungibleAssetSupply {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem to be used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the observation. I am now pre-processing and storing the ConcurrentFungibleAssetSupply in object_metadatas. I modified FungibleAssetMetadataModel to store the ConcurrentFungibleAssetSupply in supply_v2 and maximum_v2.

@rtso
Copy link
Collaborator

rtso commented Apr 24, 2024

Can you also add a test plan? Usually for testing, we run the processor on example transactions and check that the data stored in postgres is correct.

@vusirikala vusirikala force-pushed the satya/fungible-concurrenty-supply branch from b9245ca to 60f48d6 Compare May 13, 2024 23:44
@lightmark
Copy link
Contributor

Any progress?

Copy link
Collaborator

@bowenyang007 bowenyang007 left a comment

Choose a reason for hiding this comment

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

looks good to me. I'm curious how it's tested though.

@@ -450,6 +450,15 @@ pub struct AggregatorSnapshotU64 {
pub value: BigDecimal,
}

// TODO: How is this different from AgggregatorU64?
#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct AggregatorU128 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if it's not diff from U64 can you just rename and reuse? These names don't matter. E.g. you can name it AggregatorBigint or something.

@bowenyang007 bowenyang007 force-pushed the satya/fungible-concurrenty-supply branch 2 times, most recently from bcaa820 to 5ba621c Compare May 31, 2024 17:44
@bowenyang007 bowenyang007 changed the base branch from main to lightmark/coin_fa_balance May 31, 2024 17:44
Base automatically changed from lightmark/coin_fa_balance to main May 31, 2024 19:01
@bowenyang007 bowenyang007 force-pushed the satya/fungible-concurrenty-supply branch from 5ba621c to 8b217d4 Compare May 31, 2024 19:03
@vusirikala vusirikala force-pushed the satya/fungible-concurrenty-supply branch from 8b217d4 to d742c8b Compare May 31, 2024 20:35
@bowenyang007 bowenyang007 merged commit 196cfc5 into main Jun 4, 2024
7 checks passed
@bowenyang007 bowenyang007 deleted the satya/fungible-concurrenty-supply branch June 4, 2024 20:17
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