refactor(contract-standards): Refine public API of NFT events

[austinabell]

Refactor of the previous API which had way too much public surface area and opportunities to misuse. Also moves the types so that they are under near_contract_standards::non_fungible_token::events since that makes more sense than having all events in a separate module for all standards, which it was before.

The main thing I'm not sure about is if constructors should be kept as a replacement or in addition to exposing fields of each log type.

Also curious why Nft Burn is not part of standard, because it doesn't seem possible to determine if the token was burned from the external API so it relies on someone to manually implement if they want the burn event log? @telezhnaya @frol maybe you have context here, is it just not necessary?

[telezhnaya]

it doesn't seem possible to determine if the token was burned from the external API

That was our pain on the Indexer side. While we could parse nft_transfer calls somehow to extract the info about transfers, we had no chance collecting mint/burn. That was the main reason to implement events.

I don't know why it's not in the standard.

[matklad]

• Let's drop Data suffix from public types -- if every type is "Data", than "Data" word adds zero bit of information.

• I am not super thrilled about inconsistency between .emit() method and emit_nft_burns top level functions. What about

  impl NftBurn {
      pub fn emit(self) { ... }
      pub fn emit_many(events: &[NftBurn]) { ... }
  }

  ? Not thrilled about this either: NftBurn::empt_all(&[NftBurn { ... }]) feels stuttery.

• code wise, I suggest putting all the public stuff on top, and then all the private stuff. It took me some seconds to figure out what is and what isn't public API.

• I am torn about making all the fields public and using things like &'a [&'a str]: if what you have is a Vec<String>, than that's an extra allocation! And, if in the end of the day we just serialize to JSON, one can imagine an iterator-based API which does everything on the fly. I think, on balance, the current API is still the best one! +1 on not having new methods for data.

• I am unsure about versioning story here. There's a delicate interplay between the semantic version of the crate, and the version of the standard itself. If we expect standards themselves to evolve, it'd be better if each standard was a separate crate.

• We might consider making the stuff : Clone.

• The fact that types are publicly : Serialize is a compatibility hazard. If, in the future, we decide that we want to hand-write serialization for performance/code size reasons, removing serde impl would be semver breaking. If we want to be super-cautious about this, we might want to have a parallel hierarchies of public !Serde / private Serde types. Probably isn't worth it though.

[austinabell]

• Let's drop Data suffix from public types -- if every type is "Data", than "Data" word adds zero bit of information.

I was thinking the same, I just think the name on its own might be confusing to a developer who might think they need to use this to actually do the transfer, burn, mint. Anyway, I've changed to drop Data suffix.

• The fact that types are publicly : Serialize is a compatibility hazard. If, in the future, we decide that we want to hand-write serialization for performance/code size reasons, removing serde impl would be semver breaking. If we want to be super-cautious about this, we might want to have a parallel hierarchies of public !Serde / private Serde types. Probably isn't worth it though.

Yeah, I think I'll keep as-is for conciseness for now. I'm not too worried since we can still change the internal implementation while keeping serde Serialize on this type. If we are removing serde altogether, there will have to be breaking changes throughout the crate anyway.

• I am not super thrilled about inconsistency between .emit() method and emit_nft_burns top level functions. What about

  impl NftBurn {
      pub fn emit(self) { ... }
      pub fn emit_many(events: &[NftBurn]) { ... }
  }

  ? Not thrilled about this either: NftBurn::empt_all(&[NftBurn { ... }]) feels stuttery.

Yeah, I'm pretty indifferent about this, but this suggestion might be slightly easier to use in some cases, I've applied it.

[ChaoticTempest]

Not a fan of the NftTransfer { ... }.emit() syntax but this seems to be the least obtuse way of doing it. Was thinking maybe NftTransfer::emit(...) instead but its purely sugar

[austinabell]

Not a fan of the NftTransfer { ... }.emit() syntax but this seems to be the least obtuse way of doing it. Was thinking maybe NftTransfer::emit(...) instead but its purely sugar

You can do this syntax if you want, but to have the params inside typed it would need to be NftTransfer::emit(NftTransfer{..}) unless you're suggesting some sort of API that takes positional args, but I would then disagree that it is more intuitive because of all the similarly typed fields.

[frol]

Also curious why Nft Burn is not part of standard, because it doesn't seem possible to determine if the token was burned from the external API so it relies on someone to manually implement if they want the burn event log?

@austinabell Do you ask about nft_burn being a standardized function name? Let's look at this problem from the following perspective: defining the standard, we want to define the mandatory interfaces and standard cannot define mint and burn rules (imagine NFT platform that mints new NFTs only if something happens, e.g. when fiat payments go through, so the "mint" function needs to receive some arbitrary inputs or even be restricted to be only called by a service account; same story about "burn") - "mint" and "burn" functions are not generic, so they cannot be adopted universally across any Wallet out there, while "transfer" is quite generic across the board. If we want to track the fact that something got minted (assuming we don't have events standard), we would need to standardize the structure of the input arguments and/or return value of mint and burn methods and developers would most probably struggle complying with those requirements potentially creating excessive cross-contract calls just to ensure that a method with a specific name and arguments is called.

TL;DR:
Method names are standardized for integration with some tool that allows user to perform some standard actions (user can usually easily transfer the ownership of a token anywhere, but minting and burning is usually quite custom).
Events are standardized for passive observation and further notification/aggregation/etc.