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

feat: add NFT events support #169

Merged
merged 7 commits into from
Nov 8, 2021
Merged

feat: add NFT events support #169

merged 7 commits into from
Nov 8, 2021

Conversation

telezhnaya
Copy link
Contributor

@telezhnaya telezhnaya commented Oct 5, 2021

I mentioned most of the things in the doc inside the SQL script, please have a look at that.
Additionally, I want to help the contract creators to find the relevant information.
We discussed the format of event logs here

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

  • token_id
  • event_kind (mint, transfer, burn), reminder: we decided to group by this
  • token_sender_account_id (should be skipped for mint)
  • token_receiver_account_id (should be skipped for burn)
  • token_transfer_approval_account_id (optional, see the info below, 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)

token_transfer_approval_account_id

https://nomicon.io/Standards/NonFungibleToken/ApprovalManagement.html
You have to fill in this information in the event log if it's relevant for the current transfer.
Let's looks at the example:
If Alice approves Bob to transfer her token, and then Bob transfers it to Maggie, then the fields should look like this:

token_sender_account_id : alice.near
token_receiver_account_id : maggie.near
token_transfer_approval_account_id : bob.near

The same situation, but Alice as the owner does the transfer by herself:

token_sender_account_id : alice.near
token_receiver_account_id : maggie.near
token_transfer_approval_account_id : <empty>

We DO NOT store called_method anywhere because we get the info from logs, the method could be retrieved from receipt if you need. @MaximusHaximus do you need this info in Wallet?

@frol @MaximusHaximus @mikedotexe
@evergreen-trading-systems @zcstarr @robert-zaremba

@telezhnaya telezhnaya requested a review from frol October 5, 2021 14:53
@telezhnaya telezhnaya self-assigned this Oct 5, 2021
@telezhnaya
Copy link
Contributor Author

FYI: the discussion could be both here or after near/NEPs#254 (comment)

@ghost
Copy link

ghost commented Oct 5, 2021

awesome. why do we want to add permissions to log authors? because this is new. even if we wanted to, it's impossible to check permissions based on logs that anyone can send.

@telezhnaya
Copy link
Contributor Author

telezhnaya commented Oct 6, 2021

@evergreen-trading-systems it will be a complete anarchy. Without simple check, anyone can write any contract with any logs and make fake transfers, and we will believe these logs. Even if noone is interested in it because of financial reasons, be sure that we will have a disaster on each April 1st. This is too easy to hack.
I agree that we can trust the authors of the contract, but we should distinguish somehow between the author and unknown contract.

Just wanted to clarify: I know that the source of truth is the contract itself, and nothing will change because of fake logs, but people seem to trust Wallet and Explorer, and I don't want to provide them with the random data.

@telezhnaya
Copy link
Contributor Author

UPD: emitted_by_contract_account_id could be always taken from receipt_receiver_account_id, and that would be the natural way of checking the author of logs. Updated the message above

@zcstarr
Copy link

zcstarr commented Oct 6, 2021

UPD: emitted_by_contract_account_id could be always taken from receipt_receiver_account_id, and that would be the natural way of checking the author of logs. Updated the message above

Yes I think this makes sense. To be able to filter by contract account id that's emitting the event. I think somewhere down the line, consuming apps, when the Near nodes start doing logs/events with bloom filters, people will do something like the following rpc call.

near.get_Logs({ blockNumberStart:0, blockNumberEnd:100, contract_accounts:["cooldapp.near", "awesomeDapp.near"], type: json, topic: "nft_mint"}) 

and under the hood the nodes use bloom filters to make all the mechanics work efficiently.

The emitted_by_contract_account_id I could see as a prelude to later interfaces that might pull this data. I'm also pro the additional optional fields.

@telezhnaya telezhnaya removed the request for review from frol October 19, 2021 12:26
@telezhnaya
Copy link
Contributor Author

@frol you may receive spam notifications from here. I removed you from the reviewers, I will return you when the code will be ready

nft.json Outdated Show resolved Hide resolved
src/db_adapters/assets/non_fungible_token_events.rs Outdated Show resolved Hide resolved
src/db_adapters/assets/non_fungible_token_events.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@telezhnaya
Copy link
Contributor Author

We also need to think about loading historical data.
I suggest collecting the current state of all known NFT contracts by https://nomicon.io/Standards/NonFungibleToken/Enumeration.html#interface
As a result, we will have a state without previous history of movements.

Copy link
Contributor

@frol frol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for thoroughly documenting the data schema!

migrations/2021-10-04-100000_assets_nft/up.sql Outdated Show resolved Hide resolved
src/db_adapters/assets/non_fungible_token_events.rs Outdated Show resolved Hide resolved
src/db_adapters/assets/non_fungible_token_events.rs Outdated Show resolved Hide resolved
src/db_adapters/assets/non_fungible_token_events.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/schema.patch Outdated Show resolved Hide resolved
src/db_adapters/assets/non_fungible_token_events.rs Outdated Show resolved Hide resolved
@telezhnaya telezhnaya force-pushed the olya/nft branch 2 times, most recently from e713128 to a2bd58c Compare November 5, 2021 16:43
@telezhnaya telezhnaya marked this pull request as ready for review November 5, 2021 17:00
@telezhnaya
Copy link
Contributor Author

@frol sorry for such a massive number of changed files: cargo clippy becomes stricter, as I understand.

Important things since your last check:

  • I fixed your comments, it's obvious 🙂
  • I rewrote src/retriable.rs so that it's possible to pass custom error handling function
  • I fixed a bug in await_retry_or_panic, it invoked the logic 1 times less than expected. If you invoked it with 1 attempt, it gave you an immediate error.
  • I fixed all cargo clippy warnings

src/db_adapters/assets/non_fungible_token_events.rs Outdated Show resolved Hide resolved
src/db_adapters/assets/non_fungible_token_events.rs Outdated Show resolved Hide resolved
src/schema.patch Show resolved Hide resolved
@telezhnaya telezhnaya changed the title feat: add NFT interface with the documentation feat: add NFT events support Nov 8, 2021
@telezhnaya telezhnaya merged commit e202cf7 into master Nov 8, 2021
@telezhnaya telezhnaya deleted the olya/nft branch November 8, 2021 08:53

let nft_types::NearEvent::Nep171(nep171_event) = event;
Some(nep171_event)
}).collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI, my point was to avoid Vec. You should be able to just drop .collect() (and drop the explicit type) and that will give you an iterator, so you can directly use it in the loop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, now I see.
I was thinking about map function in the next piece of code, but the problem was that each event can produce multiple DB rows, and I don't know how to write it in a functional style in Rust.

I was happy to change at least the first cycle here, but I didn't finish it properly 😄
I will fix it in another small PR!

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 this pull request may close these issues.

3 participants