aviabird / gringotts Goto Github PK
View Code? Open in Web Editor NEWA complete payment library for Elixir and Phoenix Framework
Home Page: https://hexdocs.pm/gringotts/Gringotts.html
License: MIT License
A complete payment library for Elixir and Phoenix Framework
Home Page: https://hexdocs.pm/gringotts/Gringotts.html
License: MIT License
Review test cases for correct implementation.
Integration tests must test each request's life; for example, there should be a test for these:
authorize
-> capture
-> refund
authorize
-> void
Config related-error reporting was introduced in #7 and currently happens at runtime, before every call to Gringotts API.
We should validate the config at compile time to reduce the overhead.
The condition of if
should not be wrapped in parentheses.
Currently, we are passing currency as a string "USD"
in the application config :global_config
.
This can be improved to use atoms like :usd
or something which can be driven and controlled by the library instead of a plane string.
Currently, gringotts
support only credit card
based payment for Authorize.Net
.
The support for customer payment profile id
based payment for functions purchase, capture, refund and void is yet not provided.
The optional arguments are silently ignored. Need lots of functions that properly format the opts
contents and send it to MONEI. Reference
billing
cart
custom
customer
invoice_id
transaction_id
category
merchant
shipping
shipping_customer
Address
struct for *_address
.Currently, we have named workers which can only be one at a time and can be set in the config passed from the application.
This is only good when we have to run a worker at a time.
If we have to support multiple workers then either we have to move away from the named workers or use something like gproc etc which support that using ETS etc.
This was introduced in first_pass PR merge.
Some refs
https://www.amberbit.com/blog/2016/5/13/process-name-registration-in-elixir/ https://github.com/uwiger/gproc
https://m.alphasights.com/process-registry-in-elixir-a-practical-example-4500ee7c0dcc
https://medium.com/elixirlabs/registry-in-elixir-1-4-0-d6750fb5aeb
http://codeloveandboards.com/blog/2016/03/20/supervising-multiple-genserver-processes/
Support all optional fields provided by Paymill
Currently, we are passing the worker name in the Kuber.Hex.purchase(.,.,.) This is not the ideal behaviour.
Ideally, the application should have no knowledge of the workers etc. It should be managed at the lib level etc.
File has the variable name before the pattern while most of the files have the variable name after the pattern when naming parameter pattern matches
doctests will help us catch any stale examples in the documentation, in case the examples break due to changes in code.
The following need to be resolved for doctests to work:
:payment_worker
but there's no gateway configuration in the app.The Authorize.Net gateway only supports USD for now.
Create the functionality to support other currencies.
Integration tests must test each request's life; for example, there should be a test for these:
authorize
-> capture
-> refund
authorize
-> void
When a network time out happens we need to handle that gracefully.
The current version of the library supports config
in which the values have to be hard coded
config :Gringotts, Gringotts.Gateways.Stripe,
adapter: Gringotts.Gateways.Stripe,
api_key: "sk_test_vIX41hC0sdfBKrPWQerLuOMld",
default_currency: "USD"
But, the values for the config should be picked from the system environment variables.
config :Gringotts, Gringotts.Gateways.Stripe,
adapter: System.get_env("ADAPTER"),
api_key: System.get_env("API_KEY"),
Review test cases for correct implementation
Large numbers should be written with underscores: 20_204
There is no whitespace around parentheses/brackets most of the time, but here there is.
File has the variable name before the pattern while most of the files have the variable name after the pattern when naming parameter pattern matches
standard we have followed in all the calls is id, amount, config
but this is not followed in the refund method of the worker.ex file.
Make changes in this format as per the standard.
This would require change in all the gateway files.
Most gateways provide some kind of client-side SDK or JS libs to allow client browsers to direct the payment data directly to the gateway, allowing even PCI-DSS non-compliant merchants to use their API and services.
Out of the gateways currently supported (v1.0.2
), only Stripe implementation provides a checkout
feature.
checkout
method in the Public API?Add checkout for these gateways:
Review test cases for correct implementation
Currently, we are using the gateway expected case styling in the gateway config block in the application code.
This introduces a lot of disparity in between the gateway configs.
Proposal:-
We have a common style and we create a map at gateway level in library to interface gateway keys with config application level keys.
Changes to be made in all the existing gateways;
This would ensure that we have a consistent API for config.
When the api takes more time to respond, the Genserver timeouts.
One options is to wait for the request to complete using
call(worker, {:authorize, gateway, amount, card, opts}, :infinity)
There is third filed which whcih specifies time in milliseconds for the handle_call of genserver. Default is 5s
It's clear that using floating point numbers for representing money is a bad idea, and using plain integers fairly limits the application since there exist currencies with more than 2 decimal places. This rules out supporting money
as it uses integers.
ex_money
and monetized
both use Decimal
s to represent money, but ex_money
offers some more features than monetized
, like:
ex_cldr
. These rules define the number of fractional digits for a currency and the rounding increment where appropriate.ex_money
).ex_money
There's not much to gain for Gringotts, all it does is format the amount
(as documented in the gateway or as per ISO4127) to construct a correct POST
request for the gateway. The real benefit is to merchants already using ex_money
, or planning to do so. They won't have to convert their super awesome Money
structs into floats
/ints
/string
and be sure that Gringotts does not cause them any loss.
amount
parameter will now accept only Money.t
.
amount = ~M[12.34]USD # or Money.new("USD", 12.34)
Gringotts.purchase(:payment_worker, Gringotts.Gateways.XYZ, amount, card, opts)
Money.t
only to convert them back into plain strings in the end.1/100th units
some in units
, so if we allow integers, we'll have to decide if Gringotts works in 1/100th units
or unit
, then enforce that in our API. Let's just use Money.t
instead as our unambiguous "protocol".Any merchant doing even trivial financial accounting in their app would be using some Money lib.
ex_money
.Money.t
struct from whatever your representation is and pass that in Gringott's API methods.Let's use ex_money
or a protocol and track it's integration into Gringotts here.
Abstract/extract following modules.
Checklist
AuthorizeNet
converts Decimal
to float
, defeating the purpose of the protocol.
Currently, the support is to set the mode
as "test" or "prod" for 'sandbox' or 'live' account in the global config of application with respect to a gateway. By default if no mode is set then the "test" mode is used.
config :gringotts, :global_config,
mode: :test
But, this configuration should be made mandatory and a compile time exception should be raised if the field is not set.
order_id is not received is response after successful transaction.
Need to formatted properly.
to_integer
(for lack of a better name)@spec to_integer(Money.t) :: {ISO4217 :: String.t, amount :: integer, exponent :: integer}
The exponent
is not used and is just being made available to the application if need be.
This is useful for gateways that require amount as integer
(like cents instead of dollars)
Examples:
usd_price = Money.new(4.1234, :USD)
Gringotts.Money.to_integer(usd_price) #=> {"USD", 412, -2}
bhd_price = Money.new(4.1234, :BHD)
Gringotts.Money.to_integer(bhd_price) #=> {"BHD", 4123, -3}
# the Bahraini dinar is divided into 1000 fils unlike the dollar which is divided in 100 cents
to_string
@spec to_string(Money.t) :: {ISO4217 :: String.t, amount :: String.t}
Returns a tuple of ISO4217
currency code and the amount
as string
. The amount
must match this regex: ~r[\d+\.\d\d{n}]
where n+1
should match the required precision for the currency
. Gringotts cannot and will not validate this of course.
Examples:
usd_price = Money.new(4.1234, :USD)
Gringotts.Money.to_string(usd_price) #=> {"USD", 4.12}
bhd_price = Money.new(4.1234, :BHD)
Gringotts.Money.to_string(bhd_price) #=> {"BHD", 4.123}
In wiredcard gateway inside authorize method we are trying to manipulate the options depending on type of payment method using method
check_and_return_payment_method(payment_method)
however same could be done by using pattern matching feature of elixir by creating two identical functions with params with diff type and pattern match the input params based on it.
gringotts.new
Generates just a gateway implementation file, it should also create:
test/gateways/<gateway>_test.exs
test/integration/gateways/<gateway>_test.exs
Which mocking library do we endorse, bypass
or mox
(I'm strongly against mock
)?
The task has no coverage, take inspiration from phx.new
.
Response
We can add arbitrary fields in a Response
thanks to Response.new()
.
Field | type | remarks |
---|---|---|
:success |
bool | |
:authorization |
?? | holds the id/token |
:status_code |
string | HTTP response code |
:error_code |
string | the gateway's response code, used for both error and success codes from gateway |
:message |
string | descriptive string from gateway or gringotts |
:avs_result |
tuple of string |
{address_status, zip_code_status} |
:cvc_result |
string | cvc_status |
:params |
?? | raw response |
:success
field seems redundant as all API calls return the usual {:ok, Response}
or {:error, Response}
:id
instead of :authorization
IMO.:token
(:authorization
is also fine).:ok
and :error
, but we could,
:ok
, :error
, :declined
, :invalid
, etc.:ok
and :error
tuples, but also add a reason
or error_reason
field:error_reason: {:server_error, "reason"} | {:invalid_format, "reason"} | {:network_error, "reason"}`
"reason"
is the error msg string returned by the gateway.:params
to :raw
{:html, String.t}
, {:json, Map.t}
(already decoded), {:xml, ??}
:avs_result
is a tuple, a map would be better as the keys would be explicit,
%{address: string, zip_code: string}
New definition:
Field (old) | Field (new) | type | remarks |
---|---|---|---|
:success |
boolean | (deprecated) will remove in future release | |
:authorization |
id |
string | (renamed) transaction ID |
:token |
string | (new) The token obtained when card details are stored for future use | |
:status_code |
:http_code |
integer | (new) HTTP response code |
:error_code |
:gateway_code |
string | (re-purposed) result code from gateway "as it is" |
:reason |
{:tag, string} |
(only for error, nil otherwise) tags like :server_error , :fraud_risk , :avs_invalid |
|
:message |
string | (no change) textual description of result code if any | |
:avs_result |
map | (converted to map) %{address: string, zip_code: string} |
|
:cvc_result |
string | (no change) | |
:params |
:raw |
{:tag, string} |
(renamed) html, string}, {:json, map}` |
Review test cases for correct implementation
Let's run all tests together, excluding those that are explicitly marked :skip
.
script:
- mix coveralls.travis --include integration
--trace
flag is redundant as traces are already enabled in the test_helper.exs
Cond statements should contain at least two conditions besides true
.
Integration tests must test each request's life; for example, there should be a test for these:
authorize
-> capture
-> refund
authorize
-> void
The merchant should have the flexibility to switch between test and the production url.
This is done so that they can use the test url to check their sandbox account, while the production url would be used for the main account.
The opts field will have the following key-value pair. Depending on this test or production url is set.
[ { mode: "test" } ]
There should be no unused return values for Keyword functions.
In Stripe Gateway, in card_params
and address_params
functions I think the struct is unnecessarily converted to map inorder to access the values. which could be easliy done using [dot] operator.
E.g:
card.name
Please go through the link to see how to access values in struct.
Get a value from a struct
defp card_params(%CreditCard{} = card) do
card = Map.from_struct(card)
[ "card[name]": card[:name],
"card[number]": card[:number],
"card[exp_year]": card[:year],
"card[exp_month]": card[:month],
"card[cvc]": card[:verification_code]
]
end
Review test cases for correct implementation
Examples and the tests do not have the tax
, shipping
and duty
fields in the opts
. Though they are optional for ANet, the implementation assumes they are present in opts
(so they are mandatory right now).
When they are absent, the implementation spits out empty tags which ANet doesn't like:
<tax>
</>
<name/>
<description/>
</tax>
<duty>
</>
<name/>
<description/>
</duty>
<shipping>
</>
<name/>
<description/>
</shipping>
The error:
{:error,
%Gringotts.Response{authorization: nil, avs_result: nil, cvc_result: nil,
error_code: "E00003", fraud_review: nil,
message: "Name cannot begin with the '>' character, hexadecimal value 0x3E. Line 36, position 6.",
params: %{"ErrorResponse" => %{"messages" => %{"message" => %{"code" => "E00003",
"text" => "Name cannot begin with the '>' character, hexadecimal value 0x3E. Line 36, position 6."},
"resultCode" => "Error"}}}, status_code: 200, success: false}}
The certain requirements which are common to all the gateways e.g. currency
can be kept in the global config
.
These should be overridden if something else is set by the gateway.
Example
Global configuration
config :gringotts, :global_config,
mode: :test
default_currency: 'USD'
Gateway specific configuration
# Keep the `key` name same as the adapter name
config :gringotts, Gringotts.Gateways.Stripe,
adapter: Gringotts.Gateways.Stripe,
api_key: "sk_test_vIX41hayC0BKrPWQerLuOMld",
default_currency: "EUR"
We are just adding test
mode in the global_config
.
Refer stripe.ex
{api_key, ""} passing this parameter as empty string does not make sense here.
defp commit(method, path, params, opts) do
config = Keyword.fetch!(opts, :config)
# TODO: credentials should be investigated why it is {api_key, ""}
# Did to mimic the earlier behaviour.
method
|> http("#{@base_url}/#{path}", params, credentials: {config[:api_key], ""})
|> respond
end
Please do necessary investigation for this one and fix it.
Edit:
http
and money utils from Base
random*
methods from Bogus
Library spits out a weird error message when the key name in the application config does not match the required_config
of the gateways in lib.
** (exit) exited in: GenServer.call(:payment_worker, {:authorize, Gringotts.Gateways.Stripe, 5, nil, [currency: "usd"]}, 5000)
** (EXIT) an exception was raised:
** (ArgumentError) argument error
:erlang.byte_size(nil)
(gringotts) lib/gringotts/gateways/stripe.ex:321: Gringotts.Gateways.Stripe.commit/4
(gringotts) lib/gringotts/worker.ex:26: Gringotts.Worker.handle_call/3
(stdlib) gen_server.erl:636: :gen_server.try_handle_call/4
(stdlib) gen_server.erl:665: :gen_server.handle_msg/6
(stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3
(elixir) lib/gen_server.ex:774: GenServer.call/3
We can definitely improve this by showing an error message which specifies that we have used wrong key names which are not present in the use Gringotts.Adapter, required_config: [:secret_key, :default_currency]
of the gateway file.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.