Add Payzum as a crypto/stablecoin payment provider (async settlement via signed notification) - #4713
Add Payzum as a crypto/stablecoin payment provider (async settlement via signed notification)#4713payzum-hq wants to merge 2 commits into
Conversation
Adds Payzum through Omnipay, with the asynchronous settlement design agreed in LycheeOrg#4712: a signed inbound notification endpoint completes the order, and the buyer's return no longer fails a payment that is still confirming on-chain. - POST /api/v2/Shop/Checkout/Notify/Payzum/{order_id}: the driver verifies the HMAC-SHA-512 signature over the raw request bytes before any field is readable; order, amount and currency are checked and completion is idempotent across redeliveries. - handlePaymentReturn refreshes the invoice once for Payzum and leaves the order in PROCESSING while it confirms; the checkout page renders a new 'processing' state. - Config, enum, .env.example and composer entries for payzum/omnipay-payzum.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (27)
🚧 Files skipped from review as they are similar to previous changes (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughPayzum support adds provider configuration, asynchronous checkout handling, signed notification processing, pending-payment views, localized messages, and endpoint tests. ChangesPayzum provider setup
Asynchronous checkout returns
Signed Payzum notifications
Processing views and coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to Payzum adds asynchronous crypto-payment settlement with signed notifications and a processing state. The reviewed coverage confirms completed payments remain reachable by browser returns and are fulfilled only once, with no remaining merge-blocking risk. Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Docstring CoverageExplanation Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 32 files. (2 skipped: 2 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lang/ar/webshop.php (1)
288-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse snake_case for the new translation keys.
Rename
paymentProcessingandpaymentProcessingMessagetopayment_processingandpayment_processing_messagein all changed locale files. Update both the v7 and v8 Vue consumers to use the renamed keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 3e77f143-0002-49fd-b0b9-f2fcff57aaf8
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
.env.exampleapp/Actions/Shop/CheckoutService.phpapp/Enum/OmnipayProviderType.phpapp/Factories/OmnipayFactory.phpapp/Http/Controllers/Shop/CheckoutController.phpapp/Http/Requests/Checkout/FinalizeRequest.phpapp/Http/Requests/Checkout/NotifyRequest.phpcomposer.jsonconfig/omnipay.phplang/ar/webshop.phplang/bg/webshop.phplang/cz/webshop.phplang/de/webshop.phplang/el/webshop.phplang/en/webshop.phplang/es/webshop.phplang/fa/webshop.phplang/fr/webshop.phplang/hu/webshop.phplang/it/webshop.phplang/ja/webshop.phplang/nl/webshop.phplang/no/webshop.phplang/pl/webshop.phplang/pt/webshop.phplang/ru/webshop.phplang/sk/webshop.phplang/sv/webshop.phplang/tr/webshop.phplang/vi/webshop.phplang/zh_CN/webshop.phplang/zh_TW/webshop.phpresources/js/v7/components/webshop/CancelledFailed.vueresources/js/v8/components/webshop/CancelledFailed.vueroutes/api_v2_shop.phptests/Webshop/Checkout/CheckoutNotifyControllerTest.php
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
…table Addresses the review on LycheeOrg#4713: - Settlement now happens inside a transaction that locks and re-reads the order, so the browser return and an inbound notification racing each other can no longer both transition it and dispatch OrderCompleted twice. The loser adopts the fresh state and reports that it changed nothing. - Completing an order no longer replaces its transaction id with the gateway reference: that id is the lookup key of both the return and the notification URLs, so replacing it made the buyer's browser return fail to resolve whenever the notification arrived first. The Payzum invoice is still reachable by it, as their API reads an invoice by payment id or by order id. - Regression tests: exactly one OrderCompleted across redeliveries, the browser return still resolving (and not re-fulfilling) after an early notification. - Translation keys renamed to snake_case in every locale and both Vue consumers.
|
Thanks — all three were worth fixing, and two of them were real defects. Pushed in 5a8e46d. Atomic settlement. Settlement now runs inside a transaction that locks and re-reads the order ( Stable lookup key. You were right, and this was the more serious one: completing the order replaced Redelivery test. Added Translation keys renamed to
|
|
@payzum-hq it will take me a bit of time to review this PR, but it is on my radar. |
ildyria
left a comment
ildyria
left a comment
There was a problem hiding this comment.
The PR is in good shape, however there are parts which need refactoring otherwise the overall quality of Lychee will make it an unmaintainable dragon. And I would like to keep that one tamed.
| private function handleAsyncPaymentReturn(Order $order, PayzumGateway $gateway, array $metadata): Order | ||
| { | ||
| if ($order->status !== PaymentStatusType::PROCESSING) { | ||
| // Already settled, e.g. the notification landed before the redirect. |
There was a problem hiding this comment.
Are you 100% it is already settled ?
In normal flow we just throw:
throw new LycheeLogicException('Order with invalid status.');
| * | ||
| * @return Order The updated order | ||
| */ | ||
| public function handlePaymentNotification(Order $order): Order |
There was a problem hiding this comment.
This is adding to much code which is Payzum specific into the CheckoutService.
This approach is not clean and is more of a hack and will prevent Lychee from being maintainable in the future.
The flow of Mollie and Paypal are relatively similar.
Hower in this specific case this is WIDELY different due to the asynchronization.
I would recommend to put those functions in a different class and use injection for it so that the logic of Payzum is centralized.
| ); | ||
| } | ||
|
|
||
| if (!$success && $order->status === PaymentStatusType::PROCESSING) { |
There was a problem hiding this comment.
You need to add a gate on the provider here.
| */ | ||
| class NotifyRequest extends BaseApiRequest implements HasBasket | ||
| { | ||
| public const ORDER_ID_ATTRIBUTE = 'order_id'; |
There was a problem hiding this comment.
There is Request constant for this ?
If not, add it to the contracts instead of here.
| */ | ||
| public function authorize(): bool | ||
| { | ||
| return $this->order?->provider === OmnipayProviderType::PAYZUM && |
There was a problem hiding this comment.
This is very much Payzum oriented, I would be more in favour of having the asynchronous flow available globally with a provider such as Payzum as one of the option.
That would allow more extensibility in the future.
Look at strategy design pattern: https://refactoring.guru/design-patterns/strategy
2 participants
Implements the plan agreed in #4712 (option 1, the inbound notification endpoint @ildyria preferred). Adds Payzum, a non-custodial crypto/stablecoin gateway (USDC, USDT and more, multi-chain — funds settle directly to the photographer's own wallet), as an Omnipay provider.
Since crypto confirmation is asynchronous, the interesting part is settlement — following the pattern discussed in the issue:
POST /api/v2/Shop/Checkout/Notify/Payzum/{order_id}(shop.checkout.notify; keyed by order id, because completing an order replaces its transaction id with the provider reference and the gateway retries to the original URL): the Omnipay driver verifies the HMAC-SHA-512 signature over the raw request bytes (with a replay window) before any payload field is readable. After signature, order, amount and currency checks, the order is completed idempotently — a redelivered notification can never fulfil twice.OrderCompletedis dispatched here (guarded bywasChanged('status')) because the buyer may never come back to the browser.handlePaymentReturnno longer fails confirming payments: for Payzum the return handler polls the invoice once (fetchTransaction) and either completes, fails (expired/cancelled), or leaves the order in PROCESSING — never FAILED just because the buyer returned before the chain confirmed.finalizethen redirects to the checkout status page, where a newprocessingstate inCancelledFailed.vue(v7+v8, i18n key added to all locales with the English fallback) tells the buyer the payment is being confirmed and the page can be closed safely.PAYZUMcase inOmnipayProviderType(requiresapiKey+webhookSecret),config/omnipay.phpblock,.env.exampleentries.composer.json/lock gainpayzum/omnipay-payzum(MIT) and its only dependency, the officialpayzum/payzum-phpSDK (zero runtime deps); nothing else in the lock changes.FinalizeRequestadditionally authorizesCOMPLETEDPayzum orders, because the notification can legitimately complete the order before the buyer's browser returns.Tests:
CheckoutNotifyControllerTestcovers the happy path, forged signatures, pending statuses, expiry, amount mismatches, redelivery idempotency, unknown orders, and the finalize-keeps-processing behaviour. The wholetests/Webshopsuite passes (295 tests), as dophpstanandphp-cs-fixer.Disclosure: I work on Payzum and will maintain the integration long-term.
Summary by CodeRabbit
New Features
Bug Fixes