SEP-24: Change completed_at field to optional

[lijamie98]

Closes #1184
completed_at was defined as Optional fields in SEP-6 and SEP-31. However, the field is defined as Assigned null for in-progress transactions. in SEP-24.

Suggest to make the field consistent among all protocols.

[leighmcculloch]

See comment inline.

[leighmcculloch]

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.

[lijamie98]

@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?

[CassioMG]

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

[CassioMG]

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

[JakeUrban]

This looks good to me, can we make sure to update the meta info at the top and the changelog at the bottom @lijamie98 ?

I agree with @leighmcculloch that ideally we use null values in JSON responses instead of omitting fields, but unfortunately it looks like our example responses in the SEPs already omit fields labeled as optional.

I would think that robust client applications are implemented without assuming the presense of a key vs. it having a null value.

[leighmcculloch]

I would think that robust client applications are implemented without assuming the presense of a key vs. it having a null value.

For context, I believe the reason null is preferred is not due to clients not being robust. It is for helping developers understand APIs as well as handling and catching typos in client code. If a fields are omitted, the shape of the response changes from one request to the next which is difficult to reason about. Also, if a field is omitted it appears as "undefined" in JS apps. If a field has a typo in its name on the client side, its value appears as "undefined". It increases the difficulty of identifying these problems.

Regardless, makes sense to keep consistent with the rest of the doc and make it ommitted.

[lijamie98]

This looks good to me, can we make sure to update the meta info at the top and the changelog at the bottom @lijamie98 ?

I agree with @leighmcculloch that ideally we use null values in JSON responses instead of omitting fields, but unfortunately it looks like our example responses in the SEPs already omit fields labeled as optional.

I would think that robust client applications are implemented without assuming the presense of a key vs. it having a null value.

@JakeUrban Thanks for the reminder. The version and dates are updated.

[lijamie98]

Can someone approve this PR if this is ready to merge?

[CassioMG]

LGTM

[leighmcculloch]

Could you update the changelog which is at the end of the document?

[lijamie98]

Could you update the changelog which is at the end of the document?

Added and updated.