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

Create new index for tracking Asset metadata #2445

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
## [Unreleased]

### Added
- [2445](https://github.com/FuelLabs/fuel-core/pull/2445): Added GQL endpoint for querying asset details.
- [2154](https://github.com/FuelLabs/fuel-core/pull/2154): Added `Unknown` variant to `ConsensusParameters` graphql queries
- [2154](https://github.com/FuelLabs/fuel-core/pull/2154): Added `Unknown` variant to `Block` graphql queries
- [2154](https://github.com/FuelLabs/fuel-core/pull/2154): Added `TransactionType` type in `fuel-client`
Expand Down
12 changes: 12 additions & 0 deletions crates/client/assets/schema.sdl
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ scalar Address

scalar AssetId

type AssetInfoDetails {
contractId: HexString!
Copy link
Contributor

Choose a reason for hiding this comment

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

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

subId: HexString!
totalSupply: U64!
}

type Balance {
owner: Address!
amount: U128!
Expand Down Expand Up @@ -850,6 +856,12 @@ type ProgramState {
}

type Query {
assetDetails(
"""
ID of the Asset
"""
id: AssetId!
): AssetInfoDetails
"""
Read register value by index.
"""
Expand Down
15 changes: 15 additions & 0 deletions crates/client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ use pagination::{
PaginationRequest,
};
use schema::{
assets::{
AssetInfoArg,
AssetInfoDetails,
},
balance::BalanceArgs,
blob::BlobByIdArgs,
block::BlockByIdArgs,
Expand Down Expand Up @@ -1191,6 +1195,17 @@ impl FuelClient {
.transpose()?;
Ok(status)
}

pub async fn asset_info(
&self,
asset_id: &AssetId,
) -> io::Result<Option<AssetInfoDetails>> {
let query = schema::assets::AssetInfoQuery::build(AssetInfoArg {
id: (*asset_id).into(),
});
let asset_info = self.query(query).await?.asset_details.map(Into::into);
Ok(asset_info)
}
}

#[cfg(any(test, feature = "test-helpers"))]
Expand Down
1 change: 1 addition & 0 deletions crates/client/src/client/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use crate::client::pagination::{
};
pub use primitives::*;

pub mod assets;
pub mod balance;
pub mod blob;
pub mod block;
Expand Down
30 changes: 30 additions & 0 deletions crates/client/src/client/schema/assets.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
use crate::client::schema::{
schema,
AssetId,
HexString,
U64,
};

#[derive(cynic::QueryVariables, Debug)]
pub struct AssetInfoArg {
pub id: AssetId,
}

#[derive(cynic::QueryFragment, Clone, Debug)]
#[cynic(
schema_path = "./assets/schema.sdl",
graphql_type = "Query",
variables = "AssetInfoArg"
)]
pub struct AssetInfoQuery {
#[arguments(id: $id)]
pub asset_details: Option<AssetInfoDetails>,
}

#[derive(cynic::QueryFragment, Clone, Debug)]
#[cynic(schema_path = "./assets/schema.sdl")]
pub struct AssetInfoDetails {
pub sub_id: HexString,
pub contract_id: HexString,
pub total_supply: U64,
}
11 changes: 10 additions & 1 deletion crates/fuel-core/src/graphql_api/ports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ use fuel_core_types::{
};
use std::sync::Arc;

use super::storage::balances::TotalBalanceAmount;
use super::storage::{
assets::AssetDetails,
balances::TotalBalanceAmount,
};

pub trait OffChainDatabase: Send + Sync {
fn block_height(&self, block_id: &BlockId) -> StorageResult<BlockHeight>;
Expand Down Expand Up @@ -128,6 +131,10 @@ pub trait OffChainDatabase: Send + Sync {
) -> StorageResult<Option<RelayedTransactionStatus>>;

fn message_is_spent(&self, nonce: &Nonce) -> StorageResult<bool>;

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

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

Choose a reason for hiding this comment

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

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

}

/// The on chain database port expected by GraphQL API service.
Expand Down Expand Up @@ -289,6 +296,7 @@ pub mod worker {
},
},
graphql_api::storage::{
assets::AssetsInfo,
balances::{
CoinBalances,
MessageBalances,
Expand Down Expand Up @@ -371,6 +379,7 @@ pub mod worker {
+ StorageMutate<DaCompressionTemporalRegistryIndex, Error = StorageError>
+ StorageMutate<DaCompressionTemporalRegistryTimestamps, Error = StorageError>
+ StorageMutate<DaCompressionTemporalRegistryEvictorCache, Error = StorageError>
+ StorageMutate<AssetsInfo, Error = StorageError>
{
fn record_tx_id_owner(
&mut self,
Expand Down
3 changes: 3 additions & 0 deletions crates/fuel-core/src/graphql_api/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use fuel_core_types::{
};
use statistic::StatisticTable;

pub mod assets;
pub mod balances;
pub mod blocks;
pub mod coins;
Expand Down Expand Up @@ -118,6 +119,8 @@ pub enum Column {
CoinBalances = 23,
/// Message balances per account.
MessageBalances = 24,
/// See [`AssetsInfo`](assets::AssetsInfo)
AssetsInfo = 25,
}

impl Column {
Expand Down
48 changes: 48 additions & 0 deletions crates/fuel-core/src/graphql_api/storage/assets.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
use fuel_core_storage::{
blueprint::plain::Plain,
codec::{
postcard::Postcard,
raw::Raw,
},
structured_storage::TableWithBlueprint,
Mappable,
};
use fuel_core_types::{
fuel_merkle::common::Bytes32,
fuel_tx::{
AssetId,
ContractId,
},
};

/// Contract info
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Contract info
/// Asset info

pub struct AssetsInfo;

pub type AssetDetails = (ContractId, Bytes32, u64); // (contract_id, sub_id, total_amount)
Copy link
Contributor

Choose a reason for hiding this comment

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

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


impl Mappable for AssetsInfo {
type Key = AssetId;
type OwnedKey = Self::Key;
type Value = Self::OwnedValue;
type OwnedValue = AssetDetails;
}

impl TableWithBlueprint for AssetsInfo {
type Blueprint = Plain<Raw, Postcard>;
type Column = super::Column;

fn column() -> Self::Column {
Self::Column::AssetsInfo
}
}

#[cfg(test)]
mod test {
use super::*;

fuel_core_storage::basic_storage_tests!(
AssetsInfo,
<AssetsInfo as Mappable>::Key::default(),
<AssetsInfo as Mappable>::Value::default()
);
}
71 changes: 67 additions & 4 deletions crates/fuel-core/src/graphql_api/worker_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@ use self::indexation::IndexationError;
use super::{
da_compression::da_compress_block,
indexation,
storage::old::{
OldFuelBlockConsensus,
OldFuelBlocks,
OldTransactions,
storage::{
assets::AssetsInfo,
old::{
OldFuelBlockConsensus,
OldFuelBlocks,
OldTransactions,
},
},
};
use crate::{
Expand Down Expand Up @@ -70,8 +73,10 @@ use fuel_core_types::{
CoinSigned,
},
Contract,
ContractIdExt,
Input,
Output,
Receipt,
Transaction,
TxId,
UniqueIdentifier,
Expand Down Expand Up @@ -381,6 +386,64 @@ where
)
.into());
}

for receipt in result.receipts() {
match receipt {
Receipt::Mint {
sub_id,
contract_id,
val,
..
} => {
let asset_id = contract_id.asset_id(sub_id);
let current_count = match db.storage::<AssetsInfo>().get(&asset_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What we're getting back here is actually not a count. Can you consider renaming the binding?

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)))
}
}
Comment on lines +399 to +407
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

.map(|info| {
info.2
.checked_add(*val)
Comment on lines +408 to +410
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

.ok_or(anyhow::anyhow!("Asset count overflow"))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

})
.transpose()?
.unwrap_or(*val);

db.storage::<AssetsInfo>()
.insert(&asset_id, &(*contract_id, **sub_id, current_count))?;
}
Receipt::Burn {
sub_id,
contract_id,
val,
..
} => {
let asset_id = contract_id.asset_id(sub_id);
let current_count = db
maschad marked this conversation as resolved.
Show resolved Hide resolved
.storage::<AssetsInfo>()
.get(&asset_id)
.unwrap_or_else(|_| {
tracing::warn!("Asset {} is not currently indexed", asset_id);
None
})
.map(|info| {
info.2
.checked_sub(*val)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

.ok_or(anyhow::anyhow!("Asset count overflow"))
maschad marked this conversation as resolved.
Show resolved Hide resolved
})
.transpose()?
.unwrap_or(*val);

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

Choose a reason for hiding this comment

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

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

}
}
}
Ok(())
}
Expand Down
1 change: 1 addition & 0 deletions crates/fuel-core/src/query.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod assets;
mod balance;
mod blob;
mod block;
Expand Down
19 changes: 19 additions & 0 deletions crates/fuel-core/src/query/assets.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
use crate::{
fuel_core_graphql_api::database::ReadView,
graphql_api::storage::assets::AssetDetails,
};
use fuel_core_storage::{
not_found,
Result as StorageResult,
};
use fuel_core_types::fuel_tx::AssetId;

impl ReadView {
pub fn get_asset_details(&self, id: AssetId) -> StorageResult<AssetDetails> {
let asset = self
.off_chain
.asset_info(&id)?
.ok_or(not_found!(AssetDetails))?;
Ok(asset)
}
}
8 changes: 5 additions & 3 deletions crates/fuel-core/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use futures::{
use std::borrow::Cow;
use tokio_stream::StreamExt;

pub mod assets;
pub mod balance;
pub mod blob;
pub mod block;
Expand All @@ -51,6 +52,7 @@ pub mod relayed_tx;

#[derive(MergedObject, Default)]
pub struct Query(
assets::AssetInfoQuery,
dap::DapQuery,
balance::BalanceQuery,
blob::BlobQuery,
Expand Down Expand Up @@ -142,7 +144,7 @@ where
} else if let Some(last) = last {
(last, IterDirection::Reverse)
} else {
return Err(anyhow!("Either `first` or `last` should be provided"))
return Err(anyhow!("Either `first` or `last` should be provided"));
};

let start;
Expand Down Expand Up @@ -170,7 +172,7 @@ where
// Skip until start + 1
if key == start {
has_previous_page = true;
return true
return true;
}
}
}
Expand All @@ -184,7 +186,7 @@ where
// take until we've reached the end
if key == end {
has_next_page = true;
return false
return false;
}
}
count = count.saturating_sub(1);
Expand Down
Loading
Loading