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, 24, & 6: add optional refund_memo to transaction initiation requests #1321

Merged
merged 5 commits into from
Nov 18, 2022

Conversation

JakeUrban
Copy link
Contributor

Receiving anchors often use custodial services to hold their on-chain assets, and these custodians often use omnibus accounts, so memos are used by the custodian to map inbound payments to their customer's balances.

To support this case, the standard allows Receiving anchors to provide a memo & memo type to use for each transaction in the POST /transactions request.

Sending anchors may also use custodial services to hold their on-chain assets, so in the event that a refund payment is necessary, sending anchors should be allowed to provide the receiving anchor a memo to use.

Thats why I'm adding two optional parameter to the POST /transactions request body, allowing the sending anchor to provide the receiving anchor with a memo to use when sending refund payments back.

@JakeUrban
Copy link
Contributor Author

cc @Nopik

lijamie98
lijamie98 previously approved these changes Nov 16, 2022
Copy link
Contributor

@lijamie98 lijamie98 left a comment

Choose a reason for hiding this comment

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

LGTM after reading the messages from slack.

@JakeUrban
Copy link
Contributor Author

JakeUrban commented Nov 16, 2022

If we agree on this design (cc @marcelosalloum) then I will make the same changes to SEP-24 & SEP-6

marcelosalloum
marcelosalloum previously approved these changes Nov 17, 2022
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.

LGTM!

Please, don't forget to update the Changelog section at the bottom of the file.

Comment on lines 440 to 441
`refund_memo` | (optional) The memo the Receiving Anchor must use when sending refund payments back to the Sending Anchor. If not specified, the Receiving Anchor should use the same memo the Sending Anchor used to send the original payment.
`refund_memo_type` | (optional) The type of the `refund_memo`. Can be `id`, `text`, or `hash`. See the [memos](https://developers.stellar.org/docs/encyclopedia/memos) documentation for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Should anchors use a default value for the memo type? I mean, if the user sends a memo but no memo type, should it be considered a text memo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do want to standardize a default type, it should probably be id. That way the ecosystem could migrate to using muxed account addresses for refunds eventually.

But I don't know if providing a default makes sense. Maybe I should just add a clause to the definitions saying refund_memo and refund_memo_type must be sent together.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I don't know if providing a default makes sense. Maybe I should just add a clause to the definitions saying refund_memo and refund_memo_type must be sent together.

Yeah, that makes sense to me.

@JakeUrban JakeUrban dismissed stale reviews from marcelosalloum and lijamie98 via b99dc9b November 18, 2022 19:08
@JakeUrban JakeUrban changed the title SEP-31: add optional refund_memo to POST /transactions request SEP-31, 24, & 6: add optional refund_memo to POST /transactions request Nov 18, 2022
@JakeUrban JakeUrban changed the title SEP-31, 24, & 6: add optional refund_memo to POST /transactions request SEP-31, 24, & 6: add optional refund_memo to transaction initiation requests Nov 18, 2022
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.

LGTM!

I just added a comment about the versioning of SEP-24.

Comment on lines +9 to 10
Updated: 2022-11-18
Version 2.7.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be 2.8.0, right?

Comment on lines +992 to +993

* `v2.7.0`: Add `refund_memo` and `refund_memo_type` parameters to withdraw endpoint. ([#1321](https://github.com/stellar/stellar-protocol/pull/1321))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be 2.8.0, right?

I believe you forget to add the 2.7.0 changelog in #1320

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I messed up by actually incrementing the minor version on SEP-24 instead of the patch version. So 2.7 makes sense for this change.

@JakeUrban JakeUrban merged commit bcc5551 into master Nov 18, 2022
@JakeUrban JakeUrban deleted the sep31-add-refund-memo branch November 18, 2022 20:13
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.

3 participants