stellar / stellar-protocol

Developer discussion about possible changes to the protocol.
509 stars 301 forks source link

Improve SEP-006 #1448

Closed christian-rogobete closed 2 months ago

christian-rogobete commented 4 months ago
  1. fix json examples
  1. extend withdrawal and withdrawal-exchange request
  1. fix some typos
Ifropc commented 3 months ago

Hey @christian-rogobete thanks for the PR, I totally missed it. Can't we just pass email address as part of SEP-9 field and use it for notification purposes?

christian-rogobete commented 3 months ago

Hello @Ifropc , thank you very much for your message.

I have noticed that the email_address field is currently present in the deposit request but not in the withdraw request. I have added it there for the sake of completeness.

At first I was also wondering why it is not used as part of SEP-09/SEP-12 and I had the following thoughts:

It may be that the anchor does not specify the email field as mandatory in SEP-12 and therefore it is not present. Furthermore, if the user wants to receive notifications only for this specific case (SEP-06), he can enter the email address in the request and receive the notifications only for the executed action (deposit or withdrawal).

I think we should either delete the email_address field from the deposit request, or add it to the withdraw request as well.

What would perhaps be even better is to create a new SEP, through which the user can determine in fine granularity what he wants to receive notifications for. He could also specify how he wants to receive the notifications (e.g. email, phone, callback, etc.). What do you think?

github-actions[bot] commented 2 months ago

This pull request is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed.

Ifropc commented 2 months ago

Hey @christian-rogobete I believe anchor can make passing the email address mandatory via SEP-12 and use it to send notifications. User may unsubscribe from email chain if they want to using the standard email functionality. For now, I don't see the use for this. Thank you for suggesting the change!

christian-rogobete commented 2 months ago

Hey @Ifropc, thank you for letting me know. Sounds good. I closed the PR because I am unsure if min_amount and max_amount can be part of a withdraw-exchange (and deposit-exchange) asset in the info response.

The text suggests that they are not, but they are used in the example. I will reintegrate them into the SDKs to make sure that nothing is missing. They are also used in the Anchor Platform.