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

add sep24 callback params #97

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

add sep24 callback params #97

wants to merge 6 commits into from

Conversation

acharb
Copy link
Contributor

@acharb acharb commented Feb 5, 2024

@@ -61,6 +61,10 @@ export class Sep24 {
* @param {ExtraFields} [params.extraFields] - Additional fields for the request.
* @param {Memo} [params.destinationMemo] - Memo information for the destination account.
* @param {string} [params.destinationAccount] - The destination account for the deposit.
* @param {string} [params.callback] - The callback URL or postMessage the anchor should POST to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ifropc is postMessage something that's used / we should support?

and if so what's an example of one? just a JSON string?

Choose a reason for hiding this comment

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

We should not support postMessage. This is an error-prone way of signaling to the wallet to close the webview, and there are more straightforward approaches to this that have better user experiences as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We are deprecating postMessage
  2. We are also deprecating approach of passing callbacks "as is" to the request parameter. I'm working on drafting new approach, where callbacks are passed outside of the request.
    So let's hold on this PR for now

Copy link

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

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

We may want to hold this PR until we come to a conclusion on what changes we want to make to the SEPs regarding callbacks.

There have been various discussions internally and with partners about whether the proposals should encourage callbacks to endpoints provided by the client on a per-transaction basis. It appears as if most businesses providing callback prefer to have client's register static endpoints outside of the transaction processing flow defined in SEPs. MoneyGram and Circle for example both take this approach to their respective callback services.

The two viable paths I see on this topic is to

  1. Keep the callback and on_change_callback parameters in the proposal, leaving it up to each anchor and wallet pair to determine whether and how they will implement support.
  2. Deprecate and eventually remove the parameters and recommend that anchors require clients to register callback endpoints with them prior to initiating transactions.

@acharb
Copy link
Contributor Author

acharb commented Feb 5, 2024

makes sense, I'll keep this PR in drafts then until we know how we want to handle the callbacks

@acharb acharb marked this pull request as draft February 5, 2024 19:47
@acharb
Copy link
Contributor Author

acharb commented Mar 11, 2024

hey @JakeUrban how long do you think it will be until the new way to handle callbacks is determined?

and do you think it'd make sense to merge this code in now, and then we can change it later once the SEP changes

@Ifropc
Copy link
Contributor

Ifropc commented Mar 11, 2024

Hey @acharb I think for now we can close this PR. We still need to discuss it more internally. Currently, in the Anchor Platform, which we use as a standard implementation of all SEPs, callbacks are configured on the Anchor side. So we may want to align protocol to the implementation, as we think this approach is better for various reasons

@mtwtfss
Copy link

mtwtfss commented Mar 11, 2024

@Ifropc are you saying the Anchor Platform implementation currently deviates from the SEP standard and we think we might want to update the SEP standard to reflect this deviation?

any risk to merging what we have here for now while we figure out the changes we'd like to make?

also do you have any kind of ETA for when we'll land on a longterm solution?

@Ifropc
Copy link
Contributor

Ifropc commented Mar 11, 2024

@mtwtfss
Yes, that's correct!
There's no risk in merging it, but potentially we might carry deprecated parameters in the Wallet SDK, and would need to deal with the deprecation appropriately.
Sorry for now there's no ETA, but I created a task for finalizing decisions of callback handling and put it on the top of my backlog

Base automatically changed from release/1.4.0 to main March 26, 2024 17:38
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