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

SEP-31: add callback URL registration endpoint #1214

Merged
merged 6 commits into from
May 27, 2022

Conversation

JakeUrban
Copy link
Contributor

@JakeUrban JakeUrban commented May 24, 2022

resolves #1209

Adds a PUT /transactions/:id/callback endpoint for Sending Anchor to request callback requests to a specified URL. This removes the need for Sending Anchors to poll the statuses of in-progress transactions, reducing the chatter between services, especially for anchors with high transaction throughput.

I considered adding this functionality into the existing POST /transactions and PATCH /transactions requests as an optional parameter, but figured that it was likely for Receiving Anchors who did not yet implement this feature to simply ignore the callback_url request attribute, leading Sending Anchors to believe their callback URL was accepted but never receiving the callback requests.

Note: originally we decided not to add this functionality in #636. I disagree with my former self there and don't see why we wouldn't add this capability for the benefit of Sending Anchors that wanted it. Receiving Anchors can chose not to implement this until a Sending Anchors requests it. cc @gaclaudiu

leighmcculloch
leighmcculloch previously approved these changes May 24, 2022
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

This new endpont is subject to the same authenticaton requirements of SEP-10? LGTM!

@JakeUrban
Copy link
Contributor Author

This new endpont is subject to the same authenticaton requirements of SEP-10?

Yes it is, but I think thats clear from the authentication section.

I'll wait for another member of the anchor eng team to approve before merging.

Copy link
Contributor

@marcelosalloum marcelosalloum left a comment

Choose a reason for hiding this comment

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

There seem to be a few details missing in this change:

  • Add this endpoint to the API Endpoints section.
  • Mention the usage of this endpoint in the "detailed sending/receiving anchor flows"

@JakeUrban
Copy link
Contributor Author

JakeUrban commented May 26, 2022

Thanks for the comments @marcelosalloum ! I definitely missed those items.

Copy link
Contributor

@marcelosalloum marcelosalloum left a comment

Choose a reason for hiding this comment

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

Not so sure about one line of your change:

ecosystem/sep-0031.md Outdated Show resolved Hide resolved
@JakeUrban JakeUrban merged commit 6aef92a into master May 27, 2022
@JakeUrban JakeUrban deleted the sep31-add-callback-endpoint branch May 27, 2022 19:49
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.

SEP-31: add a transaction status callback mechanism
3 participants