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 Events Standard #652

Closed
wants to merge 11 commits into from
Closed

NFT Events Standard #652

wants to merge 11 commits into from

Conversation

BenKurrek
Copy link
Contributor

@BenKurrek BenKurrek commented Nov 30, 2021

Implements the events standard for the non-fungible-token core standards.

Fixes (#649)

@BenKurrek BenKurrek marked this pull request as draft November 30, 2021 21:53
@BenKurrek BenKurrek marked this pull request as ready for review November 30, 2021 23:47
Comment on lines 284 to 285
if !approval_id.is_none() {
authorized_id = Some(sender_id.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if !approval_id.is_none() {
authorized_id = Some(sender_id.to_string());
if approval_id.is_some() {
authorized_id = Some(sender_id.to_string());

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be an AccountId instead of a string?

Comment on lines 294 to 295
old_owner_id: owner_id.to_string(),
new_owner_id: receiver_id.to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

same with these, can/should they not be an AccountId for some reason?

near-contract-standards/src/non_fungible_token/events.rs Outdated Show resolved Hide resolved
near-contract-standards/src/non_fungible_token/events.rs Outdated Show resolved Hide resolved
near-contract-standards/src/non_fungible_token/events.rs Outdated Show resolved Hide resolved
@willemneal
Copy link
Contributor

willemneal commented Dec 1, 2021

@BenKurrek
This seems to be double work of #627

@BenKurrek BenKurrek requested a review from austinabell December 1, 2021 18:36
@BenKurrek
Copy link
Contributor Author

@BenKurrek This seems to be double work of #627

I didn't know this was being worked on until I was almost finished. Perhaps we can combine our PRs into one. :)

@willemneal
Copy link
Contributor

@BenKurrek You could use log functions from my PR to update the implementations.

@austinabell
Copy link
Contributor

Closing in favour of #717 #627 which have come in

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.

4 participants