feat: generate paymentId on server #558
Conversation
WalkthroughReplaces local synchronous paymentId generation with an async createPayment(...) call. PayButton and WidgetContainer now obtain paymentId asynchronously when enabled. Conversion state (convertedAmount, convertedCurrencyObj) is added and threaded through PaymentDialog → WidgetContainer → Widget for shared conversion state. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant PayButton as PayButton
participant PD as PaymentDialog
participant WC as WidgetContainer
participant Widget as Widget
participant API as api-client.createPayment
participant Server as Payments API
rect rgb(245,250,255)
note right of PayButton: User clicks PayButton
User->>PayButton: click
PayButton->>PayButton: validate flags (disablePaymentId, to, fiat)
alt needs paymentId
PayButton->>API: createPayment(amountToUse, to, apiBaseUrl)
API->>Server: POST /api/payments/paymentId { amount, address }
Server-->>API: { paymentId }
API-->>PayButton: paymentId
PayButton->>PayButton: set paymentId
end
PayButton->>PD: open(paymentId, convertedAmount, convertedCurrencyObj)
end
rect rgb(250,245,255)
note right of PD: Dialog forwards conversion props to container
PD->>WC: render(convertedCurrencyObj, setConvertedCurrencyObj)
WC->>Widget: render(convertedCurrencyObj, setConvertedCurrencyObj)
Widget->>Widget: prefer provided convertedCurrencyObj or compute & set it
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential attention points:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
lissavxo
force-pushed
the
feat/genrate-paymentId-on-server
branch
from
b12bbdc to
feba15c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
react/lib/components/PayButton/PayButton.tsx (2)
148-158: Ensure a paymentId exists before opening the dialog to avoid race conditionsCurrently, the dialog (and websockets) can start with
paymentIdstill undefined, breaking success detection whendisablePaymentIdis false. Generate it on click if needed, before opening.-const handleButtonClick = useCallback(async (): Promise<void> => { - if (onOpen !== undefined) { - if (isFiat(currency)) { - void waitPrice(() => { onOpen(cryptoAmountRef.current, to, paymentId) }) - } else { - onOpen(amount, to, paymentId) - } - } - setDialogOpen(true); -}, [cryptoAmount, to, paymentId, price]) +const handleButtonClick = useCallback(async (): Promise<void> => { + let pid = paymentId; + if (!disablePaymentId && to && !pid) { + try { + pid = await createPayment(amount, to, apiBaseUrl); + setPaymentId(pid); + } catch (e) { + console.error('Error creating payment ID on click:', e); + } + } + if (onOpen !== undefined) { + if (isFiat(currency)) { + void waitPrice(() => { onOpen(cryptoAmountRef.current, to, pid) }) + } else { + onOpen(amount, to, pid) + } + } + setDialogOpen(true); +}, [cryptoAmount, to, paymentId, price, disablePaymentId, amount, apiBaseUrl])
195-244: WebSocket setup should wait for paymentId (when required) and react to updatesThe socket effect doesn’t depend on
paymentId. If it initializes beforepaymentIdresolves,expectedPaymentIdis undefined and won’t update, breaking success matching.useEffect(() => { if (dialogOpen === false) { return } + // If paymentId is required, wait until it’s available + if (!disablePaymentId && !paymentId) { + return; + } (async () => { if (txsSocket === undefined) { const expectedAmount = currencyObj ? currencyObj?.float : undefined await setupChronikWebSocket({ address: to, txsSocket, apiBaseUrl, wsBaseUrl, setTxsSocket, setNewTxs, setDialogOpen, checkSuccessInfo: { currency, price, randomSatoshis: randomSatoshis ?? false, disablePaymentId, - expectedAmount, - expectedOpReturn: opReturn, - expectedPaymentId: paymentId, + expectedAmount, + expectedOpReturn: opReturn, + expectedPaymentId: paymentId, currencyObj, } }) } if (altpaymentSocket === undefined && useAltpayment) { await setupAltpaymentSocket({ addressType, altpaymentSocket, wsBaseUrl, setAltpaymentSocket, setCoins, setCoinPair, setLoadingPair, setAltpaymentShift, setLoadingShift, setAltpaymentError, }) } })() return () => { if (altpaymentSocket !== undefined) { altpaymentSocket.disconnect(); setAltpaymentSocket(undefined); } } -}, [dialogOpen, useAltpayment]); +}, [dialogOpen, useAltpayment, disablePaymentId, paymentId]);
🧹 Nitpick comments (4)
react/lib/util/api-client.ts (1)
106-112: Optional: exposecreatePaymenton the default export for consistencyYou’re already using the named export correctly. If the rest of the module’s API is commonly consumed via the default export, consider adding
createPaymentthere for consistency.export default { getAddressDetails, getTransactionDetails, getBchFiatPrice, getXecFiatPrice, getAddressBalance, + // createPayment, // uncomment if you prefer exposing it via default export too };react/lib/components/Widget/WidgetContainer.tsx (1)
142-158: Avoid calling API for invalid addresses and guard against stale updates
- Only call
createPaymentiftois a valid BCH/XEC address.- Guard against setting state after unmount and stale responses (latest-only).
- Confirm that regenerating on every
amountchange is desired (can spam the API while typing).useEffect(() => { - const initializePaymentId = async () => { - if ((paymentId === undefined || paymentId === '') && !disablePaymentId) { - if (to) { - try { - const responsePaymentId = await createPayment(amount, to, apiBaseUrl); - setThisPaymentId(responsePaymentId); - } catch (error) { - console.error('Error creating payment ID:', error); - } - } - } else { - setThisPaymentId(paymentId); - } - }; - - initializePaymentId(); + let active = true; + const initializePaymentId = async () => { + if ((paymentId === undefined || paymentId === '') && !disablePaymentId) { + if (to /* optionally validate here if available in scope */) { + try { + const responsePaymentId = await createPayment(amount, to, apiBaseUrl); + if (active) setThisPaymentId(responsePaymentId); + } catch (error) { + console.error('Error creating payment ID:', error); + } + } + } else { + setThisPaymentId(paymentId); + } + }; + + initializePaymentId(); + return () => { active = false; }; }, [paymentId, disablePaymentId, amount, to, apiBaseUrl]);If
isValidCashAddress/isValidXecAddressare available in this module, add a validation guard around the API call. Based on learningsreact/lib/components/PayButton/PayButton.tsx (2)
117-130: Validate address before requesting and guard setState-after-unmountAdd a validity check for
toand an “active” guard to prevent stale updates.useEffect(() => { - const initializePaymentId = async () => { - if (!disablePaymentId && to) { - try { - const responsePaymentId = await createPayment(amount, to, apiBaseUrl); - setPaymentId(responsePaymentId); - } catch (error) { - console.error('Error creating payment ID:', error); - } - } - }; - - initializePaymentId(); + let active = true; + const initializePaymentId = async () => { + if (!disablePaymentId && to /* && (isValidCashAddress(to) || isValidXecAddress(to)) */) { + try { + const responsePaymentId = await createPayment(amount, to, apiBaseUrl); + if (active) setPaymentId(responsePaymentId); + } catch (error) { + console.error('Error creating payment ID:', error); + } + } + }; + + initializePaymentId(); + return () => { active = false; }; }, [disablePaymentId, amount, to, apiBaseUrl]);
112-116: Optional: consider a shared hook for paymentIdBoth PayButton and WidgetContainer duplicate the “create on load” logic. A
usePaymentId({ to, amount, apiBaseUrl, disabled })hook would reduce drift and centralize race guards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
react/lib/components/PayButton/PayButton.tsx(2 hunks)react/lib/components/Widget/WidgetContainer.tsx(2 hunks)react/lib/util/api-client.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
react/lib/components/PayButton/PayButton.tsx (2)
react/lib/util/address.ts (1)
getCurrencyTypeFromAddress(26-34)react/lib/util/api-client.ts (1)
createPayment(92-103)
react/lib/components/Widget/WidgetContainer.tsx (2)
react/.storybook/default-args.ts (1)
to(1-1)react/lib/util/api-client.ts (1)
createPayment(92-103)
🔇 Additional comments (1)
react/lib/components/PayButton/PayButton.tsx (1)
117-130: Confirm API endpoint and payload shape
In react/lib/util/api-client.ts, createPayment posts toPOST ${rootUrl}/api/payments/paymentIdwith payload{ amount, address }. Ensure your backend route and handler accept this exact path and both fields.
| export const createPayment = async ( | ||
| amount: string | number | undefined, | ||
| address: string, | ||
| rootUrl = config.apiBaseUrl, | ||
| ): Promise<string> => { | ||
| const { data } = await axios.post( | ||
| `${rootUrl}/api/payments/paymentId`, | ||
| { amount, address } | ||
| ); | ||
| return data.paymentId; | ||
|
|
||
| }; | ||
|
|
There was a problem hiding this comment.
Validate API response, omit undefined fields, and add a timeout
Guard against missing paymentId, avoid sending amount: undefined, and add a client timeout to prevent hanging requests. Also type the response.
+type CreatePaymentResponse = { paymentId: string };
+
export const createPayment = async (
amount: string | number | undefined,
address: string,
rootUrl = config.apiBaseUrl,
): Promise<string> => {
- const { data } = await axios.post(
- `${rootUrl}/api/payments/paymentId`,
- { amount, address }
- );
- return data.paymentId;
+ const payload: Record<string, unknown> = { address };
+ if (typeof amount !== 'undefined') payload.amount = amount;
+
+ const { data } = await axios.post<CreatePaymentResponse>(
+ `${rootUrl}/api/payments/paymentId`,
+ payload,
+ { timeout: 10000 }
+ );
+ if (!data?.paymentId) {
+ throw new Error('Invalid response from createPayment: missing paymentId');
+ }
+ return data.paymentId;
};📝 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.
| export const createPayment = async ( | |
| amount: string | number | undefined, | |
| address: string, | |
| rootUrl = config.apiBaseUrl, | |
| ): Promise<string> => { | |
| const { data } = await axios.post( | |
| `${rootUrl}/api/payments/paymentId`, | |
| { amount, address } | |
| ); | |
| return data.paymentId; | |
| }; | |
| // Add a typed response for clarity and safety | |
| type CreatePaymentResponse = { paymentId: string }; | |
| export const createPayment = async ( | |
| amount: string | number | undefined, | |
| address: string, | |
| rootUrl = config.apiBaseUrl, | |
| ): Promise<string> => { | |
| // Build payload without undefined fields | |
| const payload: Record<string, unknown> = { address }; | |
| if (typeof amount !== 'undefined') { | |
| payload.amount = amount; | |
| } | |
| // Send typed request with timeout | |
| const { data } = await axios.post<CreatePaymentResponse>( | |
| `${rootUrl}/api/payments/paymentId`, | |
| payload, | |
| { timeout: 10000 } | |
| ); | |
| // Validate response | |
| if (!data?.paymentId) { | |
| throw new Error('Invalid response from createPayment: missing paymentId'); | |
| } | |
| return data.paymentId; | |
| }; |
🤖 Prompt for AI Agents
In react/lib/util/api-client.ts around lines 92 to 104, the createPayment
function should (1) avoid sending amount when it's undefined by constructing the
request body conditionally so amount is omitted if undefined, (2) add an axios
request timeout (e.g. 5–10s) to the post call to avoid hanging, (3) type the
expected response (e.g. an interface with paymentId?: string) and use it for
axios.post's generic, and (4) validate the response before returning by checking
data.paymentId exists and throwing a descriptive error if missing so callers
never receive undefined.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
react/lib/components/PaymentDialog/PaymentDialog.tsx (1)
80-80: Default normalization LGTM; consider prop typing alignmentLooks good. Since isPropsTrue accepts string | boolean, consider widening PaymentDialogProps.disablePaymentId to string | boolean for consistency with usage across components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
react/lib/components/PayButton/PayButton.tsx(3 hunks)react/lib/components/PaymentDialog/PaymentDialog.tsx(1 hunks)react/lib/components/Widget/WidgetContainer.tsx(3 hunks)react/lib/util/api-client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- react/lib/util/api-client.ts
🧰 Additional context used
🧬 Code graph analysis (3)
react/lib/components/PayButton/PayButton.tsx (3)
react/lib/util/format.ts (2)
isPropsTrue(66-75)amount(5-12)react/lib/util/address.ts (1)
getCurrencyTypeFromAddress(26-34)react/lib/util/api-client.ts (1)
createPayment(92-107)
react/lib/components/PaymentDialog/PaymentDialog.tsx (1)
react/lib/util/format.ts (1)
isPropsTrue(66-75)
react/lib/components/Widget/WidgetContainer.tsx (2)
react/lib/util/format.ts (2)
isPropsTrue(66-75)amount(5-12)react/lib/util/api-client.ts (1)
createPayment(92-107)
chedieck
force-pushed
the
feat/genrate-paymentId-on-server
branch
from
a0ada03 to
fb6707d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
react/lib/components/Widget/WidgetContainer.tsx (1)
143-172: Payment ID init still deadlocks and misses finalization on error; revert to boolean guard with try/finallyInitialize as boolean false, guard on true, handle disabled/provided IDs early, and always reset in finally. This also allows re-run on deps change. Regression from earlier review.
- const [fetchingPaymentId, setFetchingPaymentId] = useState<boolean | undefined>(); + const [fetchingPaymentId, setFetchingPaymentId] = useState<boolean>(false); ... - useEffect(() => { - if ( - fetchingPaymentId !== undefined || - thisPaymentId !== undefined - ) { - return - } - setFetchingPaymentId(true) - const initializePaymentId = async () => { - if (paymentId === undefined && !disablePaymentId) { - if (to) { - try { - const responsePaymentId = await createPayment(amount, to, apiBaseUrl); - setThisPaymentId(responsePaymentId); - setFetchingPaymentId(false); - } catch (error) { - console.error('Error creating payment ID:', error); - } - } - } else { - setThisPaymentId(paymentId); - setFetchingPaymentId(false); - } - }; - - initializePaymentId(); - }, [paymentId, disablePaymentId, amount, to, apiBaseUrl]); + useEffect(() => { + if (fetchingPaymentId) return; + + // Provided or disabled: just propagate, no fetch. + if (disablePaymentId || (paymentId !== undefined && paymentId !== '')) { + setThisPaymentId(paymentId); + return; + } + // Cannot create without address + if (!to) return; + + setFetchingPaymentId(true); + const initializePaymentId = async () => { + try { + const responsePaymentId = await createPayment(amount, to, apiBaseUrl); + setThisPaymentId(responsePaymentId); + } catch (error) { + console.error('Error creating payment ID:', error); + } finally { + setFetchingPaymentId(false); + } + }; + initializePaymentId(); + }, [paymentId, disablePaymentId, amount, to, apiBaseUrl]);
🧹 Nitpick comments (3)
react/lib/components/Widget/WidgetContainer.tsx (2)
247-261: Trim unused dependency to avoid needless re-renders
cryptoAmountis not used inhandlePayment; remove it from deps.- [ + [ onSuccess, onTransaction, enqueueSnackbar, hideToasts, sound, txSound, - cryptoAmount, successText, to, thisPaymentId, altpaymentShift, thisPrice, currencyObj, randomSatoshis ],
297-300: Use forEach for side effects instead of mapAvoid creating an unused array.
- thisNewTxs?.map(tx => { - handleNewTransaction(tx); - }); + thisNewTxs?.forEach(handleNewTransaction);react/lib/components/PayButton/PayButton.tsx (1)
299-331: Converted amount can go stale; recompute on changes (remove one-time guards)The
if(!convertedCurrencyObj)/!convertedAmountguards block updates when amount/price/randomSatoshis change.useEffect(() => { - if (currencyObj and isFiat(currency) and price) { - if(!convertedCurrencyObj) { + if (currencyObj && isFiat(currency) && price) { const addressType: Currency = getCurrencyTypeFromAddress(to); const convertedObj = getCurrencyObject( currencyObj.float / price, addressType, randomSatoshis, ); setCryptoAmount(convertedObj.string); setConvertedAmount(convertedObj.float); setConvertedCurrencyObj(convertedObj); - } } else if (!isFiat(currency) && randomSatoshis && !convertedAmount){ const convertedObj = getCurrencyObject( amount as number, addressType, randomSatoshis, - ); setCryptoAmount(convertedObj.string); + ); + setCryptoAmount(convertedObj.string); setConvertedAmount(convertedObj.float); setConvertedCurrencyObj(convertedObj); } else if (!isFiat(currency) && !randomSatoshis) { setCryptoAmount(amount?.toString()); } }, [price, currencyObj, amount, currency, randomSatoshis, to]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react/lib/components/PayButton/PayButton.tsx(8 hunks)react/lib/components/Widget/WidgetContainer.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
react/lib/components/Widget/WidgetContainer.tsx (3)
react/lib/util/types.ts (1)
CurrencyObject(4-8)react/lib/util/format.ts (2)
isPropsTrue(66-75)amount(5-12)react/lib/util/api-client.ts (1)
createPayment(92-107)
react/lib/components/PayButton/PayButton.tsx (6)
react/lib/util/format.ts (2)
isPropsTrue(66-75)amount(5-12)react/lib/util/address.ts (1)
getCurrencyTypeFromAddress(26-34)react/lib/util/currency.ts (1)
isFiat(4-6)react/lib/util/api-client.ts (1)
createPayment(92-107)react/lib/util/socket.ts (2)
setupChronikWebSocket(141-153)setupAltpaymentSocket(95-114)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)
🔇 Additional comments (1)
react/lib/components/PayButton/PayButton.tsx (1)
139-189: I need to examine the WidgetContainer call site more carefully to understand the context.Verify currency conversion consistency for createPayment at WidgetContainer.tsx:158.
The review comment requests verification that all
createPaymentinvocations use crypto units. The grep results reveal two call sites with different patterns:
- PayButton.tsx:153 – uses conditional logic (
amountToUse) to select eitherconvertedAmount(when fiat or randomSatoshis) oramount- WidgetContainer.tsx:158 – passes
amountdirectly without visible conversion logicThe PayButton implementation shows explicit awareness of unit conversion (fiat vs. crypto), but WidgetContainer's approach lacks visible equivalent handling. Without inspecting WidgetContainer's broader context—including how
amountis sourced, whether currency detection occurs upstream, and whether the calling function handles conversions—it's not possible to confirm whether both call sites properly normalize to crypto units before invokingcreatePayment.Manually review WidgetContainer.tsx around line 156–160 and trace the
amountparameter origin to ensure consistent unit handling across both call sites.
| const [paymentId, setPaymentId] = useState<string | undefined>(undefined); | ||
| const [addressType, setAddressType] = useState<CryptoCurrency>( | ||
| getCurrencyTypeFromAddress(to), | ||
| ); | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Reset paymentId when address/currency/randomization inputs change to avoid stale IDs
Otherwise an ID tied to a previous address or unit policy may be reused.
const [paymentId, setPaymentId] = useState<string | undefined>(undefined);
+ useEffect(() => {
+ setPaymentId(undefined);
+ }, [to, currency, randomSatoshis]);If the server binds paymentId to the original amount as well, also include amount in the dependency list. Please confirm backend semantics.
---
🏁 Script executed:
```shell
#!/bin/bash
# Locate PayButton.tsx and examine its content
find . -name "PayButton.tsx" -type f 2>/dev/null | head -20
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for paymentId usage patterns in the codebase
rg -n "paymentId" --type=ts --type=tsx -C 5 | head -100Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for useEffect hooks and state management in PayButton area
fd "PayButton" -type f | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for backend API calls related to payment
rg -n "payment" --type=ts --type=tsx -i -A 2 -B 2 | grep -i "api\|fetch\|axios\|request" | head -50Length of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Read the PayButton.tsx file directly
cat ./react/lib/components/PayButton/PayButton.tsx
Length of output: 12741
🏁 Script executed:
#!/bin/bash
# Search for paymentId usage (corrected syntax)
rg -n "paymentId" -C 5Length of output: 90419
🏁 Script executed:
#!/bin/bash
# Search for amount usage in payment context
rg -n "amount" -C 3 | head -80Length of output: 4137
🏁 Script executed:
#!/bin/bash
# Look for useEffect hooks in the codebase
rg -n "useEffect" ./react/lib/components/PayButton/ -C 3Length of output: 4531
🏁 Script executed:
#!/bin/bash
# Search for API calls or backend requests
rg -n "fetch\|axios\|api" -i ./react/lib/components/PayButton/ -C 3Length of output: 0
Reset paymentId when address, currency, amount, or randomization inputs change to prevent reuse of stale IDs
Without resetting paymentId when these inputs change, the component can reuse IDs tied to previous addresses, amounts, or unit policies, causing validation mismatches.
const [paymentId, setPaymentId] = useState<string | undefined>(undefined);
+ useEffect(() => {
+ setPaymentId(undefined);
+ }, [to, currency, amount, randomSatoshis]);The backend's createPayment API endpoint receives both amount and address and generates an ID bound to those values (per react/lib/util/api-client.ts lines 98–99). Therefore, amount must be included in the dependency list alongside to, currency, and randomSatoshis.
📝 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.
| const [paymentId, setPaymentId] = useState<string | undefined>(undefined); | |
| const [addressType, setAddressType] = useState<CryptoCurrency>( | |
| getCurrencyTypeFromAddress(to), | |
| ); | |
| const [paymentId, setPaymentId] = useState<string | undefined>(undefined); | |
| useEffect(() => { | |
| setPaymentId(undefined); | |
| }, [to, currency, amount, randomSatoshis]); | |
| const [addressType, setAddressType] = useState<CryptoCurrency>( | |
| getCurrencyTypeFromAddress(to), | |
| ); |
🤖 Prompt for AI Agents
In react/lib/components/PayButton/PayButton.tsx around lines 115 to 121, the
component fails to reset paymentId when key inputs change which can cause reuse
of stale IDs; update the effect/logic that computes addressType and paymentId so
that it includes amount (and already-present dependencies to, currency,
randomSatoshis) in its dependency list and call setPaymentId(undefined) whenever
any of those change, ensuring a new ID is requested for the new
address/amount/unit policy.
| const getPaymentId = useCallback(async ( | ||
| currency: Currency, | ||
| amount: number, | ||
| convertedAmount: number | undefined, | ||
| to: string | undefined, | ||
| ): Promise<string | undefined> => { | ||
| if (disablePaymentId || !to) return paymentId | ||
| try { | ||
| const amountToUse = | ||
| (isFiat(currency) || randomSatoshis) && convertedAmount | ||
| ? convertedAmount | ||
| : amount | ||
|
|
||
| console.log('Creating payment ID with amount:', amountToUse) | ||
| const responsePaymentId = await createPayment(amountToUse, to, apiBaseUrl) | ||
|
|
||
| setPaymentId(responsePaymentId) | ||
| return responsePaymentId | ||
| } catch (err) { | ||
| console.error('Error creating payment ID:', err) | ||
| return undefined | ||
| } | ||
| }, [disablePaymentId, apiBaseUrl, isFiat, randomSatoshis]) |
There was a problem hiding this comment.
Fix amount units for server creation and guard against concurrent calls
For fiat or randomized flows, if convertedAmount isn’t ready, wait for price and compute a crypto amount; also guard to prevent duplicate createPayment calls.
+ const creatingPaymentIdRef = useRef(false);
const getPaymentId = useCallback(async (
currency: Currency,
amount: number,
convertedAmount: number | undefined,
to: string | undefined,
): Promise<string | undefined> => {
- if (disablePaymentId || !to) return paymentId
+ if (disablePaymentId || !to) return paymentId;
+ if (creatingPaymentIdRef.current) return paymentId;
+ creatingPaymentIdRef.current = true;
try {
- const amountToUse =
- (isFiat(currency) || randomSatoshis) && convertedAmount
- ? convertedAmount
- : amount
-
- console.log('Creating payment ID with amount:', amountToUse)
- const responsePaymentId = await createPayment(amountToUse, to, apiBaseUrl)
-
- setPaymentId(responsePaymentId)
- return responsePaymentId
+ let amountToUse = amount;
+ if (isFiat(currency) || randomSatoshis) {
+ if (convertedAmount !== undefined) {
+ amountToUse = convertedAmount;
+ } else {
+ // Wait until price is available if fiat, then compute crypto amount now
+ if (isFiat(currency) && priceRef.current === 0) {
+ await new Promise<void>((resolve) => waitPrice(resolve));
+ }
+ const addressTypeLocal: Currency = getCurrencyTypeFromAddress(to);
+ const convertedObj = getCurrencyObject(
+ isFiat(currency) && priceRef.current ? amount / priceRef.current : amount,
+ addressTypeLocal,
+ randomSatoshis,
+ );
+ amountToUse = convertedObj.float;
+ }
+ }
+ const responsePaymentId = await createPayment(amountToUse, to, apiBaseUrl);
+ setPaymentId(responsePaymentId);
+ return responsePaymentId;
} catch (err) {
- console.error('Error creating payment ID:', err)
- return undefined
+ console.error('Error creating payment ID:', err);
+ return undefined;
+ } finally {
+ creatingPaymentIdRef.current = false;
}
- }, [disablePaymentId, apiBaseUrl, isFiat, randomSatoshis])
+ }, [disablePaymentId, apiBaseUrl, randomSatoshis])Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In react/lib/components/PayButton/PayButton.tsx around lines 139-161, the
current getPaymentId can call createPayment with the wrong units when
convertedAmount is not yet available and it can trigger duplicate concurrent
createPayment calls; fix by (1) if currency is fiat or randomSatoshis and
convertedAmount is undefined, await the price/conversion helper to compute the
proper crypto amount in the server-expected unit before calling createPayment,
(2) add a simple in-flight guard (e.g., an internal promise or boolean lock) so
concurrent calls reuse the same in-flight request rather than issuing duplicate
createPayment requests, (3) ensure you still setPaymentId and return the result,
and (4) update the hook dependency list to include any new helpers/vars you
reference (like paymentId or price helper) so useCallback is stable.
| if (paymentId === undefined && !disablePaymentId) { | ||
| if (to) { | ||
| try { | ||
| const responsePaymentId = await createPayment(amount, to, apiBaseUrl); | ||
| setThisPaymentId(responsePaymentId); | ||
| setFetchingPaymentId(false); |
There was a problem hiding this comment.
Wrong units may be sent to server; use converted crypto amount when fiat or randomSatoshis
When currency is fiat or randomSatoshis is enabled, prefer the converted crypto amount for createPayment.
- const responsePaymentId = await createPayment(amount, to, apiBaseUrl);
+ const amountForPaymentId =
+ ((currency && typeof currency === 'string') && /* fiat check below */ false) || randomSatoshis
+ ? (convertedAmount ?? amount)
+ : amount;
+ const responsePaymentId = await createPayment(amountForPaymentId, to, apiBaseUrl);And import a proper fiat check for correctness:
-} from '../../util';
+ isPropsTrue,
+ isFiat,
+} from '../../util';Optionally include currency, convertedAmount, and randomSatoshis in the effect deps if this path is used in standalone WidgetContainer usage.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In react/lib/components/Widget/WidgetContainer.tsx around lines 155-160, the
code calls createPayment with amount which can be fiat or unconverted when
randomSatoshis is enabled; change it to pass the converted crypto amount instead
(use convertedAmount when currency is fiat or when randomSatoshis is true).
Import and use the existing fiat-check helper (e.g. isFiatCurrency) to detect
fiat currency correctly. Ensure setThisPaymentId and setFetchingPaymentId remain
after awaiting createPayment. Also add currency, convertedAmount, and
randomSatoshis to the effect dependency array so the paymentId flow reacts when
those values change.
lissavxo
force-pushed
the
feat/genrate-paymentId-on-server
branch
from
0f06954 to
6f4ae35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
react/lib/components/Widget/WidgetContainer.tsx (2)
142-171: Fix guard logic, finalization, and re-initialization flow to prevent deadlock.The current implementation has several critical issues that were flagged in previous reviews but remain unresolved:
- Guard blocks all future runs:
fetchingPaymentId !== undefined(line 147) prevents the effect from ever running again after the first execution, even when dependencies change.- Missing finalization in error path: The try-catch block (lines 156-162) doesn't reset
fetchingPaymentIdon error, leaving it stuck attrueforever.- Additional problematic guard:
thisPaymentId !== undefined(line 148) prevents re-initialization whento,amount, or other deps change but a paymentId already exists.- Missing critical dependencies:
convertedAmount,currency, andrandomSatoshisare not in the dependency array, so the effect won't re-run when conversion completes or these values change.Apply this fix to resolve the guard logic and finalization issues:
- const [fetchingPaymentId, setFetchingPaymentId] = useState<boolean | undefined>(); + const [fetchingPaymentId, setFetchingPaymentId] = useState<boolean>(false);useEffect(() => { - if ( - fetchingPaymentId !== undefined || - thisPaymentId !== undefined - ) { - return + if (fetchingPaymentId) return; + + // If disabled or a paymentId is provided, just propagate it + if (disablePaymentId || (paymentId !== undefined && paymentId !== '')) { + setThisPaymentId(paymentId); + return; } - setFetchingPaymentId(true) + + // Cannot create without an address + if (!to) return; + + setFetchingPaymentId(true); const initializePaymentId = async () => { - if (paymentId === undefined && !disablePaymentId) { - if (to) { - try { - const responsePaymentId = await createPayment(amount, to, apiBaseUrl); - setThisPaymentId(responsePaymentId); - setFetchingPaymentId(false); - } catch (error) { - console.error('Error creating payment ID:', error); - } - } - } else { + try { + const responsePaymentId = await createPayment(amount, to, apiBaseUrl); + setThisPaymentId(responsePaymentId); + } catch (error) { + console.error('Error creating payment ID:', error); + } finally { + setFetchingPaymentId(false); + } + }; + + initializePaymentId(); + }, [paymentId, disablePaymentId, amount, to, apiBaseUrl, fetchingPaymentId]);Based on learnings
157-157: Send converted crypto amount instead of fiat or unconverted amount.When
currencyis fiat orrandomSatoshisis enabled, the server expects the converted crypto amount but receives the unconvertedamount(which could be a fiat value). This issue was flagged in a previous review but remains unresolved.You need to:
- Use
convertedAmount(fromconvertedCurrencyObj) when available for fiat or randomSatoshis flows- Add
currency,convertedAmount, andrandomSatoshisto the effect dependencies so it re-runs when conversion completes+ // Determine the correct amount to send based on currency type + const amountForPaymentId = + (isFiat(currency) || randomSatoshis) && convertedAmount + ? convertedAmount + : amount; const initializePaymentId = async () => { try { - const responsePaymentId = await createPayment(amount, to, apiBaseUrl); + const responsePaymentId = await createPayment(amountForPaymentId, to, apiBaseUrl); setThisPaymentId(responsePaymentId); } catch (error) { console.error('Error creating payment ID:', error); } finally { setFetchingPaymentId(false); } }; initializePaymentId(); - }, [paymentId, disablePaymentId, amount, to, apiBaseUrl, fetchingPaymentId]); + }, [paymentId, disablePaymentId, amount, to, apiBaseUrl, fetchingPaymentId, currency, convertedAmount, randomSatoshis]);Also add the import for the fiat check helper:
isPropsTrue, + isFiat, } from '../../util';Based on learnings
react/lib/components/PayButton/PayButton.tsx (3)
116-121: Add effect to reset paymentId when key parameters change.Without resetting
paymentIdwhento,currency,amount, orrandomSatoshischange, the component can reuse IDs bound to previous addresses, amounts, or unit policies. This causes validation mismatches on the server. This issue was flagged in a previous review but remains unresolved.const [paymentId, setPaymentId] = useState<string | undefined>(undefined); + + useEffect(() => { + setPaymentId(undefined); + }, [to, currency, amount, randomSatoshis]); + const [addressType, setAddressType] = useState<CryptoCurrency>( getCurrencyTypeFromAddress(to), );Based on learnings
163-188: Create paymentId before calling onOpen and opening dialog; ensure price is ready for fiat.The current flow calls
onOpenwith a stale/undefinedpaymentId(lines 165-171) before creating it (lines 173-175), then opens the dialog (line 177) before paymentId creation completes. This issue was flagged in a previous review but remains unresolved.Additionally, for fiat currencies,
waitPriceis called withvoid(line 167), making it fire-and-forget rather than awaiting completion.const handleButtonClick = useCallback(async (): Promise<void> => { - + let pid = paymentId; + + // Create paymentId first if needed + if (!disablePaymentId && !pid) { + // Ensure price is ready for fiat before getPaymentId computes crypto amount + if (isFiat(currency) && priceRef.current === 0) { + await new Promise<void>((resolve) => waitPrice(resolve)); + } + pid = await getPaymentId(currency, Number(amount), convertedAmount, to); + } + + // Now call onOpen with the correct paymentId if (onOpen) { if (isFiat(currency)) { - void waitPrice(() => onOpen(cryptoAmountRef.current, to, paymentId)) + await new Promise<void>((resolve) => waitPrice(resolve)); + onOpen(cryptoAmountRef.current, to, pid); } else { - onOpen(amount, to, paymentId) + onOpen(amount, to, pid); } } - - if (!disablePaymentId && !paymentId) { - await getPaymentId(currency, Number(amount), convertedAmount, to) - } - - setDialogOpen(true) + + setDialogOpen(true); }, [ onOpen, - isFiat, currency, amount, to, paymentId, disablePaymentId, getPaymentId, convertedAmount, ])Based on learnings
140-161: Fix getPaymentId: remove invalid dependency, add concurrent guard, handle missing convertedAmount, and fix early return.Multiple critical issues in this callback:
- Invalid dependency (line 161):
isFiatis a function imported from util, not a variable. Including it in the dependency array is incorrect and will cause the callback to be recreated on every render.- Missing concurrent call guard: Multiple simultaneous calls can create duplicate paymentIds.
- Wrong units when convertedAmount is unavailable: When
isFiat(currency)orrandomSatoshisis true butconvertedAmountis still undefined, sends the unconverted fiat amount.- Incorrect early return (line 146): Returns the current
paymentIdvalue instead ofundefinedwhendisablePaymentIdis true.+ const creatingPaymentIdRef = useRef(false); + const getPaymentId = useCallback(async ( currency: Currency, amount: number, convertedAmount: number | undefined, to: string | undefined, ): Promise<string | undefined> => { - if (disablePaymentId || !to) return paymentId + if (disablePaymentId || !to) return undefined; + if (creatingPaymentIdRef.current) return paymentId; + + creatingPaymentIdRef.current = true; try { - const amountToUse = - (isFiat(currency) || randomSatoshis) && convertedAmount - ? convertedAmount - : amount - - const responsePaymentId = await createPayment(amountToUse, to, apiBaseUrl) - - setPaymentId(responsePaymentId) - return responsePaymentId + let amountToUse = amount; + + // For fiat or randomSatoshis, use the converted crypto amount + if (isFiat(currency) || randomSatoshis) { + if (convertedAmount !== undefined) { + amountToUse = convertedAmount; + } else { + // convertedAmount not ready yet - shouldn't happen if flow is correct + console.warn('convertedAmount not available for fiat/randomSatoshis flow'); + } + } + + const responsePaymentId = await createPayment(amountToUse, to, apiBaseUrl); + setPaymentId(responsePaymentId); + return responsePaymentId; } catch (err) { - console.error('Error creating payment ID:', err) - return undefined + console.error('Error creating payment ID:', err); + return undefined; + } finally { + creatingPaymentIdRef.current = false; } - }, [disablePaymentId, apiBaseUrl, isFiat, randomSatoshis]) + }, [disablePaymentId, apiBaseUrl, paymentId, randomSatoshis]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
react/lib/components/PayButton/PayButton.tsx(6 hunks)react/lib/components/PaymentDialog/PaymentDialog.tsx(3 hunks)react/lib/components/Widget/Widget.tsx(4 hunks)react/lib/components/Widget/WidgetContainer.tsx(6 hunks)react/lib/util/api-client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- react/lib/components/PaymentDialog/PaymentDialog.tsx
- react/lib/util/api-client.ts
- react/lib/components/Widget/Widget.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
react/lib/components/PayButton/PayButton.tsx (6)
react/lib/util/types.ts (4)
CurrencyObject(4-8)Transaction(10-21)CryptoCurrency(138-138)Currency(140-140)react/lib/util/address.ts (1)
getCurrencyTypeFromAddress(26-34)react/lib/util/currency.ts (1)
isFiat(4-6)react/lib/util/api-client.ts (1)
createPayment(92-107)react/lib/util/socket.ts (2)
setupChronikWebSocket(141-153)setupAltpaymentSocket(95-114)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)
react/lib/components/Widget/WidgetContainer.tsx (3)
react/lib/util/types.ts (1)
CurrencyObject(4-8)react/lib/util/format.ts (2)
isPropsTrue(66-75)amount(5-12)react/lib/util/api-client.ts (1)
createPayment(92-107)
🔇 Additional comments (2)
react/lib/components/Widget/WidgetContainer.tsx (1)
337-338: Props correctly forwarded to Widget.The
convertedCurrencyObjandsetConvertedCurrencyObjprops are properly passed to the Widget component, enabling shared conversion state across the component tree.react/lib/components/PayButton/PayButton.tsx (1)
406-407: Props correctly forwarded to PaymentDialog.The
convertedCurrencyObjandsetConvertedCurrencyObjprops are properly passed to PaymentDialog, enabling currency conversion state sharing across the component tree.
| useEffect(() => { | ||
| if (currencyObj && isFiat(currency) && price) { | ||
| const addressType: Currency = getCurrencyTypeFromAddress(to); | ||
| if(!convertedCurrencyObj) { | ||
| const addressType: Currency = getCurrencyTypeFromAddress(to); | ||
| const convertedObj = getCurrencyObject( | ||
| currencyObj.float / price, | ||
| addressType, | ||
| randomSatoshis, | ||
| ); | ||
| setCryptoAmount(convertedObj.string); | ||
| setConvertedAmount(convertedObj.float); | ||
| setConvertedCurrencyObj(convertedObj); | ||
| } | ||
| } else if (!isFiat(currency) && randomSatoshis && !convertedAmount){ | ||
| const convertedObj = getCurrencyObject( | ||
| currencyObj.float / price, | ||
| amount as number, | ||
| addressType, | ||
| randomSatoshis, | ||
| ); | ||
| ); | ||
| setCryptoAmount(convertedObj.string); | ||
| } else if (!isFiat(currency)) { | ||
| setConvertedAmount(convertedObj.float); | ||
| setConvertedCurrencyObj(convertedObj); | ||
| } else if (!isFiat(currency) && !randomSatoshis) { | ||
| setCryptoAmount(amount?.toString()); | ||
| } | ||
| }, [price, currencyObj, amount, currency, randomSatoshis, to]); |
There was a problem hiding this comment.
Remove guards that prevent recalculation when dependencies change.
The guards at lines 309 (if(!convertedCurrencyObj)) and 320 (&& !convertedAmount) prevent the conversion logic from re-running when price, currencyObj, amount, currency, randomSatoshis, or to change. This can leave stale conversion values when parameters update.
useEffect(() => {
if (currencyObj && isFiat(currency) && price) {
- if(!convertedCurrencyObj) {
- const addressType: Currency = getCurrencyTypeFromAddress(to);
- const convertedObj = getCurrencyObject(
- currencyObj.float / price,
- addressType,
- randomSatoshis,
- );
- setCryptoAmount(convertedObj.string);
- setConvertedAmount(convertedObj.float);
- setConvertedCurrencyObj(convertedObj);
- }
- } else if (!isFiat(currency) && randomSatoshis && !convertedAmount){
+ const addressType: Currency = getCurrencyTypeFromAddress(to);
+ const convertedObj = getCurrencyObject(
+ currencyObj.float / price,
+ addressType,
+ randomSatoshis,
+ );
+ setCryptoAmount(convertedObj.string);
+ setConvertedAmount(convertedObj.float);
+ setConvertedCurrencyObj(convertedObj);
+ } else if (!isFiat(currency) && randomSatoshis) {
const convertedObj = getCurrencyObject(
amount as number,
addressType,
randomSatoshis,
- );
+ );
setCryptoAmount(convertedObj.string);
setConvertedAmount(convertedObj.float);
setConvertedCurrencyObj(convertedObj);
} else if (!isFiat(currency) && !randomSatoshis) {
setCryptoAmount(amount?.toString());
}
}, [price, currencyObj, amount, currency, randomSatoshis, to]);🤖 Prompt for AI Agents
In react/lib/components/PayButton/PayButton.tsx around lines 307 to 332, the two
guards (if(!convertedCurrencyObj) and && !convertedAmount) stop recalculation
when dependencies change; remove those guards so the useEffect always recomputes
convertedObj when its inputs change: always compute convertedObj and call
setCryptoAmount, setConvertedAmount and setConvertedCurrencyObj in the fiat
branch when currencyObj && isFiat(currency) && price, and always compute
convertedObj and set the same values in the non-fiat branch when
!isFiat(currency) && randomSatoshis; keep the final branch setting cryptoAmount
when !isFiat(currency) && !randomSatoshis; ensure addressType is computed where
needed and that state is overwritten on each calculation to avoid stale values.
Related to #66
Description
Generate paymentId on server
Test plan
Create a button with paymentId make a payment make sure it works
Summary by CodeRabbit
New Features
Bug Fixes