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

Fix NonFungibleTokenApprovalReceiver::nft_on_approve return type #1053

Open
Tracked by #918
agostbiro opened this issue Jul 13, 2023 · 0 comments
Open
Tracked by #918

Fix NonFungibleTokenApprovalReceiver::nft_on_approve return type #1053

agostbiro opened this issue Jul 13, 2023 · 0 comments
Labels

Comments

@agostbiro
Copy link
Contributor

agostbiro commented Jul 13, 2023

Problem

The NonFungibleTokenApprovalReceiver::nft_on_approve trait method currently has a near_sdk::PromiseOrValue<String> return type, but the standard mandates a void return type as reported in #693.

Possible Solutions

The void return type in the standard is probably an oversight, since in the documentation of the nft_approve interface it says that it "returns promise call to nft_on_approve, which can resolve with whatever it wants." This suggests that the intended return type nft_on_approve in the standard was any. We could submit an errata to fix this.

Question is how to represent the any return type of NonFungibleTokenApprovalReceiver::nft_on_approve in Rust? I see the following options:

  1. The obvious solution would be to use Box<dyn std::any::Any>, but this won't work, because the trait Serialize is not implemented for dyn Any.
  2. An associated type on the trait (this might not play nicely with bindgen macros) (h/t @austinabell )
  3. Require the implementation to serialize the value. We could express this in the type system by having NonFungibleTokenApprovalReceiver::nft_on_approve return near_sdk::PromiseOrValue<serde_json::Value>. This would be a good solution, but we need to support borsh as well. I'm not too familiar with borsh, but as far as I can tell, borsh::schema::BorshSchema achieves the same result, so we could have NonFungibleTokenApprovalReceiver::nft_on_approve return an enum value that wraps either serde_json::Value or BorshSchema, but this might be getting too complex.
  4. Leave the return type near_sdk::PromiseOrValue<String> since anything can be serialized into a string and it hasn't caused any problems so far except for some confusion around the documentation.

Edit: added associate type solution from Austin originally written in #918.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: NEW❗
Development

No branches or pull requests

1 participant