koinos / koinos-contract-standards

A repository to propose, track, and discuss smart contract standards for the Koinos Blockchain.
3 stars 2 forks source link

Adds KCS-5 #13

Open sgerbino opened 3 weeks ago

sgerbino commented 3 weeks ago

Resolves #11.

sgerbino commented 3 weeks ago

All protocol buffer method arguments should be _arguments and all results should be _result. Also, we should add each of these standards to https://github.com/koinos/koinos-proto. I propose we put them in contracts/standard/kcs-*.proto. I did not completely review all of these refactors, simply normalize them so they can be reviewed individually.

sgerbino commented 3 weeks ago

KCS-2 has events listed in a different way. We should probably indicate if standards are supersets of other standards as well.

Perhaps we can add fields in the markdown header?

sgerbino commented 3 weeks ago

@joticajulian @jredbeard The events are not clearly defined on KCS-2, we should have them clearly defined on both KCS-2 and this standard (KCS-5) as well.

Can we use KCS-1 and KCS-3 as a standard way to define events and add them on both of these?

joticajulian commented 3 weeks ago

are you referring to the name of the event? or the protobuffer structure?

as far as I understand kollection.app expects to find collections.* in the name of the event to be able to track NFT changes. Maybe we should continue using them to not introduce breaking changes.

With regarding to protobuffers they use the same structure as the arguments.

jredbeard commented 3 weeks ago

are you referring to the name of the event? or the protobuffer structure? as far as I understand kollection.app expects to find collections.* in the name of the event to be able to track NFT changes. Maybe we should continue using them to not introduce breaking changes.

@sgerbino @joticajulian can confirm yes, Kollection looks through receipts for these specific event names on collection contracts:

  collections_royalties: "collections.royalties_event",
  collection_owner: "collections.owner_event",
  token_mint: "collections.mint_event",
  token_transfer: "collections.transfer_event",

I'm fine with adding this to the KCS-2 standard - that seems to make sense. Collections should emit these events. I think it makes sense to have events on all of the standards actually... it makes it easy to track things.

In general, it's great to have things like KCS-5 has where you can get all NFT's minted in a collection using an endpoint - that's useful, however, I wouldn't want to forego having events where I can follow the chain for changes. I'd much rather be able to make an API call every 3 seconds to get a new block rather than spamming the endpoint of a contract every time someone requests data about a collection. It's much fewer requests and much more efficient this way. It's not a big deal for small size websites, but, for large ones, they will want to store information in a database and serve it from there rather than relying on calls directly to nodes.

joticajulian commented 3 weeks ago

I added these extra functions to simplify the work of the developers. In this way, they will not have to setup an API to track changes in the ownership of tokens. At the same time as I discussed with you last year, I would like to introduce the possibility of storing the metadata onchain. The purpose of this feature is the same one: Improve the development experience as much as possible. In this way, the developer will not have to setup an API to resolve the metadata of the tokens. Of course this is optional, they can continue providing an API and storing the metadata offchain. It would be good if you can adapt Kollection.app for this. The process is the following: If the collection has an uri defined then the metadata is stored offchain. If the uri is undefined then the metadata is stored onchain. You can use the event of the set_metadata function to track changes there.

jredbeard commented 3 weeks ago

I added these extra functions to simplify the work of the developers. In this way, they will not have to setup an API to track changes in the ownership of tokens. At the same time as I discussed with you last year, I would like to introduce the possibility of storing the metadata onchain. The purpose of this feature is the same one: Improve the development experience as much as possible. In this way, the developer will not have to setup an API to resolve the metadata of the tokens.

Yes being able to get all NFT's from a collection is very useful, but, if it's required and you cannot get updates via events it is a non-starter. Calling a contract each and every time someone visits a page does not scale, and no large scale businesses will take us seriously if we don't have events for tracking changes. It's a vast difference in being able to simply get every block and follow the chain vs making a call to a node for every page visit.

I like the idea of optionally storing metadata on-chain for NFT's - this is totally fine and I don't have a problem eventually updating Kollection to support this. It's probably worth noting that developers don't need an API to serve metadata, most just upload the metadata json files to IPFS - but it is useful to only have to worry about image hosting rather than storing metadata in IPFS - if people want to spend their mana on that, that's totally fine. It's also more immutable than IPFS which is kinda cool - the metadata would live on forever no matter what which to me makes an NFT more valuable.

joticajulian commented 3 weeks ago

Sure, I'm not trying to get rid off the events by adding these functions. The events still exist and anyone can track changes with them. This is good for small and large businesses: Small businesses can just query the contract to get the list of tokens, they don't have to worry about extra APIs. And large businesses can optimize this by having their own API that tracks the events.

jredbeard commented 3 weeks ago

Sure, I'm not trying to get rid off the events by adding these functions. The events still exist and anyone can track changes with them. This is good for small and large businesses: Small businesses can just query the contract to get the list of tokens, they don't have to worry about extra APIs. And large businesses can optimize this by having their own API that tracks the events.

Ok sounds good then. Same page.

sgerbino commented 3 weeks ago

Please add protobuf definitions and events on both KCS-2/KCS-5 that match the current standard when you can, you can merge into this PR branch.

sgerbino commented 1 week ago

Blocked on events in the standard from @joticajulian and/or @jredbeard