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.

Sign up for GitHub

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

Jump to bottom

SEP-24: Change completed_at field to optional #1185

Merged
merged 1 commit into from
May 5, 2022
Merged

SEP-24: Change completed_at field to optional #1185

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions ecosystem/sep-0024.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ Title: Hosted Deposit and Withdrawal
Author: SDF
Status: Active
Created: 2019-09-18
Updated: 2021-12-01
Version 2.2.0
Updated: 2022-05-03
Version 2.2.1
```

## Simple Summary
Expand Down Expand Up @@ -689,7 +689,7 @@ Name | Type | Description
`amount_fee` | string | Amount of fee charged by anchor.
`amount_fee_asset` | string | (optional) The asset in which fees are calculated in. Must be present if the deposit/withdraw was made using non-equivalent assets. The value must be in [SEP-38 Asset Identification Format](sep-0038.md#asset-identification-format). See the [Asset Exchanges](#asset-exchanges) section for more information.
`started_at` | UTC ISO 8601 string | Start date and time of transaction.
`completed_at` | UTC ISO 8601 string | Completion date and time of transaction. Assigned null for in-progress transactions.
`completed_at` | UTC ISO 8601 string | (optional) Completion date and time of transaction.
Copy link
Member

Choose a reason for hiding this comment

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

What does this change mean in terms of the API response. Will optional mean that the response field will be omitted? There might have been a reason we made this explicitly null. I don't remember if this field specifically related to this, but the Vibrant team provided feedback at one point that having omitted fields is more difficult to debug and code against than null fields, and so we started preferring null fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leighmcculloch Yes, this means optional fields may be omitted. When we were looking at the example JSON API responses, optional fields seems to imply omission.

@CassioMG Can you also provide feedback of making SEP24 consistent with SEP-6 and SEP-31 which mark completed_at as optional? Is Vibrant treating the completed_at field in SEP-24 different from SEP-6 and SEP-31?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lijamie98 it's completely fine to omit this field, we are not making any comparisons with null

Copy link
Contributor

Choose a reason for hiding this comment

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

Also +1 for making it consistent throughout all SEPs, thanks for fixing this

`stellar_transaction_id` | string | transaction_id on Stellar network of the transfer that either completed the deposit or started the withdrawal.
`external_transaction_id` | string | (optional) ID of transaction on external network that either started the deposit or completed the withdrawal.
`message` | string | (optional) Human readable explanation of transaction status, if needed.
Expand Down Expand Up @@ -935,5 +935,5 @@ There is a small set of changes when upgrading from SEP-6 to SEP-24.
* Solar wallet: https://solarwallet.io

## Changelog

* `v2.2.1`: Make `completed_at` field optional. ([#1185](https://github.com/stellar/stellar-protocol/pull/1185))
* `v2.2.0`: Deprecate refunded boolean. Add refund object to transaction records. ([#1128](https://github.com/stellar/stellar-protocol/pull/1128))