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

changed to match standard #693

Closed
wants to merge 2 commits into from
Closed

changed to match standard #693

wants to merge 2 commits into from

Conversation

mattlockyer
Copy link
Contributor

@austinabell
Copy link
Contributor

hmm, strange, seems that nft_approve is forwarding any result from this call as its return. I don't think necessarily that this function signature violates the standard though, since adding a return doesn't break any call unless someone for some reason asserted that the return bytes were empty?

In any case, this is a bad signature because it only allows String return anyway. I won't weigh in on the standards part of this, but changing this from its current signature is something I've wanted to do before, but I will wait until it is agreed on the direction to go with this.

@mattlockyer
Copy link
Contributor Author

Just found it confusing.

@agostbiro
Copy link
Contributor

Thanks for opening this PR and raising the issue @mattlockyer ! We need to discuss more how to approach it, so I opened a new issue for it and will close this PR in favor of that. Hope you don't mind and it would be great if you could participate in the discussion in #1053.

@agostbiro agostbiro closed this Jul 13, 2023
@agostbiro agostbiro deleted the mattlockyer-patch-1 branch July 13, 2023 10:45
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