numbersprotocol / capture-lite

A photo-sharing app with only verifiable photos and videos.
https://numbersprotocol.github.io/
GNU General Public License v3.0
25 stars 8 forks source link

ionic side (Order of ai commit) #2937

Open sync-by-unito[bot] opened 11 months ago

sync-by-unito[bot] commented 11 months ago

Expectation

Every asset when it is register from Capture Cam, it should have two commits.

  1. initial registration
  2. commit(miningPreference...)
    1. For miningPreference value, please refer gitbook
    2. the default should be "notAllowed"

┆Issue is synchronized with this Asana task by Unito ┆Created By: Kenny Hung

sync-by-unito[bot] commented 11 months ago

➤ Sam commented:

Kenny Hung, (cc: Ethan Wu) want to clarify.

Capture cam should not care about commits, right?

Capture cam should only care about

  1. Making sure user can capture image/video asset from camera or gallery.
  2. Calling backend to register asset (aka Create an asset ( https://dia-backend-dev.numbersprotocol.io/api/v3/redoc/#operation/assets_create ) handled from ionic side)
  3. Calling backend to mintAndShare (currently is handled from bubble iframe side)

So it' not clear what is expected form ionic side?

Should I set miningPreference to notAllowed from capture cam?

sync-by-unito[bot] commented 11 months ago

➤ Kenny Hung commented:

Sam (cc Tammy YangEthan WuScott Yan)

No, we want every asset from Capture service(including Cam, dashboard) to have consistent experience.

So when user takes a photo or upload from gallery, the asset should have 2 commits.

The first one(initial registration) should be the same now.

The second one should be like the asset from dashboard, the commit will show the "miningPreference" is "notAllowed"

  1. sample asset: https://nftsearch.site/asset-profile?nid=bafkreibze5ybkaceei3awt6cmttix3sq7g2ctldj4arjfolacxg7jc3nem ( https://nftsearch.site/asset-profile?nid=bafkreibze5ybkaceei3awt6cmttix3sq7g2ctldj4arjfolacxg7jc3nem )
  2. sample assetTree: ipfs-pin.numbersprotocol.io/ipfs/bafkreihkxk2soz242fxgyyqmghpmbt7prc4buo7ffrxg5jinsoqz4a6qri ( https://ipfs-pin.numbersprotocol.io/ipfs/bafkreihkxk2soz242fxgyyqmghpmbt7prc4buo7ffrxg5jinsoqz4a6qri )
  3. detail format please refer https://docs.numbersprotocol.io/introduction/numbers-protocol/defining-web3-assets/assettree ( https://docs.numbersprotocol.io/introduction/numbers-protocol/defining-web3-assets/assettree )
sync-by-unito[bot] commented 11 months ago

➤ Sam commented:

Kenny Hung, thank you for clarification.

sync-by-unito[bot] commented 11 months ago

➤ Sam commented:

Ethan Wu, I have no access to bubble dashboard. So I need to clarify the following.

What API is called from Capture Dashboard during creation ( https://dashboard.captureapp.xyz/version-test/main?tab=createnft ).

Tammy Yang, would be nice to have access to Capture Dashboard so I could inspect the API call or the logic so I can keep ionic implementation as close as possible to bubble implementation to keep experiences as consistent as possible.

sync-by-unito[bot] commented 11 months ago

➤ Ethan Wu commented:

Sam

we are calling the following:

  1. Create An Asset: https://dia-backend.numbersprotocol.io/api/v3/assets/ ( https://dia-backend.numbersprotocol.io/api/v3/assets/ )
  2. Commit : https://docs.numbersprotocol.io/developers/commit-asset-history/commit-via-api ( https://docs.numbersprotocol.io/developers/commit-asset-history/commit-via-api )
sync-by-unito[bot] commented 11 months ago

➤ Ethan Wu commented:

Sam

what we do is

(1) create an asset

(2) after 5 minute delay create commit

sync-by-unito[bot] commented 11 months ago

➤ Sam commented:

Ethan Wu, thank you for clarification. The screenshot is very helpful. 🙏

sync-by-unito[bot] commented 11 months ago

➤ Sam commented:

Ethan Wu, is it a requirement (or a limitation) to call create commit after 5 minute or it's okay to do create commit after asset registration immediately?

sync-by-unito[bot] commented 11 months ago

➤ Ethan Wu commented:

Sam there needs to a delay because what happens is if you call the immediately what you get is the commit appearing before initial registration

sync-by-unito[bot] commented 11 months ago

➤ Ethan Wu commented:

Sam i have tested this many times. the 5 minute duration can maybe reduced maybe.

most of the time people don't look at the asset profile immediately so its ok if the commit appears a little later.

sync-by-unito[bot] commented 11 months ago

➤ Sam commented:

Kenny Hung, (cc: Tammy Yang).

I would suggest to move ionic side (Order of ai commit) (Implement on 0731 sprint) ( https://app.asana.com/0/0/1205120894428797 ) to next milestone.

Because bubble side has built in scheduler that can trigger certain actions after certain time. However on ionic side we need to implement it manually and test it carefully. Also ionic side should consider offline cases.

Since v230808-capture-app-ionic-launch ( https://app.asana.com/0/inbox/1202182817452381/1205132295508392/1205217942354076 ) release is tomorrow I don't think this task can be implemented and test in one day.

Please let me know what you think.

sync-by-unito[bot] commented 11 months ago

➤ Kenny Hung commented:

Sam (cc Tammy YangScott Yan )

Okay, Shall we exchange [FR] Remove options from list on ionic asset page ( https://app.asana.com/0/1201083422707776/1205200411494663/f ) with this task?

If not, it's also fine.

sync-by-unito[bot] commented 11 months ago

➤ Sam commented:

Kenny Hung, I would suggest not to exchange.

sync-by-unito[bot] commented 11 months ago

➤ Kenny Hung commented:

Wait, I notice this task is in our roadmap ( https://docs.google.com/spreadsheets/d/14xP-HuGdiS5UExmn3ebMKpjGEqqbTPEa5GJCOLM0WSU/edit?pli=1#gid=1112418286 ), we need Tammy Yang's confirmation. (Push this item into next sprint.) (cc Sam)

sync-by-unito[bot] commented 11 months ago

➤ Tammy Yang commented:

Thank you for sharing the information with me ❤️. Let's push it to the next sprint and make it right.

sync-by-unito[bot] commented 10 months ago

➤ Sam commented:

Kenny Hung, this is low chance to happen.

What if user has enough for initial registration but by the time I call commit(miningPreference=notAllowed) user has not enough NUMs.

Should I notify user or just wait until user top up balance?

sync-by-unito[bot] commented 10 months ago

➤ Kenny Hung commented:

Sam It's a good question! (cc Tammy Yang)

I suggest notifying user.

How do you think? Tammy YangSam

sync-by-unito[bot] commented 10 months ago

➤ Sam commented:

Kenny Hung, Tammy Yang

Agree we need to let users know where his NUMs are going.

Comment by @Ethan Wu on ionic side (Order of ai commit) (Implement on 0731 sprint) ( https://app.asana.com/0/0/1205120894428797/1205217942354068/f ) In Dashboard we do commit(miningPreference=...) 5 minutes after initial registration. Lets refer to then as pending commits in this context.

Use case #1:

User takes photos A, B, C and D in 1 minute and all those NUMs are waster for initial registration then by the time capture-cam(dashboard) calls commit(miningPreference=notAllowed) user might not have enough NUMs.

We tell user that "Hey you don't have NUMs for commit(miningPreference=notAllowed)" most users might not understand what is it and why they need it. For them it might feel like hidden fee.

Back to use case #1:

User goes okay I will top up NUMs and try to call network action "Mint & Share" on photo D.

So my question to Tammy Yang before "Mint & Share" (any network action that might consume NUMs) on photo D is invoked should we check wether

  1. commit(miningPreference=notAllowed) was done on photo D yet
  2. should we check if we have other pending commits (in our case maybe commit(miningPreference=notAllowed) was not called on photo C yet)

If yes then just scheduling is not enough we should also keep track of pending commits (list of commit(miningPreference=...)).

Also can this be the case in Dashboard Ethan Wu?

sync-by-unito[bot] commented 10 months ago

➤ Kenny Hung commented:

Tammy YangEthan WuSam (cc Scott Yan)

Although we have discussed this previously ( https://app.asana.com/0/1201016280880500/1204967077542160/1204086179720532/f ), perhaps we can include the miningPreference in the first commit (initial registration) to avoid more special cases like Sam mentioned. (This part might need backend support?)

This is my proposal, this time we should just notify user the commit is incomplete when in this small case ( https://app.asana.com/0/0/1205120894428797/1205330940994102/f ).

sync-by-unito[bot] commented 10 months ago

➤ Sam commented:

James Chien, (cc: Olga) is it bad idea to use generate_iota_transaction ( https://github.com/numbersprotocol/storage-backend/blob/75f9ab91ab97257758d3e7284d86f8b27e626069/storage_backend/apps/assets/tasks.py#L112 ) to keep asset post creation commits consistent?

As I understood eventually assets expected to have 2 commits as mentioned ( https://app.asana.com/0/1201016280880500/1204967077542160/1204086179720532 ) by Tammy Yang

According [issue] The same is for registering assets, but the number of commits generated by the capture app and the dashboard is different ( https://app.asana.com/0/1201016280880500/1204967077542160/f )

As I noticed same "additional commit(miningPreference=...)" should be implemented twice one in Dashboard and capture cam.

James Chien (cc: Olga) Why not offload additional commit(miningPreference=...) to generate_iota_transaction ( https://github.com/numbersprotocol/storage-backend/blob/75f9ab91ab97257758d3e7284d86f8b27e626069/storage_backend/apps/assets/tasks.py#L112 ) or other backend workflow in since 2 commits will be commited after every asset creation anyway?

James Chien (cc: Olga) Is it because storage-backend will be tightly coupled with capture-cam and dashboard but we want to keep storage-backend client agnostic in case other 3rd parties want to rely on storage backend?

sync-by-unito[bot] commented 10 months ago

➤ James Chien commented:

Backend does have other clients, other than Capture Cam and Dashboard. The SDK released to project partners are using backend to register assets, so unless the 2-commits behavior are expected to be a default behavior for backend, we cannot just change it to always make 2 commits.

From my point of view, the most ideal solution is backend should provide a configuration like “registration_commit_preset” and offer a few options to support all use cases. If [Discussion] Update Registration contract to support batch commit ( https://app.asana.com/0/1201016280880508/1205065060064612 ) is implemented it could also be used in backend to create multiple commits at the same time.

(cc Tammy Yang I think tammy should also be involved in the discussion since it’s more design decisions than technical good or bad)

sync-by-unito[bot] commented 10 months ago

➤ Olga commented:

I agree with Kenny Hung's idea. We can add an extra custom commit during the initial registration to prevent the need for additional special cases, as Sam mentioned. Also, the api ( https://api.numbersprotocol.io/api/v3/redoc/#operation/assets_create ) field nit_commit_custom has been implemented after the initial discussion.

sync-by-unito[bot] commented 10 months ago

➤ Ethan Wu commented:

Kenny Hung with what Olga mentioned, we can just leverage the nit_commit_custom field to add extra information (license, generatedBy, aiTraining, etc.) in the initial registration commit.

sync-by-unito[bot] commented 10 months ago

➤ Sam commented:

Comment by @Ethan Wu on ionic side (Order of ai commit) (Implement on 0731 sprint) ( https://app.asana.com/0/0/1205120894428797/1205335361765950/f ) that's supper cool. Thanks Ethan Wu for explaining in daily sync.

So as far as I understood if we leverage nit_commit_custom then

sync-by-unito[bot] commented 10 months ago

➤ Kenny Hung commented:

Tammy Yang (cc SamEthan WuOlgaJames ChienScott Yan)

This part needs your confirmation. There are two options.

  1. Let the first commit(initial registration) will include "the miningPreference" default setting.
  2. Keep the same way as dashboard (every asset will have two commits.) It will face the condition which Sam mentioned ( https://app.asana.com/0/0/1205120894428797/1205331896255082/f ).

Currently, we prefer #1, because the UX is better & could avoid the potential risk.

sync-by-unito[bot] commented 10 months ago

➤ Kenny Hung commented:

Tammy Yang (cc Sam)

^ ^ This part needs your confirmation, thanks.

sync-by-unito[bot] commented 10 months ago

➤ Kenny Hung commented:

Tammy YangSam (cc Scott Yan)

Due to v230822-capture-cam-ionic-internal ( https://app.asana.com/0/0/1205228861926299 ) is internal release, I'll move this task into v230905-capture-cam-ionic-launch ( https://app.asana.com/0/0/1205341665472656 ).

sync-by-unito[bot] commented 10 months ago

➤ Kenny Hung commented:

Tammy Yang When you have time, please help to confirm this part.

There are two options.

  1. (We prefer this.) Let the first commit(initial registration) will include "the miningPreference" default setting. (reason of suggestion ( https://app.asana.com/0/0/1205120894428797/1205331757097609/f ).)
  2. Keep the same way as dashboard (every asset will have two commits.) It will face the condition that Sam mentioned ( https://app.asana.com/0/1201016280880500/1205120894428797/1205331896255082 ).
sync-by-unito[bot] commented 10 months ago

➤ Tammy Yang commented:

SamKenny Hung for product feature suggestion, our principle is "only do what is truly needed"

With this principle, I do not see any need to update the commit strategy of Capture Cam yet. Let's not implement this for now. It can be a Medium feature request (I also don't see a need for Capture Cam user why this has to be Critical).

sync-by-unito[bot] commented 10 months ago

➤ Tammy Yang commented:

Please also remember, two commits meaning two payments for users. We also need to think about if this is reasonable for users.

With the "preferred" option, user has no clue what choice they have made and we are going to add additional commit for them with no reasons. To me, it is a "pure technical" choice (which is not ideal in most cases) instead of a "use driven" choice.