Page MenuHomePhorge

Coinbase payments
ClosedPublic

Authored by mollekopf on May 24 2022, 6:16 PM.
Tags
  • Restricted Project
Referenced Files
Unknown Object (File)
Wed, Mar 20, 8:43 PM
Unknown Object (File)
Wed, Mar 6, 6:05 AM
Unknown Object (File)
Fri, Mar 1, 10:43 PM
Unknown Object (File)
Feb 22 2024, 7:25 AM
Unknown Object (File)
Feb 12 2024, 10:59 PM
Unknown Object (File)
Feb 8 2024, 6:47 AM
Unknown Object (File)
Jan 29 2024, 7:37 AM
Unknown Object (File)
Jan 28 2024, 3:48 PM
Subscribers
Restricted Project

Diff Detail

Repository
rK kolab
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mollekopf created this revision.
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 subscribed.

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
src/tests/CoinbaseMocksTrait.php
32
mollekopf marked 11 inline comments as done.

Addressed comments

Test error responses handling

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.Jun 1 2022, 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.Jun 1 2022, 1:43 PM
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.

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

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