Joystream / atlas

Whitelabel consumer and publisher experience for Joystream
https://www.joystream.org
GNU General Public License v3.0
100 stars 45 forks source link

Address some issues with logic in Designs: Issuing NFT #2171

Closed KubaMikolajczyk closed 2 years ago

KubaMikolajczyk commented 2 years ago

AC:


Original msg was from @kdembler on rocketchat:

Hey [kuba], I've just started writing logic for running NFT blockchain transactions, and looking much more deeply into specific listing options etc. I arrived to a couple of questions regarding the issuing design you've done:

  1. I see that in our designs we don't include the minimal bid step option. This defines what's a minimal difference between last and new bid. You can see it in the handbook. Was this skipped deliberately or we missed that?
  2. Because of how we compose options for the auction, it seems we may run into some incompatible scenarios. In our form everything is optional, but that's not fully the case on technical level. For example: let's assume you toggle the Fixed price option and provide a value. On its own, this option would indicate that we don't want to put the NFT in an auction, just set it in the BuyNow mode, as seen in this section. Then, let's say, you trigger the auction duration option. Now, with a duration, the NFT can no longer be issued in the BuyNow mode but it needs to be put on an auction, depending on whether you provide an end date, that will be either open or English auction. You are free to proceed in this state. However, on technical level, both of those auctions also require providing other values, for example Minimum bid and Bid step mentioned in the point above. Is the problem description clear? What do you think we could do here?
kdembler commented 2 years ago

One more note: Seems Royalties option is available always in the second form step (auction details) even when putting an existing NFT on sale. However, royalties need to be defined at time of NFT issuance. This has 2 implications:

  1. We shouldn't present an option to edit royalties when an existing NFT is being put on sale
  2. We should allow the user to set royalties also when they're issuing their NFT in an idle mode.
KubaMikolajczyk commented 2 years ago

Intro

@dmtrjsg @bedeho it's a big chunk of work that has to be done asap so if you could scan what we agreed on with Klaudiusz and raise any concerns if you have any. Or just give me a green light to proceed with this work that would be perfect 🟢

Overview

After the above issues were raised we met together with @kdembler to discuss currently discovered problems and deliver fast solutions and here is what we agreed on:

1. Minimum bid step

image

It should be added as a setting that the NFT creator/owner can set from the "set up listing" step, but it shouldn't be a required field. This means that in an inactive state it should have some default value - the current proposition from Klaudiusz is to set the default value to 5% of the minimum bid value (but this value will be discussed between @kdembler and @bedeho)

2. Conflicting setting scenarios

Due to the recent discovery that many combinations won't work together, (if it comes to settings of listing), there is a need to rearrange a bit our fields in steps.

image

In the first screen we should have 3 listing types: Auction, Buy now & Not for sale

image

This will allow us, whenever a user chooses to go with the Auction listing type, to comfortably communicate about the fact that bidding will be accepted anyway even if the user did not specify a specific value for a minimum bid, and btw we can have a default value for the minimum bid

image

And if the user chooses the buy now option - only a fixed price field will be there. So in this case user can be sure that no bids will be accepted and the video NFT can be only bought for a fixed price. No auction duration, no whitelist, no minimum step bid - because all of those settings require accepting a bid offer.

image

And last but not least - in auction listing type you will also be able to set a fixed price but even if you do not specify a minimum bid - the minimum bid field will have its minimum default value (which is to be specified by @kdembler )

3. Royalties as NFT setting, not listing setting

image

Another issue that was raised is that royalties as a setting can be set only once and only while creating an NFT. And we should think about it more as an NFT setting, not necessarily a listing option - Therefore it should be set in a separate step which will be visible to the user only once while creating an NFT (it doesn't matter if this will be a buy now, auction or not for sale listing type - royalties can be set only once at the time of creation, this value cannot be edited later)

So we should put enough emphasis on the fact that this value cannot be changed later. And probably put it in a separate step from all others called NFT settings. This should address all issues regarding the royalties system.

image

So if somebody is an NFT owner, not its creator we simply don't show this step but rather in the summary of a new listing show a piece of non-editable information that royalties are "that much" and go to "this member".

Work to be done

This was requested as a top priority issue and will be tackled before other work asap. As it is a major blocker.

bedeho commented 2 years ago

I cannot review this I am afraid, due to time constraints, however, I would love it if you (e.g. in some retro) would identify the reason we did not catch this earlier.

dmtrjsg commented 2 years ago

@KubaMikolajczyk good turnaround, agree with the treatment here.

Few comments and

1️⃣ Minimal Bid Step

It does have impact on the expected revenue. I'd be puzzled to enter a reasonable default value for this input without a prompt / default value. 2% from the minimum bid would make sense as a starting point.

Here's what Ebay Proxy bidding system uses (system that is aimed at optimising seller's revenue), which is based on the current auction price, but 2% is in the range :)

Screenshot 2022-02-21 at 19 17 26

2️⃣ Scenarios

I agree with this solution, makes 100% sense! But looks like we need to swap Buy Now and Fixed price in the copy on the respective screens (use Fixed price on the stage 1, where 3 routes are offered; use Buy Now price in setting the auction up). Re-reading the handbook

Buy now price = once paid, auction stops and winner is who paid it. And seems to be named correctly in the Purchase flow

Screenshot 2022-02-21 at 18 52 06

3️⃣ Royalties

Makes sense to move it upstream in the flow and separate step seems justified. I'd call it royalties if there are no other NFT settings. Need extra focus on copy treatment for the "not for sale" flow as royalties isn't smth the user would expect as part of this flow.

There's min and max creator royalties that are defined externally and act as validation on the royalties input fields link to handbook

4️⃣ Reservation price ⚠️

Do we need to consider the adjustments for Reservation Price ❓ in teh Purchase NFT

Reservation Price: The minimum balance required for amount of any bid to be valid > So the balance of the bidder cannot be lower than reservation price, regardless of what the minimum bid is. Looks like worth adding in or have I missed prior decisions/ discussions @kdembler ?

KubaMikolajczyk commented 2 years ago

@dmtrjsg @kdembler Regarding points raised by Dmitry: 1️⃣ If we can have this logic of bid step on the system side rather than on the user side then we could resign from adding this new field and would be probably easier for most users. (I would be puzzled with it as well). Klaudiusz please tell me if you think adapting a system like this would answer our needs regarding min bid step. 2️⃣. I'm ok with a swap like that - @kdembler wdyt? 3️⃣ I'm ok with naming it royalties - it sounds better than naming it NFT settings as long as we have only royalties here to set :P 4️⃣ On rocketchat we discussed that reservation_price is called now starting_price but in the UI its actually minimum bid - because reservation price was a too complicated term to understand at a glance for the user.

kdembler commented 2 years ago

1️⃣ Our system doesn't allow this kind of dynamic bid step, it needs to be defined as a static value when the auction starts. We can use 2% of a minimal price 2️⃣ Yes, sounds good! 3️⃣ 👍 4️⃣ Yep, reservation price is an old term it seems. I think what we have in the UI currently, "Minimum bid", does perfect job to convey what this means

dmtrjsg commented 2 years ago

1 - confirmed today with Bedeho this approach ✅ 2,3,4 - 👍 👍

KubaMikolajczyk commented 2 years ago

So to confirm are we going to use only this percentage system or user will be able to set his/her own min bid step? @kdembler @dmtrjsg

dmtrjsg commented 2 years ago

@KubaMikolajczyk

We will be using 2% from the Minimum Bid input, for all auctions.

No need to have minimum bid step in the UI, but this info can be shown in the tooltip for the minimum bid field.

Copy "Minimum bid is the smallest bid that the first bidder can place on the auctioned item. The minimum bid step or the smallest increment of consecutive bids by any other bidders, is calculated as 2% from the minimum bid value provided in this field."

KubaMikolajczyk commented 2 years ago

Short video update: https://www.loom.com/share/678b5b98e4da456a9eab3a5b86719236?sharedAppSource=team_library

dmtrjsg commented 2 years ago

@KubaMikolajczyk thank you for the walkthrough.. my very small comments are here, mostly copy-related:

Screenshot 2022-02-23 at 12 30 45

Spacing in the stepper component needs to be calibrated, but I guess this was not the focus of this walkthrough and sure you have it on your list of actions ;)

Rename Buy Now -> Buy Now Price in step 3 and final review screens

Royalties are called Creator's Royalties and I think its even better, so let's use it for the step 2 naming as well ;)

Transaction Fee -> does it make sense ot have Transaction is a section header and only Fees under it? Perhaps explore putitng "Transaction fee' in the same font ?

KubaMikolajczyk commented 2 years ago

1️⃣ Spacing in stepper is fine - the same component we use everywhere - if you are not referring to spacing between each individual step but rather the fact that not all steps are fully on screen here - if this component has more steps than screen space it becomes scrollable horizontally which is already described in component page 2️⃣. Sure thing! Done 3️⃣ Alright - done! 4️⃣ Done!

KubaMikolajczyk commented 2 years ago

All work on changes is done. @kdembler @dmtrjsg Changelog with all changes: https://www.figma.com/file/xm6vBdSnQDroc57rGfGtPM/Video-workspace?node-id=1378%3A93428 Video update describing all changes: https://www.loom.com/share/b1f3f2ccaac540e8bc232a530f8c089c