Page MenuHomekolab.org

Coinbase payments
ClosedPublic

Authored by mollekopf on May 24 2022, 6:16 PM.

Diff Detail

Repository
rK kolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mollekopf requested review of this revision.May 24 2022, 6:16 PM
mollekopf created this revision.
mollekopf added a project: Restricted Project.May 24 2022, 6:19 PM
mollekopf added a subscriber: Restricted Project.

Please note: While we could extend this to accept other cryptocurrencies, it currently assumes bitcoin only (and I configured coinbase to only accept bitcoin).
Accepting multiple currencies with a single "coinbase payment method" is feasible, but we currently assume a single currency per method.
In any case, I consider bitcoin only good enough for now, we can extend once we see a need for it.

Please note: While we could extend this to accept other cryptocurrencies, it currently assumes bitcoin only (and I configured coinbase to only accept bitcoin).
Accepting multiple currencies with a single "coinbase payment method" is feasible, but we currently assume a single currency per method.
In any case, I consider bitcoin only good enough for now, we can extend once we see a need for it.

machniak requested changes to this revision.May 27 2022, 11:11 AM
machniak added a subscriber: machniak.

I suppose you didn't use phpcs. Also, it does not apply on master, so I only reviewed the code and didn't run it.

src/app/Providers/Payment/Coinbase.php
105

Don't use meet config here.

151

\config('app.name')

152

Why not $payment['description']?

203

Don't assume ['error']['message'] exists.

226

return false

254

"Coinbase request signature verification failed".

273

I'd check here whether $data is an array and contains expected elements.

src/app/Providers/PaymentProvider.php
368

It would probably make sense to include coinbase method only if services.coinbase.key is set. So, we can easily disable it at any time. Or use app.payment.methods_*?

src/resources/lang/en/ui.php
514

Coinbase with a capital letter.

src/resources/vue/Wallet.vue
289

newWindowUrl would be clearer.

src/tests/Feature/Controller/PaymentsCoinbaseTest.php
109

Test with some more realistic values, e.g. 0.005. I see that currency_amount column is integer, so it might be an issue.

114

It would be also good to test an error response.

This revision now requires changes to proceed.May 27 2022, 11:11 AM
machniak added inline comments.Mon, May 30, 2:39 PM
src/tests/CoinbaseMocksTrait.php
32
mollekopf updated this revision to Diff 10313.Tue, May 31, 11:04 AM
mollekopf marked 11 inline comments as done.

Addressed comments

mollekopf updated this revision to Diff 10319.Tue, May 31, 11:21 AM

Test error responses handling

mollekopf updated this revision to Diff 10325.Tue, May 31, 11:38 AM

Rebased on master

mollekopf marked an inline comment as done.Tue, May 31, 11:43 AM
mollekopf added inline comments.
src/app/Providers/Payment/Coinbase.php
273

IMO it's fine to just crash if we get unexpected data.

src/tests/CoinbaseMocksTrait.php
32

Let's do that separately

machniak requested changes to this revision.Wed, Jun 1, 1:43 PM
machniak added inline comments.
src/resources/lang/en/ui.php
513

Missing dot at the end.

src/resources/vue/Wallet.vue
290

The browser blocks popup windows by default, so user has to allow popups. It's not perfect. I wonder whether it would make sense (if possible) to display the checkout page in an iframe inside the "Top up your wallet" dialog. I guess, popups will do for now.

291

It should be this.$refs.paymentDialog.hide()

src/tests/Browser/PaymentCoinbaseTest.php
79

Here we could test the checkout page content, so we know some more about the state of the integration. Also, the test should be skipped if services.coinbase.key is not set

This revision now requires changes to proceed.Wed, Jun 1, 1:43 PM
mollekopf marked 2 inline comments as done.Fri, Jun 3, 10:49 AM
mollekopf added inline comments.
src/resources/vue/Wallet.vue
290

I agree it kind of sucks, but I couldn't find a quick solution that was better. Perhaps an iframe would work indeed.

src/tests/Browser/PaymentCoinbaseTest.php
79

We're not doing that for other tests, (I think?) and personally I'm not a fan of conditionally skipping tests because it makes it harder to make sure you're testing everything.
I can do it if you prefer it though.

mollekopf updated this revision to Diff 10349.Fri, Jun 3, 10:51 AM

Addressed missing "." and hideDialog() via refs.

machniak accepted this revision.Fri, Jun 3, 11:20 AM
This revision is now accepted and ready to land.Fri, Jun 3, 11:20 AM
This revision was landed with ongoing or failed builds.Wed, Jun 8, 1:29 PM
Closed by commit rKe3d358a8ea13: Coinbase payments (authored by mollekopf). · Explain Why
This revision was automatically updated to reflect the committed changes.