rafactor: payment#4089
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Waffo Pancake payment integration: backend service for signed checkout sessions and webhook verification, controller endpoints to initiate payments and receive webhooks, model recharge logic, settings/options wiring, frontend settings and top-up UI, and unit tests. Changes
Sequence DiagramsequenceDiagram
actor User
participant FE as Frontend (React)
participant API as Backend (Gin API)
participant SDK as WaffoPancake Service
participant DB as Database
participant Provider as External Payment Provider
User->>FE: click "Waffo Pancake 充值" (amount)
FE->>API: POST /api/user/waffo-pancake/pay { amount }
activate API
API->>DB: create pending TopUp (tradeNo)
API->>SDK: CreateWaffoPancakeCheckoutSession(params)
activate SDK
SDK-->>API: { checkout_url, session_id, expires_at }
deactivate SDK
API-->>FE: { checkout_url, order_id }
deactivate API
FE->>User: open checkout popup
User->>Provider: complete payment
Provider->>API: POST /api/waffo-pancake/webhook (payload + X-Waffo-Signature)
activate API
API->>SDK: VerifyConfiguredWaffoPancakeWebhook(payload, sig)
API->>SDK: ExtractWaffoPancakeTradeNo(event)
API->>DB: lock TopUp row FOR UPDATE
API->>DB: RechargeWaffoPancake(tradeNo) -> update status, add quota
DB-->>API: success
API-->>Provider: 200 OK
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
model/topup.go (1)
439-494: Consolidate duplicated Waffo recharge transaction logic.
RechargeWaffoandRechargeWaffoPancakenow share almost identical idempotency + transaction + quota-credit code. This is drift-prone when one path gets patched and the other is missed.♻️ Proposed refactor sketch
+func rechargeWaffoByTradeNo(tradeNo string) (topUp *TopUp, quotaToAdd int, err error) { + // shared lock/query/status/quota/update logic +} + func RechargeWaffo(tradeNo string) (err error) { - // duplicated logic... + topUp, quotaToAdd, err := rechargeWaffoByTradeNo(tradeNo) + // provider-specific logging } func RechargeWaffoPancake(tradeNo string) (err error) { - // duplicated logic... + topUp, quotaToAdd, err := rechargeWaffoByTradeNo(tradeNo) + // provider-specific logging }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/topup.go` around lines 439 - 494, Both RechargeWaffo and RechargeWaffoPancake duplicate the same idempotency + transaction + quota-credit logic; extract that common flow into a single helper (e.g., processWaffoTopUp or applyTopUpTransaction) that accepts the identifying tradeNo and any source-specific metadata, performs the FOR UPDATE lookup on TopUp (mirroring the current refCol logic), validates status, computes quotaToAdd, updates TopUp (CompleteTime/Status) and increments User.quota inside one DB.Transaction, and returns the updated TopUp, quotaToAdd and an error; then simplify RechargeWaffo and RechargeWaffoPancake to call this helper and only handle post-success logging (RecordLog) and error mapping. Ensure the helper preserves existing behaviors: PostgreSQL quoting, idempotent success return (no-op if already success), same error messages, and calling tx.Save / tx.Model updates as currently implemented.controller/topup_waffo_pancake.go (2)
169-171: Ignored error when updating TopUp status to failed.If
topUp.Update()fails, the database will have a stale pending record that never transitions to failed. Consider logging the error to aid debugging.Proposed fix
log.Printf("create Waffo Pancake checkout session failed: %v", err) topUp.Status = common.TopUpStatusFailed - _ = topUp.Update() + if updateErr := topUp.Update(); updateErr != nil { + log.Printf("failed to mark Waffo Pancake topup as failed: %v, trade_no=%s", updateErr, tradeNo) + } c.JSON(200, gin.H{"message": "error", "data": "拉起支付失败"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/topup_waffo_pancake.go` around lines 169 - 171, The call to topUp.Update() ignores errors causing stale pending records; change the code around setting topUp.Status = common.TopUpStatusFailed to capture the returned error from topUp.Update(), handle it (e.g., log via your logger or c.Error/c.JSON with an internal error message), and only proceed to respond after ensuring the update succeeded or after logging the update error; reference the topUp variable and the topUp.Update() method and ensure the failure path logs the error before sending c.JSON(200, gin.H{...}).
206-216: Silently returning OK for missing trade number may hide webhook issues.When
tradeNois empty, the handler logs and returns200 OK. This prevents retries, but if the metadata is malformed due to a bug, the order will never be fulfilled. Consider whether this should be a400to trigger investigation, or add monitoring/alerting for this case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/topup_waffo_pancake.go` around lines 206 - 216, The handler currently logs and returns 200 OK when tradeNo is empty (in the block using service.ExtractWaffoPancakeTradeNo and event.NormalizedEventType), which suppresses retries and hides malformed webhook metadata; change this to return a client/error status (e.g., c.String(400, "missing trade no") or 422) instead of 200, enrich the log (replace log.Printf call) to include event.ID and raw payload/metadata, and emit/record a monitoring metric or alert (e.g., call metrics.IncWebhookMalformed or monitoring.Alert) so missing trade numbers are visible and actionable rather than silently ignored.web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx (3)
300-307:min={0}allows zero unit price, but validation requires> 0.The
Form.InputNumberforWaffoPancakeUnitPricesetsmin={0}, but the submit validation at line 141 rejects values<= 0. Setmin={0.01}to provide consistent feedback and prevent user confusion.Proposed fix
<Form.InputNumber field='WaffoPancakeUnitPrice' precision={2} label={t('充值价格（x元/美金）')} placeholder='1.0' - min={0} + min={0.01} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx` around lines 300 - 307, The Form.InputNumber for WaffoPancakeUnitPrice currently sets min={0} which contradicts the form submit validation that rejects values <= 0; update the InputNumber to use min={0.01} so the UI prevents zero input and matches the submit-side check (locate the Form.InputNumber with field='WaffoPancakeUnitPrice' in SettingsPaymentGatewayWaffoPancake.jsx and align it with the submit validation logic that enforces > 0).
180-193: Parallel API calls may result in partial updates without clear feedback.Using
Promise.allmeans all requests run in parallel. If some succeed and some fail, the user sees error toasts for failures but no indication of what succeeded. The earlyreturnat line 192 also preventsprops.refresh?.()and the success toast, leaving the UI potentially out of sync.Consider using
Promise.allSettledto handle partial success more gracefully, or switch to sequential updates if atomicity is important.Alternative approach using Promise.allSettled
- const results = await Promise.all( - options.map((opt) => - API.put('/api/option/', { - key: opt.key, - value: opt.value, - }), - ), - ); - - const errorResults = results.filter((res) => !res.data.success); - if (errorResults.length > 0) { - errorResults.forEach((res) => showError(res.data.message)); - return; - } + const results = await Promise.allSettled( + options.map((opt) => + API.put('/api/option/', { + key: opt.key, + value: opt.value, + }), + ), + ); + + const failures = results.filter( + (r) => r.status === 'rejected' || !r.value?.data?.success, + ); + if (failures.length > 0) { + failures.forEach((r) => { + const msg = r.status === 'rejected' ? r.reason : r.value?.data?.message; + showError(msg || t('更新失败')); + }); + if (failures.length < results.length) { + showSuccess(t('部分设置已更新')); + props.refresh?.(); + } + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx` around lines 180 - 193, The current bulk update uses Promise.all with options.map and then filters results into errorResults and early returns, which can leave partial updates and skip props.refresh?.() and the success toast; change the implementation to use Promise.allSettled for the API.put calls (or perform them sequentially if you require atomicity), inspect settled results to separate fulfilled vs rejected (check result.status and result.value.data.success), call showError for each failure and show a single success toast if at least one update succeeded, then always call props.refresh?.() so the UI is consistent; update references to results, errorResults, API.put, showError, and props.refresh?.() in the function accordingly.
95-98: UseparseIntfor integer fieldWaffoPancakeMinTopUp.
WaffoPancakeMinTopUprepresents a count (minimum number of units), soparseFloatmay introduce floating-point precision issues. UseparseIntto match the field's semantics.Proposed fix
WaffoPancakeMinTopUp: props.options.WaffoPancakeMinTopUp !== undefined - ? parseFloat(props.options.WaffoPancakeMinTopUp) + ? parseInt(props.options.WaffoPancakeMinTopUp, 10) : 1,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx` around lines 95 - 98, The WaffoPancakeMinTopUp value is an integer count but currently uses parseFloat; change the parsing to parseInt with an explicit radix (10) so the field is interpreted as an integer and falls back to 1 when undefined or NaN (update the code around WaffoPancakeMinTopUp in SettingsPaymentGatewayWaffoPancake.jsx to use parseInt(props.options.WaffoPancakeMinTopUp, 10) and ensure a default of 1 if the parsed result is NaN).web/src/components/topup/RechargeCard.jsx (1)
426-426: Empty catch block silently swallows errors.Consider logging the error or removing the try-catch if the default
usdRate = 7fallback is intentional for all failure cases.Proposed fix
- } catch (e) {} + } catch (e) { + console.warn('Failed to parse status for USD rate:', e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/topup/RechargeCard.jsx` at line 426, The empty catch in RechargeCard.jsx silently swallows errors when computing usdRate (the try-catch around the usdRate assignment), so either remove the try-catch if you intend to always use the fallback usdRate = 7, or log the error inside the catch (e.g., console.error or your app logger) and then keep the fallback; update the catch block in the function/component that sets usdRate to include a meaningful log message (reference variable usdRate and the try block that computes it) rather than an empty catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/topup_waffo_pancake.go`:
- Around line 218-219: The current in-process LockOrder/UnlockOrder (used in
handlers like the Waffo webhook where LockOrder(tradeNo) and defer
UnlockOrder(tradeNo) are called) uses a process-local sync.Map and won’t prevent
races in multi-instance deployments; replace this with a distributed lock
(preferred: Redis-based lock using the existing Redis in the stack, or DB
row-level locking via a SELECT ... FOR UPDATE inside a transaction that scopes
the webhook processing) so that concurrent webhook requests for the same tradeNo
across instances are serialized; alternatively, if you intend to keep the
process-local LockOrder/UnlockOrder, add clear documentation of the
single-instance deployment constraint and change usages (e.g., in the Waffo
handler) to use the new distributed lock API instead of LockOrder/UnlockOrder.
In `@controller/topup.go`:
- Around line 82-86: The enableWaffoPancake boolean currently only checks
merchant/private/store/product IDs; include the webhook public-key readiness in
this gating by adding a non-empty trimmed check for the webhook public key (e.g.
strings.TrimSpace(setting.WaffoPancakeWebhookPublicKey) != "") into the same
expression that defines enableWaffoPancake so callback verification won't be
skipped when the key is missing; update the enableWaffoPancake assignment
accordingly (and optionally add a log/warning elsewhere if the webhook key is
missing).
In `@service/waffo_pancake.go`:
- Around line 334-343: The current verifyWaffoPancakeWebhookWithResolvedKeys
function falls back to test when opts.environment is empty, allowing test-signed
webhooks in prod; change it to require an explicit environment and remove the
prod->test fallback: have verifyWaffoPancakeWebhookWithResolvedKeys accept only
"prod" or "test" (return a clear error for any other/empty value), call
resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys) once and
pass that to verifyWaffoPancakeWebhookWithKey, and ensure callers (e.g.,
VerifyConfiguredWaffoPancakeWebhook) always provide opts.environment.
- Around line 173-181: VerifyConfiguredWaffoPancakeWebhook currently omits the
sandbox environment flag so verifyWaffoPancakeWebhook falls back to trying both
keys; update VerifyConfiguredWaffoPancakeWebhook to set opts.environment using
setting.WaffoPancakeSandbox (or equivalent) so the
waffoPancakeWebhookVerifyOptions.environment field is populated, ensuring
verifyWaffoPancakeWebhookWithResolvedKeys uses only the correct environment
(Test or Prod) when resolving publicKeys and prevents acceptance of the
hardcoded test key in production.
- Around line 154-158: The code is using http.DefaultClient.Do(req) which has no
timeout and can hang; replace it by creating a dedicated http.Client with a
sensible Timeout (e.g., 10s) and use client.Do(req) instead of
http.DefaultClient.Do; update the scope where resp and err are obtained (the
block using req, resp, err and defer resp.Body.Close()) so it uses this new
client (refer to the variables req, resp, err and the call currently to
http.DefaultClient.Do) and ensure any context-based cancellation is preserved if
the surrounding function accepts a context.
In `@web/src/components/settings/PaymentSetting.jsx`:
- Line 140: The current updater setInputs((prev) => ({ ...prev, ...newInputs }))
preserves prior values for keys that the backend omits, which can re-submit
stale Waffo Pancake credentials; change the update logic in the PaymentSetting
component so that when handling Waffo Pancake payloads you either replace the
credentials object entirely or explicitly clear sensitive fields omitted by
newInputs (e.g., merchantKey, privateKey) instead of merging with prev—locate
the call to setInputs and make the merge conditional: if newInputs contains a
Waffo Pancake credentials object, use that object as the source of truth (or set
omitted sensitive keys to null/empty) rather than shallow-merging with prev.
In `@web/src/i18n/locales/zh-CN.json`:
- Line 2345: Remove the duplicate translation entry for the key "未启用" so only a
single mapping for that key remains in the JSON; locate both occurrences of the
string "未启用" in the locale file, delete the later duplicate (or merge if
translations differ), and validate the JSON afterwards to ensure no trailing
commas or syntax errors.
---
Nitpick comments:
In `@controller/topup_waffo_pancake.go`:
- Around line 169-171: The call to topUp.Update() ignores errors causing stale
pending records; change the code around setting topUp.Status =
common.TopUpStatusFailed to capture the returned error from topUp.Update(),
handle it (e.g., log via your logger or c.Error/c.JSON with an internal error
message), and only proceed to respond after ensuring the update succeeded or
after logging the update error; reference the topUp variable and the
topUp.Update() method and ensure the failure path logs the error before sending
c.JSON(200, gin.H{...}).
- Around line 206-216: The handler currently logs and returns 200 OK when
tradeNo is empty (in the block using service.ExtractWaffoPancakeTradeNo and
event.NormalizedEventType), which suppresses retries and hides malformed webhook
metadata; change this to return a client/error status (e.g., c.String(400,
"missing trade no") or 422) instead of 200, enrich the log (replace log.Printf
call) to include event.ID and raw payload/metadata, and emit/record a monitoring
metric or alert (e.g., call metrics.IncWebhookMalformed or monitoring.Alert) so
missing trade numbers are visible and actionable rather than silently ignored.
In `@model/topup.go`:
- Around line 439-494: Both RechargeWaffo and RechargeWaffoPancake duplicate the
same idempotency + transaction + quota-credit logic; extract that common flow
into a single helper (e.g., processWaffoTopUp or applyTopUpTransaction) that
accepts the identifying tradeNo and any source-specific metadata, performs the
FOR UPDATE lookup on TopUp (mirroring the current refCol logic), validates
status, computes quotaToAdd, updates TopUp (CompleteTime/Status) and increments
User.quota inside one DB.Transaction, and returns the updated TopUp, quotaToAdd
and an error; then simplify RechargeWaffo and RechargeWaffoPancake to call this
helper and only handle post-success logging (RecordLog) and error mapping.
Ensure the helper preserves existing behaviors: PostgreSQL quoting, idempotent
success return (no-op if already success), same error messages, and calling
tx.Save / tx.Model updates as currently implemented.
In `@web/src/components/topup/RechargeCard.jsx`:
- Line 426: The empty catch in RechargeCard.jsx silently swallows errors when
computing usdRate (the try-catch around the usdRate assignment), so either
remove the try-catch if you intend to always use the fallback usdRate = 7, or
log the error inside the catch (e.g., console.error or your app logger) and then
keep the fallback; update the catch block in the function/component that sets
usdRate to include a meaningful log message (reference variable usdRate and the
try block that computes it) rather than an empty catch.
In `@web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx`:
- Around line 300-307: The Form.InputNumber for WaffoPancakeUnitPrice currently
sets min={0} which contradicts the form submit validation that rejects values <=
0; update the InputNumber to use min={0.01} so the UI prevents zero input and
matches the submit-side check (locate the Form.InputNumber with
field='WaffoPancakeUnitPrice' in SettingsPaymentGatewayWaffoPancake.jsx and
align it with the submit validation logic that enforces > 0).
- Around line 180-193: The current bulk update uses Promise.all with options.map
and then filters results into errorResults and early returns, which can leave
partial updates and skip props.refresh?.() and the success toast; change the
implementation to use Promise.allSettled for the API.put calls (or perform them
sequentially if you require atomicity), inspect settled results to separate
fulfilled vs rejected (check result.status and result.value.data.success), call
showError for each failure and show a single success toast if at least one
update succeeded, then always call props.refresh?.() so the UI is consistent;
update references to results, errorResults, API.put, showError, and
props.refresh?.() in the function accordingly.
- Around line 95-98: The WaffoPancakeMinTopUp value is an integer count but
currently uses parseFloat; change the parsing to parseInt with an explicit radix
(10) so the field is interpreted as an integer and falls back to 1 when
undefined or NaN (update the code around WaffoPancakeMinTopUp in
SettingsPaymentGatewayWaffoPancake.jsx to use
parseInt(props.options.WaffoPancakeMinTopUp, 10) and ensure a default of 1 if
the parsed result is NaN).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7f7f4f5-7cf3-41e1-8a73-b58cab233604
📒 Files selected for processing (12)
controller/topup.gocontroller/topup_waffo_pancake.gomodel/option.gomodel/topup.gorouter/api-router.goservice/waffo_pancake.gosetting/payment_waffo_pancake.goweb/src/components/settings/PaymentSetting.jsxweb/src/components/topup/RechargeCard.jsxweb/src/components/topup/index.jsxweb/src/i18n/locales/zh-CN.jsonweb/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx
Comment on lines
+218
to
+219
| LockOrder(tradeNo) | ||
| defer UnlockOrder(tradeNo) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other webhook handlers in the codebase handle locking
rg -n -A5 "LockOrder|UnlockOrder" --type=goRepository: QuantumNous/new-api
Length of output: 5240
🏁 Script executed:
# View the complete LockOrder/UnlockOrder implementation
sed -n '278,310p' controller/topup.goRepository: QuantumNous/new-api
Length of output: 678
🏁 Script executed:
# Check if there's documentation or deployment configuration
fd -e md -e yaml -e yml -e json -e toml | head -20Repository: QuantumNous/new-api
Length of output: 459
🏁 Script executed:
# Search for deployment or distributed deployment references
rg -i "(distributed|deployment|multi.instance|cluster)" --type=go -l | head -10Repository: QuantumNous/new-api
Length of output: 312
🏁 Script executed:
# Check if Redis is configured and used elsewhere in the codebase
rg -i "redis" --type=go -l | head -10Repository: QuantumNous/new-api
Length of output: 282
🏁 Script executed:
# Check docker-compose.yml for multi-instance setup
cat docker-compose.ymlRepository: QuantumNous/new-api
Length of output: 3338
🏁 Script executed:
# Check README for deployment guidance
head -100 README.md | grep -i -A5 "deploy\|instance\|distributed"Repository: QuantumNous/new-api
Length of output: 201
🏁 Script executed:
# Search for any comments about single-instance assumptions in topup.go
rg -B5 -A5 "LockOrder|orderLocks" controller/topup.go | head -50Repository: QuantumNous/new-api
Length of output: 953
In-process locking is insufficient for distributed deployments.
The LockOrder/UnlockOrder implementation uses a process-local sync.Map. The docker-compose.yml includes a comment indicating multi-node deployment is a supported scenario (SESSION_SECRET=random_string # multi-node deployment, set this to a random string). If multiple instances are deployed, concurrent webhook requests for the same tradeNo across instances can race.
This pattern is pervasive across all webhook handlers (Epay, Stripe, Creem, Waffo, etc.). For multi-instance deployments, switch to database-level locking (e.g., SELECT ... FOR UPDATE) or Redis-based locking (Redis is already in the stack). If single-instance deployment is the only supported mode, explicitly document this constraint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/topup_waffo_pancake.go` around lines 218 - 219, The current
in-process LockOrder/UnlockOrder (used in handlers like the Waffo webhook where
LockOrder(tradeNo) and defer UnlockOrder(tradeNo) are called) uses a
process-local sync.Map and won’t prevent races in multi-instance deployments;
replace this with a distributed lock (preferred: Redis-based lock using the
existing Redis in the stack, or DB row-level locking via a SELECT ... FOR UPDATE
inside a transaction that scopes the webhook processing) so that concurrent
webhook requests for the same tradeNo across instances are serialized;
alternatively, if you intend to keep the process-local LockOrder/UnlockOrder,
add clear documentation of the single-instance deployment constraint and change
usages (e.g., in the Waffo handler) to use the new distributed lock API instead
of LockOrder/UnlockOrder.
Comment on lines
+154
to
+158
| resp, err := http.DefaultClient.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("request Waffo Pancake checkout session: %w", err) | ||
| } | ||
| defer resp.Body.Close() |
Contributor
There was a problem hiding this comment.
HTTP client without timeout may hang indefinitely.
http.DefaultClient has no timeout configured. If the Waffo Pancake API is slow or unresponsive, the goroutine will block indefinitely, potentially exhausting resources.
Proposed fix: Use a client with timeout
+var waffoPancakeHTTPClient = &http.Client{
+ Timeout: 30 * time.Second,
+}
+
func CreateWaffoPancakeCheckoutSession(ctx context.Context, params *WaffoPancakeCreateSessionParams) (*WaffoPancakeCheckoutSession, error) {
// ... existing code ...
- resp, err := http.DefaultClient.Do(req)
+ resp, err := waffoPancakeHTTPClient.Do(req)
if err != nil {
return nil, fmt.Errorf("request Waffo Pancake checkout session: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resp, err := http.DefaultClient.Do(req) | |
| if err != nil { | |
| return nil, fmt.Errorf("request Waffo Pancake checkout session: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| var waffoPancakeHTTPClient = &http.Client{ | |
| Timeout: 30 * time.Second, | |
| } | |
| func CreateWaffoPancakeCheckoutSession(ctx context.Context, params *WaffoPancakeCreateSessionParams) (*WaffoPancakeCheckoutSession, error) { | |
| // ... existing code ... | |
| resp, err := waffoPancakeHTTPClient.Do(req) | |
| if err != nil { | |
| return nil, fmt.Errorf("request Waffo Pancake checkout session: %w", err) | |
| } | |
| defer resp.Body.Close() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/waffo_pancake.go` around lines 154 - 158, The code is using
http.DefaultClient.Do(req) which has no timeout and can hang; replace it by
creating a dedicated http.Client with a sensible Timeout (e.g., 10s) and use
client.Do(req) instead of http.DefaultClient.Do; update the scope where resp and
err are obtained (the block using req, resp, err and defer resp.Body.Close()) so
it uses this new client (refer to the variables req, resp, err and the call
currently to http.DefaultClient.Do) and ensure any context-based cancellation is
preserved if the surrounding function accepts a context.
| }); | ||
|
|
||
| setInputs(newInputs); | ||
| setInputs((prev) => ({ ...prev, ...newInputs })); |
Contributor
There was a problem hiding this comment.
Avoid preserving stale Waffo Pancake credentials when option payload is partial.
setInputs((prev) => ({ ...prev, ...newInputs })) keeps old values for keys omitted by the backend response, which can accidentally resubmit outdated merchant/private key settings.
🔧 Suggested safe merge for Waffo Pancake keys
- setInputs((prev) => ({ ...prev, ...newInputs }));
+ setInputs((prev) => ({
+ ...prev,
+ WaffoPancakeEnabled: false,
+ WaffoPancakeSandbox: false,
+ WaffoPancakeMerchantID: '',
+ WaffoPancakePrivateKey: '',
+ WaffoPancakeStoreID: '',
+ WaffoPancakeProductID: '',
+ WaffoPancakeReturnURL: '',
+ WaffoPancakeCurrency: 'USD',
+ WaffoPancakeUnitPrice: 1.0,
+ WaffoPancakeMinTopUp: 1,
+ WaffoPancakeProdWebhookPublicKey: '',
+ WaffoPancakeTestWebhookPublicKey: '',
+ ...newInputs,
+ }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setInputs((prev) => ({ ...prev, ...newInputs })); | |
| setInputs((prev) => ({ | |
| ...prev, | |
| WaffoPancakeEnabled: false, | |
| WaffoPancakeSandbox: false, | |
| WaffoPancakeMerchantID: '', | |
| WaffoPancakePrivateKey: '', | |
| WaffoPancakeStoreID: '', | |
| WaffoPancakeProductID: '', | |
| WaffoPancakeReturnURL: '', | |
| WaffoPancakeCurrency: 'USD', | |
| WaffoPancakeUnitPrice: 1.0, | |
| WaffoPancakeMinTopUp: 1, | |
| WaffoPancakeProdWebhookPublicKey: '', | |
| WaffoPancakeTestWebhookPublicKey: '', | |
| ...newInputs, | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/settings/PaymentSetting.jsx` at line 140, The current
updater setInputs((prev) => ({ ...prev, ...newInputs })) preserves prior values
for keys that the backend omits, which can re-submit stale Waffo Pancake
credentials; change the update logic in the PaymentSetting component so that
when handling Waffo Pancake payloads you either replace the credentials object
entirely or explicitly clear sensitive fields omitted by newInputs (e.g.,
merchantKey, privateKey) instead of merging with prev—locate the call to
setInputs and make the merge conditional: if newInputs contains a Waffo Pancake
credentials object, use that object as the source of truth (or set omitted
sensitive keys to null/empty) rather than shallow-merging with prev.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
service/waffo_pancake.go (1)
433-441: Consider usingstrconvfor integer conversion.The
fmt.Sprintfandfmt.Sscanfwork butstrconv.FormatIntandstrconv.ParseIntare more idiomatic and slightly more efficient for simple integer conversions.♻️ Optional refactor using strconv
+import "strconv" + func strconvUnix(v int64) string { - return fmt.Sprintf("%d", v) + return strconv.FormatInt(v, 10) } func parseInt64(v string) (int64, error) { - var n int64 - _, err := fmt.Sscanf(v, "%d", &n) - return n, err + return strconv.ParseInt(v, 10, 64) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/waffo_pancake.go` around lines 433 - 441, Replace the fmt-based conversions in strconvUnix and parseInt64 with strconv functions: change strconvUnix to use strconv.FormatInt(v, 10) and change parseInt64 to use strconv.ParseInt(v, 10, 64) and return the parsed value and error; ensure parseInt64 still returns an int64 and error consistent with its signature and handle the returned (int64, error) from strconv.ParseInt appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@service/waffo_pancake.go`:
- Around line 433-441: Replace the fmt-based conversions in strconvUnix and
parseInt64 with strconv functions: change strconvUnix to use
strconv.FormatInt(v, 10) and change parseInt64 to use strconv.ParseInt(v, 10,
64) and return the parsed value and error; ensure parseInt64 still returns an
int64 and error consistent with its signature and handle the returned (int64,
error) from strconv.ParseInt appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6be8100-6886-4ea6-a8cc-1cbd4382ce92
📒 Files selected for processing (2)
service/waffo_pancake.goservice/waffo_pancake_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
service/waffo_pancake.go (2)
156-159:⚠️ Potential issue | 🔴 CriticalBound the checkout call with a client timeout.
http.DefaultClientis the zero-value client, so itsTimeoutis0and this payment path can block indefinitely on a slow or wedged upstream. Use a reusedhttp.Clientwith a bounded timeout here. (pkg.go.dev)⏱️ Proposed fix
+var waffoPancakeHTTPClient = &http.Client{ + Timeout: 30 * time.Second, +} + func CreateWaffoPancakeCheckoutSession(ctx context.Context, params *WaffoPancakeCreateSessionParams) (*WaffoPancakeCheckoutSession, error) { @@ - resp, err := http.DefaultClient.Do(req) + resp, err := waffoPancakeHTTPClient.Do(req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/waffo_pancake.go` around lines 156 - 159, The call uses http.DefaultClient.Do(req) which has no timeout and can block indefinitely; replace it with a reused http.Client that sets Timeout (e.g. create a package-level variable like waffoHTTPClient := &http.Client{Timeout: 10 * time.Second}) and call waffoHTTPClient.Do(req) instead of http.DefaultClient.Do(req); ensure you import time and reuse the client (do not create a new client per request) so resp/err handling remains the same.
227-234:⚠️ Potential issue | 🔴 CriticalRequire an explicit webhook environment; never fall back from prod to test.
VerifyConfiguredWaffoPancakeWebhookleavesopts.environmentempty, andverifyWaffoPancakeWebhookWithResolvedKeysthen tries the prod key and silently falls back to the test key. Because this verifier is used on the public webhook route, a test-signed payload can be accepted in production.🔒 Proposed fix
func VerifyConfiguredWaffoPancakeWebhook(payload string, signatureHeader string) (*waffoPancakeWebhookEvent, error) { + env := "prod" + if setting.WaffoPancakeSandbox { + env = "test" + } return verifyWaffoPancakeWebhook(payload, signatureHeader, waffoPancakeWebhookVerifyOptions{ + environment: env, toleranceMs: waffoPancakeDefaultTolerance.Milliseconds(), publicKeys: waffoPancakeWebhookPublicKeys{ Test: setting.WaffoPancakeTestWebhookPublicKey, Prod: setting.WaffoPancakeProdWebhookPublicKey, }, }) } @@ func verifyWaffoPancakeWebhookWithResolvedKeys(signatureInput string, signaturePart string, opts waffoPancakeWebhookVerifyOptions) error { - if opts.environment == "prod" || opts.environment == "test" { - return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys)) - } - - if err := verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("prod", opts.publicKeys)); err == nil { - return nil - } - return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("test", opts.publicKeys)) + if opts.environment != "prod" && opts.environment != "test" { + return fmt.Errorf("webhook environment not specified") + } + return verifyWaffoPancakeWebhookWithKey( + signatureInput, + signaturePart, + resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys), + ) }Also applies to: 398-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/waffo_pancake.go` around lines 227 - 234, VerifyConfiguredWaffoPancakeWebhook currently leaves opts.environment empty so verifyWaffoPancakeWebhook/verifyWaffoPancakeWebhookWithResolvedKeys will try prod then silently fall back to test; change VerifyConfiguredWaffoPancakeWebhook to set waffoPancakeWebhookVerifyOptions.environment explicitly (use the production environment for the public webhook) and ensure the verify path only checks the specified environment (do not allow implicit fallback to test). Apply the same fix to the other call site(s) that call verifyWaffoPancakeWebhook (the similar block around lines 398-406) so all configured verifiers pass an explicit environment and no implicit prod→test fallback occurs.
🧹 Nitpick comments (1)
service/waffo_pancake.go (1)
209-221: Expose the signed hash encoding inBodyHash.The canonical request uses a base64-encoded body digest, but
BodyHashis emitted as hex here. That makes the exported debug helper report a different representation than the one actually signed, which is easy to misread during signature debugging.🔍 Proposed fix
- BodyHash: fmt.Sprintf("%x", bodyHash[:]), + BodyHash: base64.StdEncoding.EncodeToString(bodyHash[:]),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/waffo_pancake.go` around lines 209 - 221, The BodyHash in the WaffoPancakeRequestDebugInfo is being formatted as hex but the canonical request and signWaffoPancakeRequest use a base64-encoded body digest; change the formatting where bodyHash is set (currently: bodyHash := sha256.Sum256(body) and BodyHash: fmt.Sprintf("%x", bodyHash[:])) to use base64 encoding (e.g., base64.StdEncoding.EncodeToString(bodyHash[:])) so the debug output matches the actual signed representation; ensure encoding/base64 is imported and update the assignment to BodyHash accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@service/waffo_pancake.go`:
- Around line 156-159: The call uses http.DefaultClient.Do(req) which has no
timeout and can block indefinitely; replace it with a reused http.Client that
sets Timeout (e.g. create a package-level variable like waffoHTTPClient :=
&http.Client{Timeout: 10 * time.Second}) and call waffoHTTPClient.Do(req)
instead of http.DefaultClient.Do(req); ensure you import time and reuse the
client (do not create a new client per request) so resp/err handling remains the
same.
- Around line 227-234: VerifyConfiguredWaffoPancakeWebhook currently leaves
opts.environment empty so
verifyWaffoPancakeWebhook/verifyWaffoPancakeWebhookWithResolvedKeys will try
prod then silently fall back to test; change VerifyConfiguredWaffoPancakeWebhook
to set waffoPancakeWebhookVerifyOptions.environment explicitly (use the
production environment for the public webhook) and ensure the verify path only
checks the specified environment (do not allow implicit fallback to test). Apply
the same fix to the other call site(s) that call verifyWaffoPancakeWebhook (the
similar block around lines 398-406) so all configured verifiers pass an explicit
environment and no implicit prod→test fallback occurs.
---
Nitpick comments:
In `@service/waffo_pancake.go`:
- Around line 209-221: The BodyHash in the WaffoPancakeRequestDebugInfo is being
formatted as hex but the canonical request and signWaffoPancakeRequest use a
base64-encoded body digest; change the formatting where bodyHash is set
(currently: bodyHash := sha256.Sum256(body) and BodyHash: fmt.Sprintf("%x",
bodyHash[:])) to use base64 encoding (e.g.,
base64.StdEncoding.EncodeToString(bodyHash[:])) so the debug output matches the
actual signed representation; ensure encoding/base64 is imported and update the
assignment to BodyHash accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4d68beb-6388-4ddd-9180-451e2e8775ab
📒 Files selected for processing (3)
controller/topup_waffo_pancake.goservice/waffo_pancake.goservice/waffo_pancake_signature_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controller/topup_waffo_pancake.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
service/waffo_pancake.go (3)
398-407:⚠️ Potential issue | 🔴 CriticalFallback to both keys weakens security guarantees.
When
opts.environmentis empty (which is currently always the case), the code tries the production key first, then falls back to the test key. This means a webhook signed with the test key will be accepted in production.Even after fixing
VerifyConfiguredWaffoPancakeWebhookto pass the environment, this fallback logic should be removed to enforce strict environment separation.Proposed fix: Remove fallback, require explicit environment
func verifyWaffoPancakeWebhookWithResolvedKeys(signatureInput string, signaturePart string, opts waffoPancakeWebhookVerifyOptions) error { - if opts.environment == "prod" || opts.environment == "test" { - return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys)) + if opts.environment == "" { + return fmt.Errorf("webhook environment not specified") } - - if err := verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("prod", opts.publicKeys)); err == nil { - return nil - } - return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("test", opts.publicKeys)) + return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/waffo_pancake.go` around lines 398 - 407, The current verifyWaffoPancakeWebhookWithResolvedKeys function falls back from prod to test keys when opts.environment is empty, allowing test-signed webhooks in prod; change it to require an explicit environment and remove the fallback logic by validating opts.environment (only "prod" or "test") and calling verifyWaffoPancakeWebhookWithKey with the corresponding resolved key; if opts.environment is missing or invalid, return a clear error instead of trying both keys—update any callers such as VerifyConfiguredWaffoPancakeWebhook to pass the correct environment.
227-235:⚠️ Potential issue | 🔴 CriticalCritical: Sandbox mode setting is ignored in webhook verification.
VerifyConfiguredWaffoPancakeWebhookdoes not passsetting.WaffoPancakeSandboxto determine the environment. Theopts.environmentfield is left empty, causingverifyWaffoPancakeWebhookWithResolvedKeys(lines 398-407) to fall back to trying both production and test keys.This allows an attacker to forge webhooks using the hardcoded test public key (lines 26-34) even when the system is configured for production mode, potentially crediting accounts without actual payment.
Proposed fix: Pass sandbox setting to determine environment
func VerifyConfiguredWaffoPancakeWebhook(payload string, signatureHeader string) (*waffoPancakeWebhookEvent, error) { + env := "prod" + if setting.WaffoPancakeSandbox { + env = "test" + } return verifyWaffoPancakeWebhook(payload, signatureHeader, waffoPancakeWebhookVerifyOptions{ + environment: env, toleranceMs: waffoPancakeDefaultTolerance.Milliseconds(), publicKeys: waffoPancakeWebhookPublicKeys{ Test: setting.WaffoPancakeTestWebhookPublicKey, Prod: setting.WaffoPancakeProdWebhookPublicKey, }, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/waffo_pancake.go` around lines 227 - 235, VerifyConfiguredWaffoPancakeWebhook currently omits the environment flag so verification tries both test and prod keys; update the call that constructs waffoPancakeWebhookVerifyOptions in VerifyConfiguredWaffoPancakeWebhook to set the opts.environment using the configured setting (setting.WaffoPancakeSandbox) so verifyWaffoPancakeWebhook and downstream verifyWaffoPancakeWebhookWithResolvedKeys use the correct environment and do not accept the hardcoded test key in prod.
156-160:⚠️ Potential issue | 🟠 MajorHTTP client without timeout may hang indefinitely.
http.DefaultClienthas no timeout configured. If the Waffo Pancake API is slow or unresponsive, the goroutine will block indefinitely, potentially exhausting resources.Proposed fix: Use a client with timeout
+var waffoPancakeHTTPClient = &http.Client{ + Timeout: 30 * time.Second, +} + func CreateWaffoPancakeCheckoutSession(ctx context.Context, params *WaffoPancakeCreateSessionParams) (*WaffoPancakeCheckoutSession, error) { // ... existing code ... - resp, err := http.DefaultClient.Do(req) + resp, err := waffoPancakeHTTPClient.Do(req) if err != nil { return nil, fmt.Errorf("request Waffo Pancake checkout session: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/waffo_pancake.go` around lines 156 - 160, The HTTP call uses http.DefaultClient.Do(req) which has no timeout and can hang indefinitely; replace it with a dedicated http.Client with a sensible Timeout (e.g., time.Second*10) and use that client's Do to execute req (or attach a context with deadline to req before calling client.Do) so the request will fail fast on slow/unresponsive Waffo Pancake API; update the code around resp, err := http.DefaultClient.Do(req) and ensure resp.Body.Close() remains deferred.controller/topup_waffo_pancake.go (1)
251-252:⚠️ Potential issue | 🟠 MajorIn-process locking is insufficient for distributed deployments.
The
LockOrder/UnlockOrderimplementation uses a process-localsync.Map(per context snippet 2). If multiple instances are deployed, concurrent webhook requests for the sametradeNoacross instances can race.However, reviewing
model.RechargeWaffoPancake(context snippet 1), the database-level protection is robust:
FOR UPDATErow lock prevents concurrent transactions- Idempotency check returns early if
status == "success"- Status validation ensures only "pending" orders are processed
The in-memory lock provides additional single-instance optimization but isn't the primary concurrency control. The database transaction is the true guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/topup_waffo_pancake.go` around lines 251 - 252, The in-process LockOrder/UnlockOrder calls around the webhook handling are inadequate for distributed deployments and should not be relied on for correctness; remove the LockOrder(tradeNo) and defer UnlockOrder(tradeNo) calls from the handler and rely on the database-level concurrency controls already implemented in RechargeWaffoPancake (the FOR UPDATE row lock, status == "success" idempotency check, and "pending" status validation) to prevent races, or alternatively replace the local lock usage with a distributed lock if you intend to keep optimistic single-instance short-circuiting; update any comments to document that LockOrder/UnlockOrder are only a single-process optimization and not a substitute for DB transactional protection.
🧹 Nitpick comments (2)
controller/topup_waffo_pancake.go (2)
147-206: Consider extracting session params to avoid duplication.The
WaffoPancakeCreateSessionParamsstruct is built identically twice—once for the actual call (lines 148-166) and again for debug info on error (lines 168-186). Extract to a variable to reduce duplication and ensure consistency.Suggested refactor
+ sessionParams := &service.WaffoPancakeCreateSessionParams{ + StoreID: setting.WaffoPancakeStoreID, + ProductID: setting.WaffoPancakeProductID, + ProductType: "onetime", + Currency: strings.ToUpper(strings.TrimSpace(setting.WaffoPancakeCurrency)), + PriceSnapshot: &service.WaffoPancakePriceSnapshot{ + Amount: fmt.Sprintf("%d", waffoPancakeMoneyToMinorUnits(payMoney)), + TaxIncluded: false, + TaxCategory: "saas", + }, + BuyerEmail: getWaffoPancakeBuyerEmail(user), + SuccessURL: getWaffoPancakeReturnURL(), + ExpiresInSeconds: &expiresInSeconds, + Metadata: map[string]string{ + "internalTradeNo": tradeNo, + "userId": fmt.Sprintf("%d", id), + "topupAmount": fmt.Sprintf("%d", req.Amount), + }, + } + expiresInSeconds := 45 * 60 - session, err := service.CreateWaffoPancakeCheckoutSession(c.Request.Context(), &service.WaffoPancakeCreateSessionParams{ - StoreID: setting.WaffoPancakeStoreID, - ... - }) + session, err := service.CreateWaffoPancakeCheckoutSession(c.Request.Context(), sessionParams) if err != nil { - debugInfo, debugErr := service.BuildWaffoPancakeCheckoutRequestDebugInfo(&service.WaffoPancakeCreateSessionParams{ - ... - }) + debugInfo, debugErr := service.BuildWaffoPancakeCheckoutRequestDebugInfo(sessionParams)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/topup_waffo_pancake.go` around lines 147 - 206, The WaffoPancakeCreateSessionParams struct is duplicated; create a single variable (e.g. params := &service.WaffoPancakeCreateSessionParams{...}) populated with expiresInSeconds, Currency, PriceSnapshot (using waffoPancakeMoneyToMinorUnits(payMoney)), BuyerEmail (getWaffoPancakeBuyerEmail(user)), SuccessURL (getWaffoPancakeReturnURL()), Metadata (internalTradeNo/tradeNo, userId/id, topupAmount/req.Amount), etc., then pass that params to service.CreateWaffoPancakeCheckoutSession and to service.BuildWaffoPancakeCheckoutRequestDebugInfo to remove duplication and guarantee consistency.
244-249: Missing trade number logs 200 OK without error signal.When
tradeNois empty, the handler returns 200 OK. This prevents webhook retries but masks potential configuration issues (e.g., metadata not being passed correctly by Waffo Pancake).Consider whether this should log at a higher severity or return a different status to signal an issue to operators while still acknowledging the webhook.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/topup_waffo_pancake.go` around lines 244 - 249, When ExtractWaffoPancakeTradeNo(event) returns an empty tradeNo the handler currently logs with log.Printf and responds 200 which hides problems; update the branch handling tradeNo == "" to emit an operator-visible error (e.g., use log.Printf or the app's error logger with an "ERROR" or "WARN" prefix and include event.ID and context) and return a non-2xx status (e.g., c.String(422, "missing trade_no") or c.String(400, "missing trade_no")) so the webhook provider can retry and operators are alerted; modify the code around tradeNo, service.ExtractWaffoPancakeTradeNo and the early return to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@controller/topup_waffo_pancake.go`:
- Around line 251-252: The in-process LockOrder/UnlockOrder calls around the
webhook handling are inadequate for distributed deployments and should not be
relied on for correctness; remove the LockOrder(tradeNo) and defer
UnlockOrder(tradeNo) calls from the handler and rely on the database-level
concurrency controls already implemented in RechargeWaffoPancake (the FOR UPDATE
row lock, status == "success" idempotency check, and "pending" status
validation) to prevent races, or alternatively replace the local lock usage with
a distributed lock if you intend to keep optimistic single-instance
short-circuiting; update any comments to document that LockOrder/UnlockOrder are
only a single-process optimization and not a substitute for DB transactional
protection.
In `@service/waffo_pancake.go`:
- Around line 398-407: The current verifyWaffoPancakeWebhookWithResolvedKeys
function falls back from prod to test keys when opts.environment is empty,
allowing test-signed webhooks in prod; change it to require an explicit
environment and remove the fallback logic by validating opts.environment (only
"prod" or "test") and calling verifyWaffoPancakeWebhookWithKey with the
corresponding resolved key; if opts.environment is missing or invalid, return a
clear error instead of trying both keys—update any callers such as
VerifyConfiguredWaffoPancakeWebhook to pass the correct environment.
- Around line 227-235: VerifyConfiguredWaffoPancakeWebhook currently omits the
environment flag so verification tries both test and prod keys; update the call
that constructs waffoPancakeWebhookVerifyOptions in
VerifyConfiguredWaffoPancakeWebhook to set the opts.environment using the
configured setting (setting.WaffoPancakeSandbox) so verifyWaffoPancakeWebhook
and downstream verifyWaffoPancakeWebhookWithResolvedKeys use the correct
environment and do not accept the hardcoded test key in prod.
- Around line 156-160: The HTTP call uses http.DefaultClient.Do(req) which has
no timeout and can hang indefinitely; replace it with a dedicated http.Client
with a sensible Timeout (e.g., time.Second*10) and use that client's Do to
execute req (or attach a context with deadline to req before calling client.Do)
so the request will fail fast on slow/unresponsive Waffo Pancake API; update the
code around resp, err := http.DefaultClient.Do(req) and ensure resp.Body.Close()
remains deferred.
---
Nitpick comments:
In `@controller/topup_waffo_pancake.go`:
- Around line 147-206: The WaffoPancakeCreateSessionParams struct is duplicated;
create a single variable (e.g. params :=
&service.WaffoPancakeCreateSessionParams{...}) populated with expiresInSeconds,
Currency, PriceSnapshot (using waffoPancakeMoneyToMinorUnits(payMoney)),
BuyerEmail (getWaffoPancakeBuyerEmail(user)), SuccessURL
(getWaffoPancakeReturnURL()), Metadata (internalTradeNo/tradeNo, userId/id,
topupAmount/req.Amount), etc., then pass that params to
service.CreateWaffoPancakeCheckoutSession and to
service.BuildWaffoPancakeCheckoutRequestDebugInfo to remove duplication and
guarantee consistency.
- Around line 244-249: When ExtractWaffoPancakeTradeNo(event) returns an empty
tradeNo the handler currently logs with log.Printf and responds 200 which hides
problems; update the branch handling tradeNo == "" to emit an operator-visible
error (e.g., use log.Printf or the app's error logger with an "ERROR" or "WARN"
prefix and include event.ID and context) and return a non-2xx status (e.g.,
c.String(422, "missing trade_no") or c.String(400, "missing trade_no")) so the
webhook provider can retry and operators are alerted; modify the code around
tradeNo, service.ExtractWaffoPancakeTradeNo and the early return to implement
this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0df7d44-383c-4c03-bda2-727f9a251e72
📒 Files selected for processing (3)
controller/topup_waffo_pancake.goservice/waffo_pancake.goservice/waffo_pancake_signature_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- service/waffo_pancake_signature_test.go
seefs001
force-pushed
the
feature/waffo-pay
branch
from
5f5f370 to
30ac0d7
Compare
# Conflicts: # web/src/i18n/locales/en.json # web/src/i18n/locales/fr.json # web/src/i18n/locales/ja.json # web/src/i18n/locales/ru.json # web/src/i18n/locales/vi.json # web/src/i18n/locales/zh-TW.json
xyfacai
pushed a commit
to xyfacai/new-api
that referenced
this pull request
Apr 20, 2026
rafactor: payment
This was referenced May 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 沟通提示 / Pre-submission
📝 变更描述 / Description
新增Waffo Pancake集成
优化支付配置项
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
📸 运行证明 / Proof of Work
TODO
Summary by CodeRabbit
New Features
Tests