Create new index for tracking Asset metadata #2445

maschad · 2024-11-20T21:05:48Z

Linked Issues/PRs

Closes Add indexation AssetId -> (ContractId, SubId) #2425
Related to Consider assets in check-account utility fuels-ts#3373

Description

This introduces a new GraphQL endpoint that returns info about an asset.

The off-chain worker indexes Mint and Burn events to store this metadata about an AssetId

Checklist

New behavior is reflected in tests

Before requesting review

I have reviewed the code myself

After merging, notify other teams

[Add or remove entries as needed]

TS SDK

crates/fuel-core/src/graphql_api/worker_service.rs

tests/tests/assets.rs

AurelienFT

I think we need the test to be working before merging this.

crates/fuel-core/src/graphql_api/worker_service.rs

tests/tests/assets.rs

maschad · 2024-12-02T17:08:06Z

Thanks for all your help @AurelienFT ❤️

rafal-ch · 2024-12-11T12:00:29Z

crates/client/assets/schema.sdl

@@ -2,6 +2,12 @@ scalar Address

 scalar AssetId

+type AssetInfoDetails {
+	contractId: HexString!


Not sure here, but maybe we want to use ContractId! here, as in other places.

rafal-ch · 2024-12-11T12:05:04Z

crates/fuel-core/src/graphql_api/ports.rs

+
+    fn asset_info(&self, asset_id: &AssetId) -> StorageResult<Option<AssetDetails>>;
+
+    fn asset_exists(&self, asset_id: &AssetId) -> StorageResult<bool>;


Seems like asset_exists is not used, can we remove it?

rafal-ch · 2024-12-11T12:05:38Z

crates/fuel-core/src/graphql_api/storage/assets.rs

+    },
+};
+
+/// Contract info


Suggested change

/// Contract info

/// Asset info

rafal-ch · 2024-12-11T12:08:33Z

crates/fuel-core/src/graphql_api/storage/assets.rs

+/// Contract info
+pub struct AssetsInfo;
+
+pub type AssetDetails = (ContractId, Bytes32, u64); // (contract_id, sub_id, total_amount)


Can we use struct with named fields? Something similar to what we have here.

rafal-ch · 2024-12-11T12:12:02Z

crates/fuel-core/src/graphql_api/worker_service.rs

+                    db.storage::<AssetsInfo>()
+                        .insert(&asset_id, &(*contract_id, **sub_id, current_count))?;
+                }
+                _ => {}


Please enumerate all variants explicitly to avoid surprises when new variant is added in the future.

rafal-ch · 2024-12-11T12:24:02Z

crates/fuel-core/src/graphql_api/worker_service.rs

+                    .map(|info| {
+                        info.2
+                            .checked_add(*val)


Suggested change

.map(|info| {

info.2

.checked_add(*val)

.map(|info| {

let (_, _, count) = *info;

count

.checked_add(*val)

May also affect the Receipt::Burn branch.

rafal-ch · 2024-12-11T12:24:07Z

crates/fuel-core/src/graphql_api/worker_service.rs

+                    let current_count = match db.storage::<AssetsInfo>().get(&asset_id) {
+                        Ok(count) => count,
+                        Err(_) => {
+                            // If asset doesn't exist yet, create it with 0 count
+                            db.storage::<AssetsInfo>()
+                                .insert(&asset_id, &(*contract_id, **sub_id, 0))?;
+                            Some(Cow::Owned((*contract_id, **sub_id, 0)))
+                        }
+                    }


I think we shouldn't treat all storage errors as "value not found". Some of them should just be bubbled up.

Maybe something like this will work:

let current_count = match db.storage::<AssetsInfo>().get(&asset_id)? { Some(count) => Some(count), None => { // If asset doesn't exist yet, create it with 0 count db.storage::<AssetsInfo>() .insert(&asset_id, &(*contract_id, **sub_id, 0))?; Some(Cow::Owned((*contract_id, **sub_id, 0))) } }

If you agree, please note that now two branches always return Some, so maybe we can get rid of the follow-up map() as well.

rafal-ch · 2024-12-11T12:34:55Z

crates/fuel-core/src/graphql_api/worker_service.rs

+                        })
+                        .map(|info| {
+                            info.2
+                                .checked_sub(*val)


If we go down to 0 on Burn should we remove the asset from the index? Unless we need to distinguish between "never minted" and "minted and totally burned" cases.

Anyway, currently the index may grow indefinitely.

Which also makes me think that I should consider this for the balances indexation I added recently :-)

cc: @xgreenx, wdyt?

rafal-ch · 2024-12-11T12:38:15Z

crates/fuel-core/src/graphql_api/worker_service.rs

+                    .map(|info| {
+                        info.2
+                            .checked_add(*val)
+                            .ok_or(anyhow::anyhow!("Asset count overflow"))


You may consider integrating this with the IndexationError available here: https://github.com/FuelLabs/fuel-core/blob/master/crates/fuel-core/src/graphql_api/indexation.rs#L28

It's a common type for all errors related to integration and it'll grow even more variants after the "coins to spend" indexation is merged. See here.

rafal-ch · 2024-12-11T12:40:12Z

tests/tests/assets.rs

+        .0;
+
+    // Then
+    // We should have the minted amount first


Suggested change

// We should have the minted amount first

// We should have the minted amount reduced by the burned amount

rafal-ch · 2024-12-12T10:14:53Z

crates/fuel-core/src/graphql_api/worker_service.rs

+                    }
+                    .map(|info| {
+                        info.2
+                            .checked_add(*val)


Consider the following scenario:

client v.0.40 running

asset A in contract B is minted (count=10)

we update the client to the one that supports index

asset A in contract B is minted again (count=10, for a total count of 20)

we query the asset info

it notices there is no index, initializes it with 0 and adds 10

we have inconsistency (10 vs 20)

I may not have full picture, but if you see this as a possible scenario, please consider enabling access to this endpoint conditionally, ie. it should be available only when the client was started with a clear database and have a chance to sync from genesis (and build the complete index). This is what we did with other indexes (see for example around here).

maschad added 4 commits November 19, 2024 16:16

feat: add indexing for asset id metadata for gql endpoint

610fd37

feat: add gql endpoint to retrieve from off-chain db

54375c3

feat: updated GQL structs

1dc6932

lint: formatting

246eae3

maschad self-assigned this Nov 20, 2024

maschad added 2 commits November 20, 2024 16:07

docs: added changelog

684361d

Merge branch 'master' into mc/feat/index-asset-ids

d750c73

xgreenx reviewed Nov 21, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/worker_service.rs Show resolved Hide resolved

maschad added 2 commits November 21, 2024 14:15

fix: increase based on burn/mint amount

ab09b8d

test: add test for checking totals

07fa5e5

maschad commented Nov 22, 2024

View reviewed changes

tests/tests/assets.rs Outdated Show resolved Hide resolved

fix: remove unused method

b2abf4d

maschad requested a review from xgreenx November 22, 2024 20:41

maschad marked this pull request as ready for review November 22, 2024 20:41

maschad requested review from Dentosal and MitchTurner as code owners November 22, 2024 20:41

maschad requested review from acerone85, netrome, AurelienFT, rymnc and rafal-ch November 22, 2024 20:45

Merge branch 'master' into mc/feat/index-asset-ids

3b56332

AurelienFT requested changes Nov 27, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/worker_service.rs Show resolved Hide resolved

tests/tests/assets.rs Outdated Show resolved Hide resolved

maschad and others added 4 commits November 29, 2024 19:40

Merge branch 'master' into mc/feat/index-asset-ids

0cb04fd

chore: handle unindexed asset events

6ae2051

Fix the indexation and rework the tests

098b031

Add GWT in test

630da0d

AurelienFT approved these changes Dec 2, 2024

View reviewed changes

Merge branch 'master' into mc/feat/index-asset-ids

ef2e9ce

Merge branch 'master' into mc/feat/index-asset-ids

5ae7e63

rafal-ch reviewed Dec 11, 2024

View reviewed changes

rafal-ch reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create new index for tracking Asset metadata #2445

Create new index for tracking Asset metadata #2445

maschad commented Nov 20, 2024 •

edited

Loading

AurelienFT left a comment

maschad commented Dec 2, 2024 •

edited

Loading

rafal-ch Dec 11, 2024

rafal-ch Dec 11, 2024

rafal-ch Dec 11, 2024

rafal-ch Dec 11, 2024

rafal-ch Dec 11, 2024

rafal-ch Dec 11, 2024

rafal-ch Dec 11, 2024

rafal-ch Dec 11, 2024

rafal-ch Dec 11, 2024

rafal-ch Dec 11, 2024

rafal-ch Dec 12, 2024


		fn asset_info(&self, asset_id: &AssetId) -> StorageResult<Option<AssetDetails>>;

		fn asset_exists(&self, asset_id: &AssetId) -> StorageResult<bool>;

	// We should have the minted amount first
	// We should have the minted amount reduced by the burned amount

Create new index for tracking Asset metadata #2445

Are you sure you want to change the base?

Create new index for tracking Asset metadata #2445

Conversation

maschad commented Nov 20, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

AurelienFT left a comment

Choose a reason for hiding this comment

maschad commented Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maschad commented Nov 20, 2024 •

edited

Loading

maschad commented Dec 2, 2024 •

edited

Loading