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

ionic side (Order of ai commit) #2937

Open
sync-by-unito bot opened this issue Jul 25, 2023 · 30 comments
Open

ionic side (Order of ai commit) #2937

sync-by-unito bot opened this issue Jul 25, 2023 · 30 comments

Comments

@sync-by-unito
Copy link

sync-by-unito bot commented Jul 25, 2023

Expectation

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

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

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

@sync-by-unito sync-by-unito bot changed the title ionic side(Implement on 0731 sprint) ionic side (Order of ai commit) (Implement on 0731 sprint) Aug 4, 2023
@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ Sam commented:

Kenny Hung, thank you for clarification.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ Ethan Wu commented:

Sam

what we do is

(1) create an asset

(2) after 5 minute delay create commit

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ Sam commented:

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

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ Sam commented:

Kenny Hung, I would suggest not to exchange.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 7, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 23, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 23, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 23, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 23, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 23, 2023

➤ 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

  • initial registration commit
  • additional commit (miningPreference=notAllowed or aiTraining etc)

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 )

  • When asset is registered via Dashboard it has 2 commits

  • When asset is registered via capture cam it has 1 commit

    • initial registration commit

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
Copy link
Author

sync-by-unito bot commented Aug 23, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 24, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 24, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 24, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 24, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Aug 28, 2023

➤ Kenny Hung commented:

Tammy Yang (cc Sam)

^ ^ This part needs your confirmation, thanks.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 29, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Sep 1, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Sep 1, 2023

➤ 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
Copy link
Author

sync-by-unito bot commented Sep 1, 2023

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

@sync-by-unito sync-by-unito bot changed the title ionic side (Order of ai commit) (Implement on 0731 sprint) ionic side (Order of ai commit) Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

0 participants