SEP-31: make destination address & memo optional in POST /transactions response #1294
Conversation
|
@lijamie98, can you update the status diagram to show |
ecosystem/sep-0031.md
Outdated
| `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`. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
ecosystem/sep-0031.md
Outdated
| `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`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🤔 I do think there are two breaking changes here
- Make
stellar_account_id,stellar_memo, &stellar_memo_typeoptional inPOST /transactions&GET /transactions/:idresponses - Replace current
POST /transactionsresponse body with the response body ofGET /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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Ok, I removed the |
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
resolves #1290
See the issue description for details.
cc @gaclaudiu