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

Improve SEP-006 #1448

Closed
wants to merge 1 commit into from
Closed

Improve SEP-006 #1448

wants to merge 1 commit into from

Conversation

christian-rogobete
Copy link
Contributor

  1. fix json examples
  • remove trailing commas that break the json parser
  • remove min_amount and max_amount from withdrawal-exchange response
  1. extend withdrawal and withdrawal-exchange request
  • add email_address field analogous to the deposit and deposit-exchange request
  1. fix some typos

@Ifropc
Copy link
Contributor

Ifropc commented Mar 20, 2024

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
Copy link
Contributor Author

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?

Copy link

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
Copy link
Contributor

Ifropc commented May 7, 2024

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
Copy link
Contributor Author

christian-rogobete commented May 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants