Convert SimplifiedTransaction[] to Transaction[] type in getAddressDe…#627
Conversation
|
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:
📝 WalkthroughWalkthroughValidates HTTP/JSON for address transaction responses, ensures the payload is an array, maps API entries into normalized Transaction objects, adds a JSON-stringified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react/lib/util/api-client.ts`:
- Around line 39-41: The code calls apiTransaction.inputAddresses.map(...) which
will throw if apiTransaction.inputAddresses is null/undefined; update the
construction that sets inputAddresses to either
apiTransaction.inputAddresses.map(...) when present or an empty array otherwise
(e.g., use a guard or default like (apiTransaction.inputAddresses ||
[]).map(...)) so the property inputAddresses is always an array; refer to the
mapping expression that extracts input.address from
apiTransaction.inputAddresses to locate the change.
- Around line 31-33: The API returns apiTransaction.amount as a number but
Transaction.amount is typed as string, so normalize the value when building the
Transaction object: convert apiTransaction.amount to a string (e.g., via
toString or String(...)) before assigning to the Transaction.amount field (the
block that sets hash: apiTransaction.hash, amount: apiTransaction.amount,
paymentId: apiTransaction.paymentId). Ensure you handle null/undefined safely
(e.g., String(apiTransaction.amount) or apiTransaction.amount?.toString()) so
downstream consumers see a string-typed amount.
- Around line 19-46: Check the fetch response and payload before iterating:
after calling fetch (res) verify res.ok and throw or return an empty
transactions array with a clear error log if not OK; then parse JSON into
apiTransactions and ensure Array.isArray(apiTransactions) before using forEach
(otherwise handle non-array by returning [] or throwing). Also validate that
apiTransaction.inputAddresses is an array before mapping it (used in
inputAddresses mapping) and ensure opReturn/rawMessage fields are safely
accessed to avoid runtime errors when shaping each Transaction.
Klakurka
added
bug
Fabcien
force-pushed
the
getaddressdetails_conversion
branch
from
01fdbeb to
dedfc71
Compare
|
Added more defensive programming to avoid errors from the array methods |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
react/lib/util/api-client.ts (1)
36-38:⚠️ Potential issue | 🟡 MinorNormalize
amountto a string.
Transaction.amountis typed asstring, but this still assigns the raw API value. Normalizing here prevents downstream type/format inconsistencies.🔧 Suggested normalization
- amount: apiTransaction.amount, + amount: apiTransaction.amount != null ? String(apiTransaction.amount) : '',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/util/api-client.ts` around lines 36 - 38, The assignment is taking apiTransaction.amount as-is even though Transaction.amount is typed string; convert/normalize apiTransaction.amount to a string when building the transaction object (the block assigning hash, amount, paymentId) so downstream consumers always receive a string for Transaction.amount—e.g., coerce apiTransaction.amount via String(...) or .toString() on the apiTransaction.amount value before assigning to amount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react/lib/util/api-client.ts`:
- Around line 19-25: The current code calls res.json() without handling parse
errors so a non-JSON error body will throw before your res.ok/Array.isArray
check; wrap the JSON parsing in a try/catch (or read res.text() and safely
JSON.parse it) around the res.json() call used for apiTransactions in the fetch
block (where res and apiTransactions are defined) and on parse failure log the
response and return [] so the function reliably returns an empty array when the
body is not valid JSON.
---
Duplicate comments:
In `@react/lib/util/api-client.ts`:
- Around line 36-38: The assignment is taking apiTransaction.amount as-is even
though Transaction.amount is typed string; convert/normalize
apiTransaction.amount to a string when building the transaction object (the
block assigning hash, amount, paymentId) so downstream consumers always receive
a string for Transaction.amount—e.g., coerce apiTransaction.amount via
String(...) or .toString() on the apiTransaction.amount value before assigning
to amount.
…tails The API is likely to keep evolving and maintaining the types in sync is brittle as demonstrated here. Apply a proper conversion function to ensure the format is as expected. Test Plan: yarn test Check payment detection still works on mobile.
Fabcien
force-pushed
the
getaddressdetails_conversion
branch
from
dedfc71 to
521d325
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
react/lib/util/api-client.ts (1)
47-50:⚠️ Potential issue | 🟡 MinorNormalize
amountto string to matchTransaction.
Transaction.amountis typed asstring, but Line 49 assigns the raw API value. Convert to string here to prevent downstream type/format inconsistencies.🔧 Suggested normalization
- amount: apiTransaction.amount, + amount: String(apiTransaction.amount),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/util/api-client.ts` around lines 47 - 50, The Transaction object construction assigns apiTransaction.amount directly but Transaction.amount is typed as string; update the mapping where the transaction constant is created (the transaction variable that uses apiTransaction) to normalize the amount to a string (e.g., via String(apiTransaction.amount) or .toString()) so the created Transaction conforms to the Transaction type and prevents downstream format/type issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@react/lib/util/api-client.ts`:
- Around line 47-50: The Transaction object construction assigns
apiTransaction.amount directly but Transaction.amount is typed as string; update
the mapping where the transaction constant is created (the transaction variable
that uses apiTransaction) to normalize the amount to a string (e.g., via
String(apiTransaction.amount) or .toString()) so the created Transaction
conforms to the Transaction type and prevents downstream format/type issues.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react/lib/util/api-client.ts (1)
65-85: WrapamountinDecimal()to normalize precision handling.
SimplifiedTransactiondeclaresamount: Decimal, butres.json()yields primitive numbers. InstantiatingDecimalhere aligns runtime behavior with the type and ensures arbitrary-precision decimal handling rather than JavaScript number precision, which is especially important for financial values.Suggested fix
- amount: apiTransaction.amount.toString(), + amount: new Decimal(apiTransaction.amount).toString(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/util/api-client.ts` around lines 65 - 85, The transaction mapping uses apiTransaction.amount as a JS number but the declared type is Decimal; update the mapping in the block that builds each Transaction (inside the apiTransactions.forEach) to wrap the incoming amount with Decimal(), e.g., set the Transaction.amount to Decimal(apiTransaction.amount).toString() or otherwise normalize to a Decimal representation before storing, ensuring you import/require Decimal and handle cases where apiTransaction.amount may already be a Decimal or null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react/lib/util/api-client.ts`:
- Around line 65-85: The transaction mapping uses apiTransaction.amount as a JS
number but the declared type is Decimal; update the mapping in the block that
builds each Transaction (inside the apiTransactions.forEach) to wrap the
incoming amount with Decimal(), e.g., set the Transaction.amount to
Decimal(apiTransaction.amount).toString() or otherwise normalize to a Decimal
representation before storing, ensuring you import/require Decimal and handle
cases where apiTransaction.amount may already be a Decimal or null.
| import { CURRENCY_TYPES_MAP, DECIMALS } from './constants'; | ||
| import Decimal from 'decimal.js'; | ||
|
|
||
| interface SimplifiedTransaction { |
There was a problem hiding this comment.
What's the point ? This is an API call so the type can't be enforced. This is guaranteed to go out of sync sooner or later. The whole point of this PR is to enforce the returned type when the input type is subject to change, so this type adds a false sense of type safety which is worst than no type imo
There was a problem hiding this comment.
I added mostly because I saw (via code rabbit ai) that a type was being wrongly casted from Decimal to string: not a huge deal of course, but for me it's more about keeping the contract between client and server explicit.
I also don't think is a big deal keeping both in sync, logistically speaking, since we control both the client and the server. Besides that, changing the server api's response would most likely require a change in this file (or at least reviewing it), so having them tied together looks like a good thing to me, but maybe I am not seeing it right.
In any case, I don't think this is significant enough to be worth discussing it too much, so if you prefer, you can just revert my commit (but probably keep the amount.toString() part)
chedieck
force-pushed
the
getaddressdetails_conversion
branch
from
d53e3e4 to
8bc9390
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
react/lib/util/api-client.ts (1)
51-57:⚠️ Potential issue | 🟡 MinorMisleading catch-block warning; consider typing
apiTransactionsasunknown.Two minor points on this block:
- The
catchmessage ("Failed to fetch address transactions") is inaccurate — the fetch itself already succeeded (past theres.okguard). This branch is a JSON-parse failure, not a fetch failure.let apiTransactionsis implicitly typedany. Annotating asunknownand narrowing afterward is safer.🔧 Proposed fix
- let apiTransactions; + let apiTransactions: unknown; try { apiTransactions = await res.json(); } catch (error) { - console.warn(`Failed to fetch address transactions from ${rootUrl}/address/transactions/${address}`, error); + console.warn(`Failed to parse JSON response from ${rootUrl}/address/transactions/${address}`, error); return []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/util/api-client.ts` around lines 51 - 57, The catch block after awaiting res.json() is misleading and leaves apiTransactions as implicit any; change apiTransactions to be explicitly typed unknown (e.g., let apiTransactions: unknown) and update the catch log to indicate a JSON parse/deserialize failure (mention res.json()) rather than a fetch failure; after successful parsing, perform proper runtime/type narrowing or validation of apiTransactions before using it. Ensure you reference the apiTransactions variable, the await res.json() call, and the catch block when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@react/lib/util/api-client.ts`:
- Around line 51-57: The catch block after awaiting res.json() is misleading and
leaves apiTransactions as implicit any; change apiTransactions to be explicitly
typed unknown (e.g., let apiTransactions: unknown) and update the catch log to
indicate a JSON parse/deserialize failure (mention res.json()) rather than a
fetch failure; after successful parsing, perform proper runtime/type narrowing
or validation of apiTransactions before using it. Ensure you reference the
apiTransactions variable, the await res.json() call, and the catch block when
making these changes.
| const opReturn = { | ||
| rawMessage: apiTransaction.rawMessage, | ||
| message: apiTransaction.message, | ||
| paymentId: apiTransaction.paymentId, | ||
| }; | ||
| const transaction: Transaction = { | ||
| hash: apiTransaction.hash, | ||
| amount: apiTransaction.amount, | ||
| paymentId: apiTransaction.paymentId, | ||
| confirmed: apiTransaction.confirmed, | ||
| message: apiTransaction.message, | ||
| timestamp: apiTransaction.timestamp, | ||
| address: apiTransaction.address, | ||
| rawMessage: apiTransaction.rawMessage, | ||
| // Only keep the address string, drop the amount | ||
| inputAddresses: Array.isArray(apiTransaction.inputAddresses) ? apiTransaction.inputAddresses.map((input: { address: string, amount: string }) => input.address) : [], | ||
| opReturn: JSON.stringify(opReturn), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts '\bopReturn\b' -A 3 -B 3Repository: PayButton/paybutton
Length of output: 49620
🏁 Script executed:
rg -n 'getTransactions|SimplifiedTransaction' --type=ts -B 2 -A 5 | head -100Repository: PayButton/paybutton
Length of output: 1035
🏁 Script executed:
rg -n 'getTransactions|from.*api-client' --type=ts -B 2 -A 8 | head -150Repository: PayButton/paybutton
Length of output: 1538
🏁 Script executed:
rg -n 'getAddressDetails' --type=ts -B 2 -A 10 | head -200Repository: PayButton/paybutton
Length of output: 3433
🏁 Script executed:
rg -n 'handleNewTransaction' --type=ts -B 5 -A 15Repository: PayButton/paybutton
Length of output: 4476
🏁 Script executed:
rg -n 'const handlePayment|handlePayment.*=.*\(' --type=ts -B 2 -A 15 | head -200Repository: PayButton/paybutton
Length of output: 1720
🏁 Script executed:
rg -n 'shouldTriggerOnSuccess' --type=ts -B 2 -A 12 | head -100Repository: PayButton/paybutton
Length of output: 6200
🏁 Script executed:
cat -n react/lib/util/validate.ts | sed -n '7,80p'Repository: PayButton/paybutton
Length of output: 2453
🏁 Script executed:
cat -n react/lib/util/api-client.ts | sed -n '60,90p'Repository: PayButton/paybutton
Length of output: 1425
🏁 Script executed:
rg -n 'transaction\.opReturn|tx\.opReturn' --type=ts -B 2 -A 3Repository: PayButton/paybutton
Length of output: 45
🏁 Script executed:
cat -n react/lib/util/chronik.ts | sed -n '116,160p'Repository: PayButton/paybutton
Length of output: 2078
🏁 Script executed:
cat -n react/lib/util/validate.ts | sed -n '50,62p'Repository: PayButton/paybutton
Length of output: 803
opReturn is always a truthy JSON string, masking "no OP_RETURN" cases and breaking payment validation.
opReturn is unconditionally set to JSON.stringify({ rawMessage, message, paymentId }). When there is no real OP_RETURN data, all three fields are empty strings, producing {"rawMessage":"","message":"","paymentId":""} — a truthy non-empty string. The shouldTriggerOnSuccess validation in validate.ts (line 56) explicitly checks for empty string or undefined to detect "no OP_RETURN," but receives this truthy JSON string instead. When a payment has no expected OP_RETURN (expectedOpReturn undefined), validation fails with the api-client transaction but passes with chronik transactions (which leave opReturn as empty string when no data exists). Transaction.opReturn is declared optional (opReturn?: string) to distinguish "present" from "absent," but the current implementation defeats this purpose.
🔧 Proposed fix — only set `opReturn` when meaningful data exists
- const opReturn = {
- rawMessage: apiTransaction.rawMessage,
- message: apiTransaction.message,
- paymentId: apiTransaction.paymentId,
- };
const transaction: Transaction = {
hash: apiTransaction.hash,
amount: apiTransaction.amount,
paymentId: apiTransaction.paymentId,
confirmed: apiTransaction.confirmed,
message: apiTransaction.message,
timestamp: apiTransaction.timestamp,
address: apiTransaction.address,
rawMessage: apiTransaction.rawMessage,
inputAddresses: Array.isArray(apiTransaction.inputAddresses)
? apiTransaction.inputAddresses.map((input: { address: string, amount: string }) => input.address)
: [],
- opReturn: JSON.stringify(opReturn),
+ ...(apiTransaction.rawMessage || apiTransaction.message || apiTransaction.paymentId
+ ? {
+ opReturn: JSON.stringify({
+ rawMessage: apiTransaction.rawMessage,
+ message: apiTransaction.message,
+ paymentId: apiTransaction.paymentId,
+ }),
+ }
+ : {}),
};
3 participants
…tails
The API is likely to keep evolving and maintaining the types in sync is brittle as demonstrated here. Apply a proper conversion function to ensure the format is as expected.
Test Plan:
yarn test
Check payment detection still works on mobile.
Summary by CodeRabbit
Bug Fixes
Refactor