Details
- Reviewers
machniak - Commits
- rKe3d358a8ea13: Coinbase payments
Diff Detail
- Repository
- rK kolab
- Branch
- dev/mollekopf
- Lint
Lint Warnings Severity Location Code Message Warning src/resources/lang/en/ui.php:513 PHPCS.W.Generic.Files.LineLength.TooLong Generic.Files.LineLength.TooLong - Unit
No Test Coverage - Build Status
Buildable 39176 Build 15923: arc lint + arc unit
Event Timeline
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.
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 | ||
320 | 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. |
src/tests/CoinbaseMocksTrait.php | ||
---|---|---|
32 | We should start using https://laravel.com/docs/9.x/http-client |
src/resources/lang/en/ui.php | ||
---|---|---|
513 | Missing dot at the end. | |
src/resources/vue/Wallet.vue | ||
321 | 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. | |
322 | 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 |
src/resources/vue/Wallet.vue | ||
---|---|---|
321 | 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. |