bitpay / ruby-client

Powerful, flexible, lightweight SDK for the BitPay Bitcoin Payment Gateway API.
MIT License
79 stars 64 forks source link

Do not set a guid param during POST. #50

Open btalbot opened 8 years ago

btalbot commented 8 years ago

The API seems to behave the same if a different guid is provided every time (the previous client behavior) or if no guid is provided.

Creating a random guid for every POST request breaks the idempotency intent of the API by making every POST unique and makes creating a reliable application very difficult.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.06%) to 91.034% when pulling 5386280a61bf00fbf1f0487d6ae7ce7be143d6e3 on iJJi:bugs/1 into 0ec4bb11056df5fd581834a25aa56c9df1166e25 on bitpay:master.

kleetus commented 8 years ago

I think I see your point with regard to send a POST request with a newly generated uuid; it really does nothing with respect to using the:

guid(an optional parameter to enforce idempotence for POST requests)

feature supported on the /invoices endpoint, etc. We really should be allowing the callers of the post method in rest_connector to pass in the guid to make full use of this feature. So, in the case of returning a response from process_request to post and then to the caller, I would also include the guid that was either generated or passed in to the post method.

As such, there really is no reason to pluck the guid field out summarily. @btalbot stated:

Creating a random guid for every POST request breaks the idempotency intent of the API by making every POST unique and makes creating a reliable application very difficult.

Yet our documentation stipulates that the guid field would enforce idempotence for POST requests, meaning that subsequent POST requests using the same guid would achieve idempotence. Since the current behavior of the ruby-client is to create a new guid value for each POST request, we are implicitly disregarding this feature altogether.

btalbot commented 8 years ago

The only way for a client to ensure that a bitpay resources is created exactly once for a given client-side resource (say to create a bitpay invoice for a client order) is for the client to generate the guid, persist it with the client order, and then submit the request with the guid to bitpay invoice creation.

This way, if the client disconnects or crashes before it is able to process the (successful) response from bitpay, to recover, it can simply find the order, and resubmit it again using the guid already saved with the order. This can be repeated until the client receives a response from bitpay invoice creation and then save the invoice data.

Having the sdk generate a guid and include it in the request provides (almost) no benefit over not including a guid at all. At best, an auto-generated guid is ineffective. The force-generated guid (without this PR) completely breaks idempotency feature. If a guid value is auto-generated if one is not provided will only be confusing to naive implementations that think its presence will provide them sufficient idempotency and will lead to over creation of bitpay objects like invoice, payroll payouts, etc.