MTG-1238 feat(api): add showBurnt option to GetByMethodsOptions
#387
Conversation
armyhaylenko
requested review from
StanChe,
kstepanovdev,
andrii-kl and
n00m4d
| @@ -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>, | |||
There was a problem hiding this comment.
It seems that the conditions of the task are well suited to not use the Option.
show_burnt default value is false.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- We need to maintain existing default behavior. That is, we need to show both burnt and non-burnt by default.
- The feature improvement is allowing people to show only non-burnt.
- 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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
nft_ingester/tests/api_tests.rs
Outdated
|
|
||
| #[tokio::test] | ||
| #[tracing_test::traced_test] | ||
| async fn test_search_assets_without_burnt() { |
There was a problem hiding this comment.
This should make it clearer what this test is about
test_search_assets_without_burnt -> test_search_assets_without_burnt_option
| @@ -2090,6 +2090,112 @@ mod tests { | |||
| ); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
There was a problem hiding this comment.
I would also add test case for show_burnt: Some(true),
armyhaylenko
force-pushed
the
feat/MTG-1238-show-burnt-display-option
branch
from
f28a4b5 to
78ee67e
Compare
andrii-kl
changed the title
showBurnt option to GetByMethodsOptionsshowBurnt option to GetByMethodsOptions
armyhaylenko
force-pushed
the
feat/MTG-1238-show-burnt-display-option
branch
3 times, most recently
from
ae1a8a9 to
8d77533
Compare
|
If that's the final variant with that flag - LGTM! |
armyhaylenko
force-pushed
the
feat/MTG-1238-show-burnt-display-option
branch
from
8d77533 to
904f6a7
Compare
|
Please change base to develop |
armyhaylenko
force-pushed
the
feat/MTG-1238-show-burnt-display-option
branch
from
904f6a7 to
50e7a6f
Compare
No description provided.