search

spreedly / r2d2

Ruby library for decrypting Google Pay and Android Pay payment tokens
MIT License
8 stars 15 forks source link

Raise exception on errors #5

Open rwdaigle opened 8 years ago

rwdaigle commented 8 years ago

Like we do in Gala, we should raise an error when known error states are encountered. Looking at the lib it looks like one such state is when the mac verification fails.

Perhaps DigestVerificationError?

mrezentes commented 8 years ago

I made errors for 3 foreseeable error scenarios -

https://github.com/spreedly/r2d2/blob/master/lib/r2d2/payment_token.rb#L10

https://github.com/spreedly/r2d2/blob/master/lib/r2d2/payment_token.rb#L65

https://github.com/spreedly/r2d2/blob/master/lib/r2d2/payment_token.rb#L32

mrezentes commented 8 years ago

@rwdaigle ruby code nitpicking requested for those errors.

rwdaigle commented 8 years ago

Great! Couple design points:

This library should have one job - to decrypt the payment token. So, we should do as little else as possible, including modifying the decrypted keys. Whatever the keys/values are in the token, we should just return it and not make any assumptions about key naming.

If we remove the key renaming, I believe we will also be able to remove the need to do any JSON.parse or JSON.generate. Just return whatever is decrypted and let the caller decide how to parse. This is especially important because, in Ruby, there are many JSON parsing libraries offering various pros/cons, so keeping this concern outside the scope of the library gives the caller more flexibility.

Also, looking at DecryptionError, I think we should let whatever the underlying exception is just bubble up. Catching an exception and re-typing it like that only serves to obscure the original error context (message, line number, stack trace, etc...).

If we do the above, I think we remove the need for DecryptedMessageUnparseableError and DecryptionError.

mrezentes commented 8 years ago

Exactly the type of feedback I was looking for. Thanks.