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: make destination address & memo optional in POST /transactions response #1294

Merged
merged 8 commits into from
Aug 16, 2022
Merged
31 changes: 21 additions & 10 deletions ecosystem/sep-0031.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

## Preamble

```
Expand All @@ -7,8 +6,8 @@ Title: Cross-Border Payments API
Author: SDF
Status: Active
Created: 2020-04-07
Updated: 2022-07-20
Version 1.10.0
Updated: 2022-08-12
Version 2.0.0
```

## Simple Summary
Expand Down Expand Up @@ -442,14 +441,23 @@ Name | Type | Description

##### Success (201 Created)

This is the successful case where a Receiving Anchor confirms that they can fulfill this payment as described. The response body should be a JSON object with the following values
This is the successful case where a Receiving Anchor either confirms that they can fulfill this payment as described or that confirmation is pending.

Anchors should check the `status` response attribute to determine whether or not the Receiving Anchor is ready to receive the payment.

If `status` is `pending_sender`, `stellar_account_id`, `stellar_memo`, & `stellar_memo_type` should be present.

If `status` is `pending_receiver`, the Receiving Anchor is processing the information sent and determining if the transaction can proceed. In this case, `stellar_account_id`, `stellar_memo`, & `stellar_memo_type` will not be present, and the Sending Anchor should monitor the transaction's status until it moves to `pending_sender` or `error`.

Sending Anchors can monitor the transaction's status by registering a callback URL via [`PUT /transactions/:id/callback`](#put-transaction-callback) or by polling [`GET /transactions/:id`](#get-transaction). When the transaction status moves to `pending_sender`, the Stellar account & memo fields should be populated.

Name | Type | Description
-----|------|------------
`id` | string | The persistent identifier to check the status of this payment transaction.
`stellar_account_id` | string | The Stellar account to send payment to.
`stellar_memo_type` | string | The type of memo to attach to the Stellar payment (`text`, `hash`, or `id`).
`stellar_memo` | string | The memo to attach to the Stellar payment.
`stellar_account_id` | string | (optional) The Stellar account to send payment to.
`stellar_memo_type` | string | (optional) The type of memo to attach to the Stellar payment (`text`, `hash`, or `id`).
`stellar_memo` | string | (optional) The memo to attach to the Stellar payment.
`status` | string | (optional) The initial status of the transaction, either `pending_sender` or `pending_receiver`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're making a breaking change and going to v2.0.0, what do you think of changing the response body of POST /transactions to the same response body of GET /transactions/:id?

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 that makes a lot of sense. We’ll have to be intentional about adding more guidance in the SEP and documentation though so it’s clear which attributes are useful immediately after transaction creation.

Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate that we have four optional parameters that are bound together, i.e. not individually optional, they're only optional together.

A status that isn't present is interesting, would it make sense do you think to add a status to represent this moment? It would mean the state machine always has a status, rather than this unknown status period.

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 added status as optional because existing implementations don't return it, not because there isn't a status value that represents the moment. pending_receiver would represent the moment if the address & memo fields are not returned, otherwise pending_sender would be appropriate.

If we go with @marcelosalloum's suggestion to return the same response body as the GET /transactions/:id, then status would be required.

So I think we just need to decide how big of a breaking change we want to make. Making previously required response attributes optional is less impactful to existing implementations than making previously absent fields required.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, interesting. I wonder if breaking this change into two changes in the changelog will help people understand it clearer. i.e. add status as optional, then make other changes. It doesn't improve the resulting situation though.

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 do think there are two breaking changes here

  1. Make stellar_account_id, stellar_memo, & stellar_memo_type optional in POST /transactions & GET /transactions/:id responses
  2. Replace current POST /transactions response body with the response body of GET /transactions/:id

It just pains me to bump the major version twice. Making 1 breaking change per major version bump makes things clearer, but I see other pieces of software make many breaking changes per major version bump.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, we don't want to bump it twice. Breaking changes in software is one thing, in standards it creates more work, as how do clients interoperate with all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.. then @marcelosalloum I think we should just go with breaking change 1 and not do 2. It would make the API cleaner and more RESTful but its not worth making current implementations out of date.


##### Customer Info Needed (400 Bad Request)

Expand Down Expand Up @@ -516,16 +524,17 @@ Name | Type | Description
`id` | string | The ID returned from the `POST /transactions` request that created this transaction record.
`status` | string | The status of the transaction. Values are outlined below.
`status_eta` | number | (optional) The estimated number of seconds until a status change is expected.
`status_message` | string | (optional) A human-readable message describing the status of the transaction.
`amount_in` | string | (optional) The amount of the Stellar asset received or to be received by the Receiving Anchor. Excludes any fees charged after Receiving Anchor receives the funds. If a `quote_id` was used, the `amount_in` should be equals to both: (i) the `amount` value used in the [`POST /transactions`](#post-transactions) request; and (ii) the quote's `sell_amount`.
`amount_in_asset` | string | (optional) The asset received or to be received by the Receiving Anchor. Must be present if `quote_id` or `destination_asset` was included in the `POST /transactions` request. The value must be in [SEP-38 Asset Identification Format](sep-0038.md#asset-identification-format).
`amount_out` | string | (optional) The amount sent or to be sent by the Receiving Anchor to the Receiving Client. When using a `destination_asset` in the [`POST /transactions`](#post-transactions) request, it's expected that this value is only populated after the Receiving Anchor receives the incoming payment. Should be equals to `quote.buy_amount` if a `quote_id` was used.
`amount_out_asset` | string | (optional) The asset delivered to the Receiving Client. Must be present if `quote_id` or `destination_asset` was included in the `POST /transactions` request. The value must be in [SEP-38 Asset Identification Format](sep-0038.md#asset-identification-format).
`amount_fee` | string | (optional) The amount of fee charged by the Receiving Anchor. Should be equals `quote.fee.total` if a `quote_id` was used.
`amount_fee_asset` | string | (optional) The asset in which fees are calculated in. Must be present if `quote_id` or `destination_asset` was included in the `POST /transactions` request. The value must be in [SEP-38 Asset Identification Format](sep-0038.md#asset-identification-format). Should be equals `quote.fee.asset` if a `quote_id` was used.
`quote_id` | string | (optional) The ID of the quote used to create this transaction. Should be present if a `quote_id` was included in the `POST /transactions` request. Clients should be aware though that the `quote_id` may not be present in older implementations.
`stellar_account_id` | string | The Receiving Anchor's Stellar account that the Sending Anchor will be making the payment to.
`stellar_memo_type` | string | The type of memo to attach to the Stellar payment: `text`, `hash`, or `id`.
`stellar_memo` | string | The memo to attach to the Stellar payment.
`stellar_account_id` | string | (optional) The Receiving Anchor's Stellar account that the Sending Anchor will be making the payment to.
`stellar_memo_type` | string | (optional) The type of memo to attach to the Stellar payment: `text`, `hash`, or `id`.
`stellar_memo` | string | (optional) The memo to attach to the Stellar payment.
`started_at` | UTC ISO 8601 string | (optional) Start date and time of transaction.
`completed_at` | UTC ISO 8601 string | (optional) Completion date and time of transaction.
`stellar_transaction_id` | string | (optional) The transaction_id on Stellar network of the transfer that initiated the payment.
Expand Down Expand Up @@ -609,6 +618,7 @@ Pending external (without quotes):
"id": "82fhs729f63dh0v4",
"status": "pending_external",
"status_eta": 3600,
"status_message": "Payment has been initiated via ACH deposit.",
"stellar_transaction_id": "b9d0b2292c4e09e8eb22d036171491e87b8d2086bf8b265874c8d182cb9c9020",
"external_transaction_id": "ABCDEFG1234567890",
"stellar_account_id": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H",
Expand Down Expand Up @@ -884,6 +894,7 @@ It is important to note that the Receiving Anchor is not obligated, at least by

## Changelog

* `v2.0.0`: Make `stellar_account_id`, `stellar_memo`, & `stellar_memo_type` optional in the `POST /transactions` response. Also adds the `status` attribute to the `POST /transactions` response and a `status_message` attribute to `GET /transactions/:id`. ([]())
* `v1.10.0`: Add `quote_id` to the transaction object schema. ([#1268](https://github.com/stellar/stellar-protocol/pull/1268))
* `v1.9.0`: Add callback signature requirement. ([#1264](https://github.com/stellar/stellar-protocol/pull/1264))
* `v1.8.0`: Add `expired` transaction status. ([#1233](https://github.com/stellar/stellar-protocol/pull/1233))
Expand Down