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

NFT NEP-171 require having minting/burning interface #254

Closed
telezhnaya opened this issue Sep 6, 2021 · 33 comments · Fixed by #256
Closed

NFT NEP-171 require having minting/burning interface #254

telezhnaya opened this issue Sep 6, 2021 · 33 comments · Fixed by #256

Comments

@telezhnaya
Copy link
Contributor

Our users ask for supporting FT/NFT in the tools such as Explorer and Wallet. We want to show FT/NFT balance and the history of all FT/NFT movements.

To achieve that, we need to track all the FT/NFT balance-changing events. They include minting, transferring, and burning of the tokens.
For now, it's only possible to track transfers.

While digging into FT implementation, we've found internal_transfer method, it looks like what we need. But it's the details of one exact implementation, and it's not possible to catch these calls, it's deep inside the contract. We can only see public interface calls such as ft_transfer.

It’s a critical moment for supporting FT/NFT in our and 3rd-party tools, our data would be incomplete in other cases.
We should include minting/burning in NEPs. Without that, we are blocked, we can't show valid FT/NFT balances.

Please share any thoughts, including other workarounds to solve the issue.

Previous issue #250
Issue with collecting balance-changing events in Indexer for Explorer near/near-indexer-for-explorer#153

@chadoh @mikedotexe @mattlockyer @MaximusHaximus @jberrytech @frol

@telezhnaya
Copy link
Contributor Author

@frol
Copy link
Collaborator

frol commented Sep 6, 2021

I want to highlight that currently Wallet uses some hacks that only allows it to find potential FT / NFT contracts that the account might be involved with and then tries to fetch the balance from there. There are several problems with that:

  1. It is not reliable - if the person only mints the tokens, that asset won't be detected (e.g. wNEAR uses a method called near_deposit to mint tokens)
  2. It is not efficient - Wallet scans JSONB column inside PostgreSQL database and with the grows of the number of transactions it will just timeout one day
  3. It cannot get a history of operations and thus limited to only displaying the current balance

Explorer is also blocked on it since we want to display a feed of events instead of just pure transactions (e.g. when FT is transferred, the receiver account might not be even touched b native NEAR receipts and thus Explorer won't display those incoming transfers in the first place)

It is a complex topic, but we want to raise it sooner rather later to benefit across the ecosystem and make NEAR user and developer friendly platform!

@frol
Copy link
Collaborator

frol commented Sep 6, 2021

@jberrytech Hey, Jim, Node Interfaces team has not been part of FT and NFT standards, but to support our end-user needs we dived into the current status of the things and found that the things are quite in flux and we need to have some decision made here. At the current stage, Wallet and Explorer are blocked on Indexer for Explorer, and Indexer for Explorer is blocked on the standards being completed and followed by the dApp developers.

@frol
Copy link
Collaborator

frol commented Sep 7, 2021

I had a discussion today with @ilblackdragon and it seems that there is a set of minimum requirements for FT and NFT tokens to make them useful across the ecosystem (I will use FT as a simpler example):

  1. Wallet-like applications should be able to list all the assets (missing spec for minting)
  2. Wallet-like applications should be able to inform users about the trace of the assets (missing spec for minting and burning)
  3. Wallet-like applications should be able to get the current status (ft_balance_of view-method in FT standard)
  4. Wallet-like applications should be able to transfer assets (ft_transfer change-method in FT standard)

Originally, I wanted to rely on called method names (ft_transfer and ft_resolve_transfer are great!), but it turned out that it is not realistic to expect to have a single name for minting operation, and thus it brings us back to the idea of emitting Events (logs) during the execution. I would love to hear the inputs about the idea of using logs with machine-readable format for this purpose.

@telezhnaya
Copy link
Contributor Author

telezhnaya commented Sep 7, 2021

  1. Wallet-like applications should be able to get the current status

For NFT, spec is also missing here.
We only have the mapping token -> user (nft_token function).
We should also have mapping user -> all their NFTs.

UPD: we actually have, so everything is OK here
https://nomicon.io/Standards/NonFungibleToken/Enumeration.html

@MaksymZavershynskyi
Copy link
Contributor

Are you saying that minting and burning of the tokens do not emit the corresponding logs according to the standard?

@frol
Copy link
Collaborator

frol commented Sep 7, 2021

@nearmax I am saying that the standards (FT and NFT) do not define logs, and our (Node Interfaces team) ask is to have those logs defined as a standard, so at least future contract implementations can adopt it.

@MaksymZavershynskyi
Copy link
Contributor

I agree, it is critical that FT and NFT standards define logs, given that we currently do not have proper Ethereum-like events. Ethereum events are part of the contract ABI, along side method signatures, and various integration tools heavily rely on their standardization to work with the contracts. E.g. see minting event https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.1.0/contracts/token/ERC20/ERC20.sol#L236 CC @jberrytech @DiscRiskandBisque @mikedotexe

@mikedotexe
Copy link
Contributor

I agree, it is critical that FT and NFT standards define logs, given that we currently do not have proper Ethereum-like events. Ethereum events are part of the contract ABI, along side method signatures, and various integration tools heavily rely on their standardization to work with the contracts. E.g. see minting event https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.1.0/contracts/token/ERC20/ERC20.sol#L236 CC @jberrytech @DiscRiskandBisque @mikedotexe

Just got off a call this morning with Mintbase, Paras, Matt Lockyer, and Austin. Agree we need logs as events. The time has come. I believe standards will likely need far more attention and prioritized org-wide.

@frol
Copy link
Collaborator

frol commented Sep 10, 2021

I want to cross-post by comment here: #256 (comment)

Hey, I feel that we should define our standards to be as slim as possible. This standard extension still makes sense, but from what I have heard, there are too many ways of how tokens can be minted/burned, so it feels that those cannot be limited to just certain method names (it can be implemented that way, but it might just feel unnatural.

As per my #254 (comment), I feel there are just 3 high-level scenarios that need to be standardized:

  1. Observability of minting, transfer, burning events
  2. Interface to transfer assets
  3. Interface to get status, i.e. balance (for FT) and token-by-owner and owner-by-token (for NFT)

Batch operations are great, but I would keep them an extension. I find this current proposal to be quite complicated in terms of the interface, and at the same time, it might limit some other use-cases. Batch interfaces are not required by any of the listed scenarios, and while it might address my concerns about observability, I have heard that it is not realistic to expect people to create workarounds with cross-contract calls just to make sure to have a call with nft_batch_* as a method name. I feel that log-events is the way to go, and we can easily document that there can be more than a single mint/transfer/burn events in a single execution and that will completely solve (1) with just a minor tweak to the existing implementations and it does not interfere with this batch extension.

Thus, I suggest we discuss the observability in #254 and consider if these interfaces still need to be defined.

@ghost
Copy link

ghost commented Sep 14, 2021

Thanks @frol for elaborating previously.

If we talk about minting many NFT marketplaces have this concept of royalty. Only the specification to payout royalties is standardised, not the royalty. So you can have standard logs but a fragmented NFT ecosystem. Why every NFT-marketplace has different capabilities.

I propose a royalty standard which will help minting spec. This is not standardised in general because payouts of fractional royalties is natively infeasible for many blockchains. NFTs should have royalty standard because majority use case serves that purpose.

This is brief draft of the spec - https://nep-docs-typescript-site.netlify.app/interfaces/nft.proposalbatch.royaltyargs
@mikedotexe @mattlockyer @telezhnaya

@zcstarr
Copy link
Contributor

zcstarr commented Sep 14, 2021

@frol for events perhaps we could be more terse with just 2 events approvals and transfers. Where transferring from a null address is minting, and transferring to a null address is burning. Then indexers and consumers can just watch those events to understand what's happening with the contract. Across the pond in eth land this is what happens for 721/1155. Curious what people think about this?

@zcstarr
Copy link
Contributor

zcstarr commented Sep 15, 2021

Curious here just watched the recap of the meeting. I have questions with regards to #245. I think @telezhnaya you mentioned that events being emitted from unknown methods is problematic? Is it that events can only be emitted from known methods ?

Is that what's driving extending the standards to include mint and burn, despite the addition of events? With erc-1155 many methods and even I think across ethereum, many methods use all manner of name and method signatures to mint tokens.

For instance, one anti-NFT bot measure that's gaining traction, takes a signed nonce as an argument, to verify, before the user is able to mint. I think there's a little more diversity in the minting.

So my mild concern about would be something like the buy methods here from 2 popular NFT contracts that get cloned a bit.

Examples:
https://etherscan.io/address/0x219b8ab790decc32444a6600971c7c3718252539#writeContract Sneaky Vampire Syndicate
https://etherscan.io/address/0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d#writeContract BoredApes

//Sneaky Vampire Syndicate
buy(hash,signature,nonce,quantity) // mints but I think this would be problematic with a standard mint
preSale(quantity) // probably not problematic 
//Bored Ape Yacht Club
mintApe(quantity) //mints 
reserveApes() // mints as well , probably problematic with a standard shape could ignore quantity field

In the above examples minting functions are varied and called in multiple context with different arguments that make sense for the contract. Would the current events proposal and minting indexing be able to handle something like this?

If mint and burn are part of the standard then does that hamstring use cases like the above and what do we gain?
@frol @mikedotexe @telezhnaya any thoughts ?

@mikedotexe
Copy link
Contributor

Thanks @frol for elaborating previously.

If we talk about minting many NFT marketplaces have this concept of royalty. Only the specification to payout royalties is standardised, not the royalty. So you can have standard logs but a fragmented NFT ecosystem. Why every NFT-marketplace has different capabilities.

I propose a royalty standard which will help minting spec. This is not standardised in general because payouts of fractional royalties is natively infeasible for many blockchains. NFTs should have royalty standard because majority use case serves that purpose.

This is brief draft of the spec - https://nep-docs-typescript-site.netlify.app/interfaces/nft.proposalbatch.royaltyargs
@mikedotexe @mattlockyer @telezhnaya

Thanks for this. I'm having a hard time understanding what specifically is needed on the royalty standard.
If a person wants to see the royalties of a token, this method is available:
fn nft_payout(&self, token_id: String, balance: U128, max_len_payout: u32) -> Payout;

Maybe I need a bit of a user story or a practical example for me to understand. Forgive me, as I'm not working with NFTs every day anymore, not for 8 or so months. :)

@mikedotexe
Copy link
Contributor

mikedotexe commented Sep 16, 2021

Catching up on all of this and it's a heavy amount to context-switch to. I think as we iterate on the NEP process, it's going to be most helpful for folks who have firsthand experience of NFT and indexing issues to really drive the suggestions. I think I'm almost caught up but will have to learn more. I might be most helpful reviewing other peoples' ideas, but will be the author and open a pull request if that's the best way forward. Here's my take:

Fungible tokens and non-fungible tokens are supposed to have multiple extension standards. Even ERC-721 for NFTs has metadata as an optional extension standard, even though it's hard to imagine an NFT without it. In a similar vein, there will be fungible tokens that simply never mint new tokens, like when the token supply is set upon initialization of the contract and should never change. Burning is a little different for us simply because we have state storage and an individual can opt to "remove themself" as a key value pair. If that person decides to forcefully ragequit a fungible token contract and has a non-zero balance, we must determine what to do with their tokens. Perhaps it goes to the "token owner" and the total supply remains unchanged, or perhaps it disappears (burns) and total supply is reduced. This is why we have this section in the fungible token example which utilizes the near-contract-standards:

    fn on_account_closed(&mut self, account_id: AccountId, balance: Balance) {
        log!("Closed @{} with {}", account_id, balance);
    }

    fn on_tokens_burned(&mut self, account_id: AccountId, amount: Balance) {
        log!("Account @{} burned {}", account_id, amount);
    }
}

near_contract_standards::impl_fungible_token_core!(Contract, token, on_tokens_burned);
near_contract_standards::impl_fungible_token_storage!(Contract, token, on_account_closed);

Source: https://github.com/near/near-sdk-rs/blob/072b47779fcb219f468977bd7e727b4a83d9bd44/examples/fungible-token/ft/src/lib.rs#L81-L91

That was a little bit of a tangent but the punchline is, "people don't have to have explicit minting or burning functions" and I believe this should be an optional extension standard, both for FT and NFT.

Next, I believe indexers like Wallet and Explorer must be able to parse both:

  1. Transaction/Promise data (looking for ft_ and nft_ methods being called that are from the standards)
  2. Event logs, which are being invented and suggested as a response to this issue.

I want to say that explicitly, and if someone reading this believes we should have only one, please speak up, but I don't think that's the case.

Now, we have to decide if mint and burn should be parsed by:

  1. Method name/signature
  2. Event logs
  3. Either #1 or #2

I believe it should be either #1 or #2. If the consensus in this discussion includes #1, then let's talk about what that method signature looks like.

As Max pointed out, the OpenZeppelin implementation contains minting and burning, and here's what NFT mint looks like:

function _mint(address to, uint256 tokenId) internal virtual {

Source: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/e2876b947db9ad79f0bdb0d8b044de3803cd96e9/contracts/token/ERC721/ERC721.sol#L333

I see that Pluminite included more arguments:

pub fn nft_mint(
    &mut self,
    token_id: Option<TokenId>,
    metadata: TokenMetadata,
    perpetual_royalties: Option<HashMap<AccountId, u32>>,
    receiver_id: Option<ValidAccountId>,
    token_type: Option<TokenType>,
) {

Source: https://github.com/zavodil/pluminite-ui/blob/575ea93b83fdf18b748dbe2719f016dc76991d72/contract/src/mint.rs#L6-L13

I see Paras is doing something a bit different, that seems to deal with token series, perhaps more adjacent to a multi-token:

pub fn nft_mint(
    &mut self, 
    token_series_id: TokenSeriesId, 
    receiver_id: ValidAccountId
) -> TokenId {

Source: https://github.com/ParasHQ/paras-nft-contract/blob/036c4596017477e8a1100c4736197aea827efc35/paras-nft-contract/src/lib.rs#L260-L264

Looks like Mintbase is doing batch mint.

Given the above scenarios, and if we all agree that one way to have observability of new tokens is via method name/signature, then perhaps what we need is three arguments, where the third is optional and free-form. Perhaps what OpenZeppelin is doing, by having only two arguments for the receiver and token ID, will not be as future-proof as we'd like and we'd prefer the option to provide extra detail about this mint.

Mint function suggestion

// This method is expected to mint one NFT. If custom logic exists to mint more than one, 
//    events must be used in order for indexers to pick this up.
// Arguments:
// * `receiver_id`: the valid NEAR account receiving the newly-minted token.
// * `token_id`: the token ID to mint.
// * `msg`: specifies information needed during the minting process.
//    It is up to the contract to parse this extra data.
//    If it contains data that is intended to be indexed, it must use an event.
function nft_mint(
  receiver_id: string,
  token_id: string,
  msg: string|null,
) {}

#2 approach with events (using logs) is still in discussion but as of the latest meeting, this is the structure that was proposed:

Event log suggestion

EVENT_JSON:{
  "standard": "nep171",
  "version": "1.0.0",
  "event": "mint",
  "data": [
    {
      "receiver_id": "frol.near",
      "token_id": "NEAR:mainnet/nep171:gcp.mintbase1.near
 :HMSu9SnnW9LiVvVDemLsGH_jhn2NZQNAuxUvqaWMOes"
    },
    {
      "receiver_id": "mike.near",
      "token_id": "19"
    }
  ],
}

The above suggestion has the event key which should likely correspond to some convention, probably:

  1. If we chose #1 or #3 from before, then the method name without the prefix. (Or if it's simpler, just the method name, in which case replace mint with nft_mint in the above JSON block.)
  2. If we chose #2 from before, mint is fine or whatever we want, just as long as we're in agreement.

Lastly, I think I heard some concerns about people "misusing event logs" and possibly not doing it right. Just as a reminder, standards are written for the "good actors" and there's nothing we can do to stop anonymous, malicious behavior. All we have the power to do is provide suggestions that bring order to the collaboration between good actors. I think it's a great instinct to try to determine what might go wrong, but we can never force contract authors to implement according to standards or to play nice. The community and market will determine which contracts, markets, and dApps will survive, and those will be the ones that are orderly and act as expected.

@telezhnaya
Copy link
Contributor Author

@zcstarr
Thank you for the examples!

I think @telezhnaya you mentioned that events being emitted from unknown methods is problematic? Is it that events can only be emitted from known methods ?

@mikedotexe provided the answer much better than I can give. 😄 But I want to add few words from Indexer's perspective.
Choosing exact method names for minting/burning will simplify and speed up the work of Indexer a lot.

Indexer still doesn't have any solution for FT/NFT, but I've made a draft for FT. We go through all function calls, filter from them ft_* and try to do something with this data. If we have to parse ALL logs from all functions, that would be enormous amount of work, most of it would be useless.

By the way, I'm OK with logs + the fixed function name with any parameters as you wish. This is enough from our side to identify needed function call, this gives the freedom to developers as @zcstarr was asking for. The only concern here is that it can't be strictly defined in NEP, I never seen such conditions before. @mikedotexe what do you think?

If we decide to choose the solution only with logs (I will be upset but anyway), could we mention in NEP that the method should have ft_ or nft_ prefix? It will allow us not to parse all the function calls.

@mikedotexe

In a similar vein, there will be fungible tokens that simply never mint new tokens, like when the token supply is set upon initialization of the contract and should never change.

New interesting scenario for Indexer to handle this correctly, thank you for mentioning that.
Do we have a plan to say something about this process in NEP?
Though, I don't think it's a problem to ask for balance for the contract itself explicitly on a creation moment.

@robert-zaremba
Copy link
Contributor

I've created a PR for the minting standard. We implemented it in Cheddar.

I think burning should be a separate trait. Not every coin will support burning.

@robert-zaremba
Copy link
Contributor

if the person only mints the tokens, that asset won't be detected
and
Wallet-like applications should be able to list all the assets (missing spec for minting)

We made a PR for wallet which was merged and the following (mentioned above) #249 for the mint spec.

@zcstarr
Copy link
Contributor

zcstarr commented Sep 17, 2021

@mikedotexe great write up! I think you mentioned a very good point about account storage management burning tokens, additionally within the ecosystem I think there are a lot of new use cases and usage patterns, where a contract's burn and mint might happen in the same method. One emerging pattern is combining NFTs as well as burning NFTs to mint NFTs, so it's worth considering context.

Do we think this will be a permanent indexing drag, where log filtering will always be not performant, I think
https://gov.near.org/t/ethereum-like-logs-on-near/3509/5 mentioned subscriptions to topics and things like that is that still being pursued @telezhnaya , and if we had that would that change anything about the feasibility of event logging from anywhere within the contract?

@ghost
Copy link

ghost commented Sep 19, 2021

Thanks @frol for elaborating previously.
If we talk about minting many NFT marketplaces have this concept of royalty. Only the specification to payout royalties is standardised, not the royalty. So you can have standard logs but a fragmented NFT ecosystem. Why every NFT-marketplace has different capabilities.
I propose a royalty standard which will help minting spec. This is not standardised in general because payouts of fractional royalties is natively infeasible for many blockchains. NFTs should have royalty standard because majority use case serves that purpose.
This is brief draft of the spec - https://nep-docs-typescript-site.netlify.app/interfaces/nft.proposalbatch.royaltyargs
@mikedotexe @mattlockyer @telezhnaya

Thanks for this. I'm having a hard time understanding what specifically is needed on the royalty standard.
If a person wants to see the royalties of a token, this method is available:
fn nft_payout(&self, token_id: String, balance: U128, max_len_payout: u32) -> Payout;

Maybe I need a bit of a user story or a practical example for me to understand. Forgive me, as I'm not working with NFTs every day anymore, not for 8 or so months. :)

Whenever you mint an NFT, on a store, it is store's responsibility to implement payout - value is tied to store not NFT. How will store A know to pay me when store B keeps this information. Marketplace interoperability not going to happen like this. My thought.

@ghost
Copy link

ghost commented Sep 27, 2021

@frol I am proposing this as event logs for nft_batch_mint, nft_batch_burn and nft_batch_transfer

// nft_batch_mint
{EVENT_JSON:{
  "standard": "nep171",
  "version": "1.0.0",
  "event": "nft_batch_mint",
  "data": [
    {
      "account_id": "frol1.near",
      "token_id": "0"
    },
    {
      "account_id": "frol2.near",
      "token_id": "very_long_token_id"
    },
  ]
}}

// nft_batch_burn
{EVENT_JSON:{
  "standard": "nep171",
  "version": "1.0.0",
  "event": "nft_batch_burn",
  "data": ["1","2","3"]
}}

// nft_batch_transfer
{EVENT_JSON:{
  "standard": "nep171",
  "version": "1.0.0",
  "event": "nft_batch_transfer",
  "data":  [
    {
        sender_id:"sender.near",
        "to":"receiver1.near",
        "token_id":"0"
    },
    {
        sender_id:"sender.near",
        "to":"receiver2.near",
        "token_id":"very-long-token-id"
    },
  ]
}}

Due to the 16k string limitation, different stores will have different output. As long as this is documented, it is acceptable.

This change is reflected #256

@telezhnaya
Copy link
Contributor Author

@evergreen-trading-systems thank you for the proposal! I have few comments:

  • I suggest nft_mint, nft_burn, and nft_transfer instead of nft_batch_mint, nft_batch_burn, and nft_batch_transfer. We can pass the data as a list anyway, just pass one value in the list if you don't need the batch mode
  • Not sure about nft_burn interface. Now we have to search for the owner to show that the token was burnt. It's better to mention the owner in the data. Same as we mention sender_id in nft_transfer.
  • In nft_transfer, I suggest to rename to to receiver_id.
  • To unify the interfaces, maybe it's better to use receiver_id instead of token_id in nft_mint. And we can use sender_id in nft_burn.

@ghost
Copy link

ghost commented Sep 27, 2021

@telezhnaya thanks for your comment.

  • I have removed batch from the event names.
  • I have added owner_id to nft_burn
  • I have replaced to with receiver_id in nft_transfer

These changes can be seen https://github.com/evergreen-trading-systems/NEPs/blob/feature/batch/specs/Standards/NonFungibleToken/Event.md. Updated pr too.

I don't know if receiver_id makes sense for nft_mint.

@frol
Copy link
Collaborator

frol commented Sep 27, 2021

@evergreen-trading-systems Thanks for championing the work on writing the proposal! Here are few items I heard people had concerns:

  1. How many logs can we emit in a single contract execution?
    1.1. Given the length limit of the logs, what happens if the length is exceeded? (Would it help if we use shorter names of the JSON fields?)
    1.2. Given the gas limit, how much gas do event logs contribute to? (I would write a simple contract logging "hello world" and compare it to one that emits a ton of logs)
  2. Event logs should be easily identifiable and parsable (I believe there is no need to wrap the log message with extra {} since that may mislead people; thus I believe a simple "JSON_EVENT:" prefix should be enough)

Reviewing the latest version from here, I noticed that mint and burn methods are not symmetric: you can mint a given token for a corresponding account, but you can only burn tokens for a single owner. Another asymmetry is account_id vs owner_id in minting/burning; I suggest using owner_id in both cases.

@ghost
Copy link

ghost commented Sep 27, 2021

Thanks for looking at this @frol

  1. This will vary for everyone. Everyone has a different token_id format for example and we do not enforce length size. I believe everyone should test their implementation and adjust accordingly. We are working towards interoperability, not serving specific app protocol. We could define a base number of max transactions to happen in these events and review over time.
    1.1 If a log over 16kb is sent, we can expect that event not to be indexed. I'm not sure what the indexer actually does when it sees this. Point 1 could mitigate this ensuring that limit is not reached. I imagine it at least checks the size. It could emit an error log event - that is a discussion for another day.
    1.2 I think this is out of scope for this pr since we all agreed this from previous meeting. My previous points address these concerns. It should be each dApp owners responsibility to assess how this standard affects them. Feel free to work from this base to investigate gas implications - https://github.com/evergreen-trading-systems/event_logging
  2. I think everybody that will send logs will be using json which {} represents. I believe current logs are in json format so this is nothing new.

owner_id asymmetry has been addressed. Burning tokens for multiple owners is not a use case I've heard of.

@mikedotexe
Copy link
Contributor

mikedotexe commented Sep 27, 2021

If we decide to choose the solution only with logs (I will be upset but anyway), could we mention in NEP that the method should have ft_ or nft_ prefix? It will allow us not to parse all the function calls.

Thanks everyone for the recent work here. Returning to this question from Olga, I want to check with @MaximusHaximus (Wallet team) as I seem to recall concerns about folks using these somewhat "special" prefixes (ft_, nft_, storage_, etc.) for methods that are not part of any standard.

Seeing that bloom filters are not anticipated to happen anytime soon, and Olga's concern about needing some indicator to "please check for log events in this transaction" I think I hesitantly agree with Olga here. I would love it if these special prefixes were supposed to only be used for methods that abide by a standard, but I can't think of better options at this time. Are there objections to this?

@mikedotexe
Copy link
Contributor

2. Event logs should be easily identifiable and parsable (I believe there is no need to wrap the log message with extra {} since that may mislead people; thus I believe a simple "JSON_EVENT:" prefix should be enough)

I'm going to agree with Frol on this, and perhaps I didn't explain my initial reasoning in an earlier comment. At some point, NEAR will have binary logs like Ethereum but we're not quite there yet. Until then, we can use a prefix like EVENT_JSON: in order to indicate to indexers that are parsing transaction logs that this is an event and it'll be utf8.

I can imagine other prefixes like EVENT_BORSH: or EVENT_RLP: where the remainder of the log payload wouldn't benefit from having a closing } at the end. So I do think we'll want to remove the extra {} surrounding those. Hope I made it more clear this time.

@ilblackdragon
Copy link
Member

ilblackdragon commented Sep 28, 2021

@telezhnaya What exactly is the problem for analyzing all the logs on the Indexer side? As far as I understand all the transaction and their results are getting processed and put into the database.

Given we define proper format of the logs, this processor should parse them out and put them into respective places. I would expect we would just have a single logs table containing all logs from all contracts. This is how all the indexers work right now as far as I understand in Etheruem, hence I'm not fully understanding why we need to optimize for function names for events (see below cross post why function name don't work). Then separate view/indexing process can do specific things for FT and NFT based on specific log format in those standards.


Cross posting from #249 (comment)

As I have mentioned this to @frol that he wrote in #254 (comment), you > can not and should not define minting or burning standard based on function names.

There are many times when tokens will be minted as a subsequent step of some other action:

user deposited another token (like lending)
user swapped tokens via deposit of another token with ft_transfer_call (like Ref)
user called add_liquidity (like Ref)
user called liquidate (like stable call, lending)
user burnt another token (burn near, get my token)
some conditional statement when they get minted and when they do't
etc.
Function name based standard is impossible to define here.

Instead logs emitted when new tokens enter circulation and when they leave should be present.

@frol
Copy link
Collaborator

frol commented Sep 28, 2021

What exactly is the problem for analyzing all the logs on the Indexer side?

@ilblackdragon Olga assumed that at maxed out tps and maxed out logs we may hit the performance issue unable to process a chunk. I believe that is not the case, so we will proceed and parse all the logs.

@telezhnaya
Copy link
Contributor Author

@ilblackdragon I wanted to minimize the possibility of parsing something completely irrelevant, but accidentally in the same format. I am OK with parsing all logs from all functions. Thank you for the explanation with the different scenarios!

@ghost
Copy link

ghost commented Sep 29, 2021

I am dedicated to seeing NFT core events - burn, mint and transfer. There's some crypto events happening in October where number of transactions are going to spike. This is pressing need for NFT marketplaces so users see those actions on the wallet. This is already happening now for example nft_transfer but this works for singles and not bulk. This is already makes users ask why they can see on wallet those transactions for 1 marketplace and not for another who do bulk.

I've updated the proposal accordingly. Can we get eta when the wallet will listen for events if nothing else is outstanding?

@frol @mikedotexe @telezhnaya

@telezhnaya
Copy link
Contributor Author

Please have a look at the PR that I just created.
Forwarding the most important piece:

From the event log, we (as Indexer developers) should be able to retrieve the following information:

  • emitted_by_contract_account_id (TODO we haven't discussed that, this field should be added to metainfo, where we store standard, version, and other details)
  • token_id
  • event_kind (mint, transfer, burn)
  • token_sender_account_id (should be skipped for mint)
  • token_receiver_account_id (should be skipped for burn)
  • token_transfer_approval_account_id TODO we haven't discussed that
  • token_transfer_memo (optional, feel free not to fill it inside your batch event log if it's empty or you are worried about log size)

@evergreen-trading-systems @zcstarr @robert-zaremba

@ghost
Copy link

ghost commented Oct 10, 2021

Based on #256 (comment) its obvious event format vs specific event data format e.g. nft_mint are 2 different things.

We're already clear on what an event should look like in maybe formats potentially e.g. Binary, RLP, Borsh, etc. This pr addresses that https://github.com/near/NEPs/pull/268/files

Specific events - nft_mint,nft_approve etc all need their own pr. When there is need to track another event type, pr can be submitted, each event can be examined more robustly

@telezhnaya telezhnaya changed the title FT NEP-141, NFT NEP-171 require having minting/burning interface NFT NEP-171 require having minting/burning interface Oct 27, 2021
@telezhnaya telezhnaya linked a pull request Oct 27, 2021 that will close this issue
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

Successfully merging a pull request may close this issue.

8 participants
@zcstarr @ilblackdragon @frol @robert-zaremba @mikedotexe @telezhnaya @MaksymZavershynskyi and others