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.

Sign up for GitHub

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-1238 feat(api): add showBurnt option to GetByMethodsOptions #387

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

MTG-1238 feat(api): add showBurnt option to GetByMethodsOptions #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions entities/src/api_req_params.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ pub struct GetByMethodsOptions {
pub show_zero_balance: bool,
#[serde(default)]
pub show_fungible: bool,
#[serde(default)]
pub show_burnt: Option<bool>,
Copy link
Collaborator

@andrii-kl andrii-kl Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that the conditions of the task are well suited to not use the Option.
show_burnt default value is false.

Copy link
Contributor Author

@armyhaylenko armyhaylenko Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i honestly don't really agree, i want it to follow a similar behavior to search_assets. meaning that None means show all, burnt and not burnt, Some(true) meaning only show burnt assets, and Some(false) meaning only show non-burnt assets. we can consider a rename to show_burnt_only, but this definitely should be a bool in this case and it would imply that we show either burnt or non-burnt, and not all of them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we see the task differently. In my version I don't see the need to display only burned nfts.
This can be implemented in this way, but in my opinion, it goes beyond the scope of the task / business requirements.

I see next options:
showBurnt True - display all (items with option burnt=true / false)
showBurnt False - display only not burnt items (items with option burnt=false)

No showBurnt option is equal to showBurnt False as a default behavior.

This approach will allow Wallets to show current nfts (not burned) or to show all. But it's better to get more information/requirements from the business side, since handling three variants complicates the architecture @danenbm

Copy link

@danenbm danenbm Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

When I was originally discussing this with Nhan, we had thought that most people care about non-burnt assets, so at that time I was originally thinking of hiding them by default.

However, I have talked to customers (such as Mallow) who told me that people burn the NFTs as part of an NFT collection mechanic, but still want to see the burnt NFT artwork. Also we have recently talked about the importance of matching existing DAS behavior.

Therefore, I have come to the conclusion:

  1. We need to maintain existing default behavior. That is, we need to show both burnt and non-burnt by default.
  2. The feature improvement is allowing people to show only non-burnt.
  3. Showing only burnt wasn't primary use case. Still, maybe they would use the option this way if they knew it existed, so its not bad to support it.

I have one clarifying question:
Whether it's an Option<bool> or a bool, in practice the end-user is either specifying the flag or leaving it blank, right? So the complication is just how displayOptions are parsed. In old DAS this would mean replacing a call to: options.unwrap_or_default(). But I did not see that in this PR at first glance, so not sure how much complexity is required to deal with Option<bool>.

I agree that a bool is simpler and matches how the other displayOptions are stored, but I think to have only binary behavior and meet the requirements, we would have to call it something like hideBurntItems or showUnburntItemsOnly and default to false. I think this would be more confusing to end users.

If the end user can specify in their request:

  • "showBurnt": "true" is just burnt
  • "showBurnt": "false" is only non-burnt
  • when user doesn't specify flag it shows all
    I think that would be less confusing to end-user.

We can chat about this at standup as well. This is not high priority feature request. So we can also hold off on this feature if we cannot easily settle on the interface or the complexity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO:

The field is called display options, and it's not a filter. Or at least it shouldn't be one. It should not impact the number of the assets displayed, just the layout. This is already broken by show_inscription, show_zero_balance, show_fungible and probably show_unverified_collections as well. Given this any option that we choose will only increase the conflicting behaviour.

Now for the matter at hand:

Given the existing behaviour we should call the flag skipBurnt bool (hideBurnt will be another alternative)

Copy link

Choose a reason for hiding this comment

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

I'm now thinking we should just pause on this feature and wait for more customer feedback if it is needed. It is not high priority and nobody has asked for it, and if/when we add it we should also consider backporting to digital-asset-standard-api repo and digital-asset-rpc-infrastructure repo, which is also not something I want to take time doing right now.

}

impl From<&SearchAssetsOptions> for Options {
Expand Down Expand Up @@ -591,6 +593,7 @@ impl From<SearchAssetsOptions> for GetByMethodsOptions {
show_inscription: value.show_inscription,
show_zero_balance: value.show_zero_balance,
show_fungible: false,
show_burnt: None,
}
}
}
Expand Down
1 change: 1 addition & 0 deletions nft_ingester/src/api/dapi/converters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ impl TryFrom<GetAssetsByOwner> for SearchAssetsQuery {
validate_pubkey(asset_owner.owner_address).map(|k| k.to_bytes().to_vec())?,
),
supply: Some(AssetSupply::Greater(0)),
burnt: asset_owner.options.show_burnt,
..Default::default()
})
}
Expand Down
98 changes: 98 additions & 0 deletions nft_ingester/tests/api_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1861,6 +1861,104 @@ mod tests {
);
}

#[tokio::test]
Copy link
Collaborator

@andrii-kl andrii-kl Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would also add test case for show_burnt: Some(true),

#[tracing_test::traced_test]
async fn test_get_only_non_burnt_assets_by_owner() {
let cnt = 20;
let cli = Cli::default();
let (env, generated_assets) =
setup::TestEnvironment::create_burnt(&cli, cnt, SLOT_UPDATED).await;
let api = create_api(&env, None);
let tasks = JoinSet::new();
let mutexed_tasks = Arc::new(Mutex::new(tasks));

let ref_value = generated_assets.owners[8].clone();
let payload = GetAssetsByOwner {
owner_address: ref_value.owner.value.map(|owner| owner.to_string()).unwrap_or_default(),
sort_by: None,
limit: None,
page: None,
before: None,
after: None,
cursor: None,
options: GetByMethodsOptions {
show_unverified_collections: true,
show_burnt: Some(false),
..Default::default()
},
};

let res = api.get_assets_by_owner(payload, mutexed_tasks.clone()).await.unwrap();
let res_obj: AssetList = serde_json::from_value(res).unwrap();

// in the setup all assets were created as burnt
// meaning that if we do not explicitly specify show_burnt
// in the options, the response will be empty
assert_eq!(res_obj.total, 0, "total should be 0");
assert_eq!(res_obj.items.len(), 0, "items length should be 0");
}

#[tokio::test]
#[tracing_test::traced_test]
async fn test_get_only_burnt_assets_by_owner() {
let cnt = 20;
let cli = Cli::default();
let (env, generated_assets) =
setup::TestEnvironment::create_burnt(&cli, cnt, SLOT_UPDATED).await;
let api = create_api(&env, None);
let tasks = JoinSet::new();
let mutexed_tasks = Arc::new(Mutex::new(tasks));

let ref_value = generated_assets.owners[8].clone();
let payload = GetAssetsByOwner {
owner_address: ref_value.owner.value.map(|owner| owner.to_string()).unwrap_or_default(),
sort_by: None,
limit: None,
page: None,
before: None,
after: None,
cursor: None,
options: GetByMethodsOptions {
show_unverified_collections: true,
show_burnt: Some(true),
..Default::default()
},
};

let res = api.get_assets_by_owner(payload, mutexed_tasks.clone()).await.unwrap();
let res_obj: AssetList = serde_json::from_value(res).unwrap();

assert_eq!(res_obj.total, 1, "total should be 1");
assert_eq!(res_obj.items.len(), 1, "items length should be 1");
}

#[tokio::test]
#[tracing_test::traced_test]
async fn test_search_assets_excluding_burnt_assets() {
let cnt = 20;
let cli = Cli::default();
let (env, _generated_assets) =
setup::TestEnvironment::create_burnt(&cli, cnt, SLOT_UPDATED).await;
let api = create_api(&env, None);
let tasks = JoinSet::new();
let mutexed_tasks = Arc::new(Mutex::new(tasks));
let limit = 20;
let payload = SearchAssets {
burnt: Some(false),
limit: Some(limit),
options: SearchAssetsOptions {
show_unverified_collections: true,
..Default::default()
},
..Default::default()
};
let res = api.search_assets(payload, mutexed_tasks.clone()).await.unwrap();
assert!(res.is_object());
let res_obj: AssetList = serde_json::from_value(res).unwrap();
assert_eq!(res_obj.total, 0, "total should be 0");
assert_eq!(res_obj.items.len(), 0, "assets length should be 0");
}

#[tokio::test]
#[tracing_test::traced_test]
async fn test_get_assets_by_group() {
Expand Down
3 changes: 2 additions & 1 deletion postgre-client/src/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub struct AssetSortedIndex {
pub sorting_id: String,
}

#[derive(Default)]
#[derive(Default, Debug)]
pub struct SearchAssetsFilter {
pub specification_version: Option<SpecificationVersions>,
pub specification_asset_class: Option<SpecificationAssetClass>,
Expand All @@ -92,6 +92,7 @@ pub struct SearchAssetsFilter {
pub token_type: Option<TokenType>,
}

#[derive(Debug)]
pub enum AssetSupply {
Greater(u64),
Equal(u64),
Expand Down
2 changes: 1 addition & 1 deletion rocks-db/benches/misc_benchmark.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ fn bincode_decode_benchmark(c: &mut Criterion) {
let slot = 100;
let assets = pubkeys
.iter()
.map(|pk| setup::rocks::create_test_dynamic_data(*pk, slot, "solana".to_string()))
.map(|pk| setup::rocks::create_test_dynamic_data(*pk, slot, "solana".to_string(), false))
.map(|a| serialize(&a).unwrap())
.collect::<Vec<_>>();

Expand Down
26 changes: 26 additions & 0 deletions tests/setup/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,32 @@ impl<'a> TestEnvironment<'a> {
.await
}

pub async fn create_burnt(
cli: &'a Cli,
cnt: usize,
slot: u64,
) -> (TestEnvironment<'a>, rocks::GeneratedAssets) {
Self::create_and_setup_from_closures(
cli,
cnt,
slot,
&[
SpecificationAssetClass::Unknown,
SpecificationAssetClass::ProgrammableNft,
SpecificationAssetClass::Nft,
SpecificationAssetClass::FungibleAsset,
SpecificationAssetClass::FungibleToken,
SpecificationAssetClass::MplCoreCollection,
SpecificationAssetClass::MplCoreAsset,
],
RocksTestEnvironmentSetup::with_authority,
RocksTestEnvironmentSetup::test_owner,
RocksTestEnvironmentSetup::dynamic_data_burnt,
RocksTestEnvironmentSetup::collection_without_authority,
)
.await
}

#[allow(clippy::too_many_arguments)]
pub async fn create_and_setup_from_closures(
cli: &'a Cli,
Expand Down
22 changes: 19 additions & 3 deletions tests/setup/src/rocks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,18 @@ impl RocksTestEnvironmentSetup {
pub fn dynamic_data(pubkeys: &[Pubkey], slot: u64) -> Vec<AssetDynamicDetails> {
pubkeys
.iter()
.map(|pubkey| create_test_dynamic_data(*pubkey, slot, DEFAULT_TEST_URL.to_owned()))
.map(|pubkey| {
create_test_dynamic_data(*pubkey, slot, DEFAULT_TEST_URL.to_owned(), false)
})
.collect()
}

pub fn dynamic_data_burnt(pubkeys: &[Pubkey], slot: u64) -> Vec<AssetDynamicDetails> {
pubkeys
.iter()
.map(|pubkey| {
create_test_dynamic_data(*pubkey, slot, DEFAULT_TEST_URL.to_owned(), true)
})
.collect()
}

Expand Down Expand Up @@ -356,15 +367,20 @@ impl RocksTestEnvironmentSetup {
pub const DEFAULT_PUBKEY_OF_ONES: Pubkey = Pubkey::new_from_array([1u8; 32]);
pub const PUBKEY_OF_TWOS: Pubkey = Pubkey::new_from_array([2u8; 32]);

pub fn create_test_dynamic_data(pubkey: Pubkey, slot: u64, url: String) -> AssetDynamicDetails {
pub fn create_test_dynamic_data(
pubkey: Pubkey,
slot: u64,
url: String,
is_burnt: bool,
) -> AssetDynamicDetails {
AssetDynamicDetails {
pubkey,
is_compressible: Updated::new(slot, None, false),
is_compressed: Updated::new(slot, None, false),
is_frozen: Updated::new(slot, None, false),
supply: Some(Updated::new(slot, None, 1)),
seq: None,
is_burnt: Updated::new(slot, None, false),
is_burnt: Updated::new(slot, None, is_burnt),
was_decompressed: Some(Updated::new(slot, None, false)),
onchain_data: None,
creators: Updated::new(slot, None, vec![generate_test_creator()]),
Expand Down
Loading