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: implement callback_result annotation #554

Merged
merged 11 commits into from
Sep 4, 2021

Conversation

austinabell
Copy link
Contributor

  • Implements a callback_result annotation which allows handling errors of promise callbacks
  • Adds callback_unwrap annotation to replace callback
    • Given the way that the code is set up, I don't think there is a way to deprecate this callback annotation, but would be happy if someone proves me wrong
  • Adds a new example that uses this and handles some use cases not covered in other examples (like stateless methods)

Closes #155

Optionally can add a CallbackResult type which is just

type CallbackResult<T> = Result<T, PromiseError>;

But it's a bit awkward because it really should be PromiseResult, but that type is already built into the current API and is a custom enum

@ChaoticTempest
Copy link
Member

just making sure, but would we need to add in callback_unwrap and callback_result to near-sdk-macros/lib.rs? I see there's callback and callback_vec defined there as marker attributes, but I'm not how these relate at all since everything should be going thru the near_bindgen macro itself

@austinabell
Copy link
Contributor Author

just making sure, but would we need to add in callback_unwrap and callback_result to near-sdk-macros/lib.rs? I see there's callback and callback_vec defined there as marker attributes, but I'm not how these relate at all since everything should be going thru the near_bindgen macro itself

No, that was an artifact of a previous version. These are no longer used anymore, as the annotations now are filtered out of the macro output. I just didn't remove the old ones yet since I'm not sure the implications and didn't want to group that change with this.

Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

this looks good to me so far, but might want another set of eyes on this just in case I overlooked anything

@austinabell
Copy link
Contributor Author

this looks good to me so far, but might want another set of eyes on this just in case I overlooked anything

Yeah, I'm in no rush to get this in, so will leave until it has another review. It is a little more involved of a PR so this is definitely a good candidate for more reviews

@austinabell austinabell merged commit 072b477 into master Sep 4, 2021
@austinabell austinabell deleted the austin/callback_result branch September 4, 2021 13:09
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.

Differentiate successful and failed callback
3 participants