Paul/add edge asset#207
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ): Promise<EdgeAssetInfo> { | ||
| if (network == null) { | ||
| throw new Error(`Missing network for asset: ${asset}`) | ||
| } |
There was a problem hiding this comment.
Bug: Missing date cutoff for network field requirement
The getAssetInfo function throws an error if network is null, but unlike other partners (changehero, exolix, godex) there's no date-based cutoff to gracefully handle older transactions. The cleaner at lines 151 and 158 marks depositNetwork and settleNetwork as optional, suggesting historical transactions may not have these fields. Other partners use constants like CHAIN_FIELDS_REQUIRED_DATE to skip asset info backfill for older transactions, but sideshift throws unconditionally, which could cause the entire sync to fail when processing historical data.
There was a problem hiding this comment.
Fixed in 685dc66. Added NETWORK_FIELDS_REQUIRED_DATE = '2023-01-01' and modified getAssetInfo to return undefined asset info for older transactions with null network, matching the pattern used in exolix/changehero/letsexchange.
There was a problem hiding this comment.
This has not been determined to be necessary for a sideshift. We can't just add an arbitrary date. This change should be removed
paullinator
force-pushed
the
paul/addEdgeAsset
branch
3 times, most recently
from
8466888 to
ddfaa68
Compare
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
Partial review. I included an idea in my fixup! commit too.
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
Finally finished with this. Whew
paullinator
force-pushed
the
paul/addEdgeAsset
branch
from
c81d6fb to
e2cebf6
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 7 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 7 issues found in the latest run.
- ✅ Fixed: Debug console.log left in production code
- Replaced the raw
console.login LiFi transaction processing with the scopedlog()call used elsewhere in the plugin.
- Replaced the raw
- ✅ Fixed: Rango tokenId uses raw addresses without normalization
- Rango now derives both deposit and payout
tokenIdvalues throughcreateTokenId(...)withtokenTypesinstead of storing raw addresses.
- Rango now derives both deposit and payout
- ✅ Fixed: LetsExchange status cleaner lacks fallback for unknown values
- Wrapped
asLetsExchangeStatuswithasMaybe(..., 'other')and added'other'tostatusMapso unknown statuses no longer throw in the cleaner.
- Wrapped
- ✅ Fixed: LetsExchange asValue has duplicate status entries
- Removed duplicate
'overdue'and'refund'entries from the LetsExchange status cleaner value list.
- Removed duplicate
- ✅ Fixed: ChangeNow conflates missing cache entry with native token
- Updated ChangeNow asset lookup to treat only
nullas native and throw onundefinedcache misses instead of mapping both totokenId: null.
- Updated ChangeNow asset lookup to treat only
- ✅ Fixed: Unused constant NETWORK_FIELDS_AVAILABLE_DATE defined
- Integrated
NETWORK_FIELDS_AVAILABLE_DATEinto LetsExchange asset resolution to allow missing network fields only before the cutoff and throw afterward.
- Integrated
- ✅ Fixed: Godex coins cache persists incomplete state on API failure
- Godex now caches coin data only on successful API responses and rethrows fetch errors so an incomplete fallback cache is not persisted.
Or push these changes by commenting:
@cursor push ecf51debb2
Preview (ecf51debb2)
diff --git a/src/partners/changenow.ts b/src/partners/changenow.ts
--- a/src/partners/changenow.ts
+++ b/src/partners/changenow.ts
@@ -344,14 +344,17 @@
// Look up contract address from cache
const contractAddress = getContractFromCache(currencyCode, network)
- // If not in cache or no contract address, it's a native token
- if (contractAddress == null) {
+ // null means native token, undefined means cache miss
+ if (contractAddress === null) {
return {
chainPluginId,
evmChainId,
tokenId: null
}
}
+ if (contractAddress === undefined) {
+ throw new Error(`Currency info not found for ${currencyCode} on ${network}`)
+ }
// Create tokenId from contract address
const tokenType = tokenTypes[chainPluginId]
diff --git a/src/partners/godex.ts b/src/partners/godex.ts
--- a/src/partners/godex.ts
+++ b/src/partners/godex.ts
@@ -132,6 +132,10 @@
try {
const url = 'https://api.godex.io/api/v1/coins'
const result = await retryFetch(url, { method: 'GET' })
+ if (!result.ok) {
+ const text = await result.text()
+ throw new Error(`Failed to fetch Godex coins: ${text}`)
+ }
const json = await result.json()
const coins = asGodexCoinsResponse(json)
@@ -149,11 +153,12 @@
}
}
log(`Coins cache loaded: ${cache.size} entries`)
+ godexCoinsCache = cache
+ return cache
} catch (e) {
- log.error('Error loading coins cache:', e)
+ log.error(`Error loading coins cache: ${String(e)}`)
+ throw e
}
- godexCoinsCache = cache
- return cache
}
interface GodexEdgeAssetInfo {
diff --git a/src/partners/letsexchange.ts b/src/partners/letsexchange.ts
--- a/src/partners/letsexchange.ts
+++ b/src/partners/letsexchange.ts
@@ -45,22 +45,23 @@
})
})
-const asLetsExchangeStatus = asValue(
- 'wait',
- 'confirmation',
- 'confirmed',
- 'exchanging',
- 'overdue',
- 'refund',
- 'sending',
- 'transferring',
- 'sending_confirmation',
- 'success',
- 'aml_check_failed',
- 'overdue',
- 'error',
- 'canceled',
- 'refund'
+const asLetsExchangeStatus = asMaybe(
+ asValue(
+ 'wait',
+ 'confirmation',
+ 'confirmed',
+ 'exchanging',
+ 'overdue',
+ 'refund',
+ 'sending',
+ 'transferring',
+ 'sending_confirmation',
+ 'success',
+ 'aml_check_failed',
+ 'error',
+ 'canceled'
+ ),
+ 'other'
)
// Cleaner for the new v2 API response
@@ -128,7 +129,8 @@
success: 'complete',
aml_check_failed: 'blocked',
canceled: 'cancelled',
- error: 'failed'
+ error: 'failed',
+ other: 'other'
}
// Map LetsExchange network codes to Edge pluginIds
@@ -289,14 +291,15 @@
initialNetwork: string | null,
currencyCode: string,
contractAddress: string | null,
- log: ScopedLog
+ isoDate: string
): AssetInfo | undefined {
- let network = initialNetwork
- if (network == null) {
- // Try using the currencyCode as the network
- network = currencyCode
- log(`Using currencyCode as network: ${network}`)
+ if (initialNetwork == null) {
+ if (isoDate < NETWORK_FIELDS_AVAILABLE_DATE) {
+ return undefined
+ }
+ throw new Error(`Missing network for currency ${currencyCode}`)
}
+ const network = initialNetwork
const networkUpper = network.toUpperCase()
const chainPluginId = LETSEXCHANGE_NETWORK_TO_PLUGIN_ID[networkUpper]
@@ -500,14 +503,14 @@
tx.coin_from_network ?? tx.network_from_code,
tx.coin_from,
tx.coin_from_contract_address,
- log
+ isoDate
)
// Get payout asset info using contract address from API response
const payoutAsset = getAssetInfo(
tx.coin_to_network ?? tx.network_to_code,
tx.coin_to,
tx.coin_to_contract_address,
- log
+ isoDate
)
const status = statusMap[tx.status]
diff --git a/src/partners/lifi.ts b/src/partners/lifi.ts
--- a/src/partners/lifi.ts
+++ b/src/partners/lifi.ts
@@ -297,7 +297,7 @@
}
if (statusMap[tx.status] === 'complete') {
const { orderId, depositCurrency, payoutCurrency } = standardTx
- console.log(
+ log(
`${orderId} ${depositCurrency} ${depositChainPluginId} ${depositEvmChainId} ${depositTokenId?.slice(
0,
6
diff --git a/src/partners/rango.ts b/src/partners/rango.ts
--- a/src/partners/rango.ts
+++ b/src/partners/rango.ts
@@ -19,6 +19,7 @@
Status
} from '../types'
import { retryFetch } from '../util'
+import { createTokenId, tokenTypes } from '../util/asEdgeTokenId'
import { EVM_CHAIN_IDS } from '../util/chainIds'
// Start date for Rango transactions (first Edge transaction was 2024-06-23)
@@ -268,9 +269,17 @@
const dateStr = isoDate.split('T')[0]
const depositCurrency = firstStep.fromToken.symbol
- const depositTokenId = firstStep.fromToken.address ?? null
+ const depositTokenId = createTokenId(
+ tokenTypes[depositChainPluginId],
+ depositCurrency,
+ firstStep.fromToken.address ?? undefined
+ )
const payoutCurrency = lastStep.toToken.symbol
- const payoutTokenId = lastStep.toToken.address ?? null
+ const payoutTokenId = createTokenId(
+ tokenTypes[payoutChainPluginId],
+ payoutCurrency,
+ lastStep.toToken.address ?? undefined
+ )
log(
`${dateStr} ${depositCurrency} ${depositAmount} ${depositChainPluginId}${
@@ -299,7 +308,7 @@
payoutCurrency: lastStep.toToken.symbol,
payoutChainPluginId,
payoutEvmChainId,
- payoutTokenId: lastStep.toToken.address ?? null,
+ payoutTokenId,
payoutAmount,
timestamp,
isoDate,
paullinator
force-pushed
the
paul/addEdgeAsset
branch
from
e2cebf6 to
6ed36d2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for 3 of the 4 issues found in the latest run.
- ✅ Fixed: Moonpay sell transactions misclassified as buy transactions
- I changed Moonpay direction detection to use sell-specific fields (
quoteCurrency,payoutMethod,depositHash) so sell transactions are no longer misclassified whenpaymentMethodis present.
- I changed Moonpay direction detection to use sell-specific fields (
- ✅ Fixed: LetsExchange required fields make null guard dead code
- I restored
affiliateIdandapiKeyto optional cleaner fields so the existing null guard can gracefully return empty results when config keys are missing.
- I restored
- ✅ Fixed: Bitrefill deposits for bitcoin miss altcoin amount edge case
- I added a fallback to
btcPricewhen non-bitcoin transactions unexpectedly lackaltcoinPrice, preventing unnecessary transaction-processing failures.
- I added a fallback to
Or push these changes by commenting:
@cursor push 0da1faad74
Preview (0da1faad74)
diff --git a/src/partners/bitrefill.ts b/src/partners/bitrefill.ts
--- a/src/partners/bitrefill.ts
+++ b/src/partners/bitrefill.ts
@@ -279,10 +279,9 @@
const timestamp = tx.invoiceTime / 1000
const { paymentMethod } = tx
- let depositAmountStr: string | undefined
- if (paymentMethod === 'bitcoin') {
- depositAmountStr = tx.btcPrice
- } else if (tx.altcoinPrice != null) {
+ // Fallback to btcPrice when altcoinPrice is unexpectedly missing.
+ let depositAmountStr: string | undefined = tx.btcPrice
+ if (paymentMethod !== 'bitcoin' && tx.altcoinPrice != null) {
depositAmountStr = tx.altcoinPrice
}
if (depositAmountStr == null) {
diff --git a/src/partners/letsexchange.ts b/src/partners/letsexchange.ts
--- a/src/partners/letsexchange.ts
+++ b/src/partners/letsexchange.ts
@@ -40,8 +40,8 @@
latestIsoDate: asOptional(asString, LETSEXCHANGE_START_DATE)
}),
apiKeys: asObject({
- affiliateId: asString,
- apiKey: asString
+ affiliateId: asOptional(asString),
+ apiKey: asOptional(asString)
})
})
@@ -484,6 +484,10 @@
const { apiKey } = apiKeys
const { log } = pluginParams
+ if (apiKey == null) {
+ throw new Error('Missing LetsExchange apiKey')
+ }
+
await fetchCoinCache(apiKey, log)
const tx = asLetsExchangeTx(rawTx)
diff --git a/src/partners/moonpay.ts b/src/partners/moonpay.ts
--- a/src/partners/moonpay.ts
+++ b/src/partners/moonpay.ts
@@ -312,9 +312,14 @@
// Map Moonpay status to Edge status
const status: Status = statusMap[tx.status] ?? 'other'
- // Determine direction based on paymentMethod vs payoutMethod
- // Buy transactions have paymentMethod, sell transactions have payoutMethod
- const direction = tx.paymentMethod != null ? 'buy' : 'sell'
+ // Determine direction based on sell-specific fields.
+ // Sell transactions can also include paymentMethod, so that field alone is insufficient.
+ const direction =
+ tx.quoteCurrency != null ||
+ tx.payoutMethod != null ||
+ tx.depositHash != null
+ ? 'sell'
+ : 'buy'
// Get the payout currency - different field names for buy vs sell
const payoutCurrency = direction === 'buy' ? tx.currency : tx.quoteCurrency
eddy-edge
left a comment
eddy-edge
left a comment
There was a problem hiding this comment.
PR #207 Review Summary: Add Edge Asset Info to Partner Plugins
PR: #207
Author: paullinator (Paul V Puey)
Branch: paul/addEdgeAsset → master
Status: CHANGES_REQUESTED by samholmes
Files changed: 55 | Commits: ~30+
Overview
Adds chain-aware asset metadata (depositChainPluginId, depositEvmChainId, depositTokenId, and payout equivalents) across ~15 partner plugins. Introduces scoped logging (ScopedLog), a new rango partner, semaphore-based concurrency in the query engine, and CouchDB index updates for new fields.
Critical Issues
1. Reviewer Feedback Not Addressed: Async processTx Functions
Risk: Correctness / Performance | Status: UNRESOLVED
samholmes explicitly requested that processTx functions remain synchronous, with cache loading moved outside:
processChangeNowTx→ nowasync(changenow.ts diff L347) — callsawait loadCurrencyCache(log)inside every txprocessSideshiftTx→ nowasync(sideshift.ts diff L242) — same patternprocessBanxaOrders→ nowasync(banxa.ts diff L351)
Reviewer comments:
- sideshift.ts:309: "call the async function outside of the
processSideshiftTxroutine and pass in the cache state as a parameter" - banxa.ts:484: "can we keep processBanxaTx sync to be consistent with other plugins"
- changenow.ts: "Why make this an async function and why not just call this once before the processChangeNowTx calls?"
Impact: Promise overhead on every single transaction; inconsistent with sync plugins like moonpay and letsexchange. The Mutex/loaded-flag guards mitigate redundant fetches but the async pattern is unnecessary.
2. Moonpay: Silent Default to applepay
Risk: Data Correctness | Status: UNRESOLVED
When cardType is undefined for mobile_wallet payment method, it defaults to 'applepay' (moonpay.ts diff ~L396).
Reviewer comment: "Why do we assume applePay by default?" — paullinator replied "will fix" but the code still has the default.
3. Moonpay: Reduced Type Safety from Merged Cleaners
Risk: Correctness | Severity: Medium
The separate asMoonpayTx and asMoonpaySellTx cleaners were merged into a single asMoonpayTx with many optional fields (cryptoTransactionId, currency, walletAddress, depositHash, quoteCurrency, payoutMethod). This means the cleaner no longer validates that buy txs have buy-required fields and sell txs have sell-required fields.
Reviewer comment (moonpay.ts:153): "merging the types ... we then lose the type strictness"
4. Moonpay: All Statuses Now Ingested (Previously Only completed)
Risk: Data Regression | Severity: Medium
Previously, only status === 'completed' transactions were ingested. Now ALL transactions are pushed to standardTxs regardless of status, with unknown statuses silently mapped to 'other'. The statusMap only covers completed and pending.
This is intentional (commit "Include all moonpay txs") but could flood the database with incomplete/failed transactions that were previously excluded.
Addressed Issues (Fixed in Commit 6ed36d2)
The Cursor Bugbot autofix commit addressed several issues:
| Issue | File | Status |
|---|---|---|
console.log in production |
lifi.ts:299 |
Fixed → uses log() |
| Rango raw tokenId addresses | rango.ts:271-283 |
Fixed → uses createTokenId() |
| LetsExchange duplicate status values | letsexchange.ts:36-47 |
Fixed → asMaybe + deduped |
| ChangeNow null vs undefined cache | changenow.ts:344-360 |
Fixed → === null / === undefined |
| Godex cache persistence on failure | godex.ts:132-153 |
Fixed → only caches on success |
NETWORK_FIELDS_AVAILABLE_DATE unused |
letsexchange.ts:117 |
Fixed → integrated into getAssetInfo |
Other Notable Review Items
5. Global Caches Without TTL
Risk: Stale Data | Severity: Low-Medium
Module-level caches in banxa, changenow, sideshift, godex, letsexchange persist for the entire process lifetime. Since the query engine loops infinitely, caches never refresh after first load.
Reviewer comment (banxa.ts:95): "module-level cache persists for the process lifetime ... consider adding TTL-based invalidation"
6. QueryEngine: console.log in datelog Wrapper
Severity: Low
The moved datelog function in queryEngine.ts still uses raw console.log (L39). The old console.log(e) error handler was properly replaced with log.error (L341). The datelog usage is expected since it's a standalone utility.
7. Rates Engine: Not Actually Round-Robin
Severity: Low
Commit message says "round-robin" but implementation uses hardcoded server URLs (rates3.edge.app for v3, rates2.edge.app for v2). No rotation or random selection.
Reviewer comment (ratesEngine.ts:442): "this isn't round-robin as the commit message suggests"
8. disablePartnerQuery Field Undocumented
Severity: Low
New boolean field added to plugin settings but no comment explaining semantics.
Reviewer comment (types.ts:255): "Add comment explaining the semantics"
9. Thorchain: Silent Null Returns
Severity: Low
makeThorchainProcessTx silently returns null for multiple conditions without logging.
Reviewer comment (thorchain.ts:317): "Consider adding debug-level logging to indicate why transactions are being filtered out"
Security Review
- No injection risks: API keys come from config, not user input.
- No secrets in code: API keys passed via
pluginParams.apiKeys. - Cache poisoning: If a ChangeNow/Godex/etc API returns malformed data, it could populate caches with incorrect token mappings. The
godex.tsfix (only cache on success + rethrow) mitigates this for Godex. Other plugins have similar risk but use cleaners for validation. - No new endpoints exposed to external callers.
Recommendation
Do not merge as-is. The following should be addressed before merge:
- Must fix: Make
processChangeNowTx,processSideshiftTxsync per reviewer request — load caches once before the tx processing loop - Must fix: Remove or justify the
applepaydefault in moonpay - Should fix: Document the behavior change of ingesting all moonpay statuses (not just
completed) — confirm this is desired - Should fix: Add comment for
disablePartnerQuerysemantics - Nice to have: Add cache TTL or periodic refresh mechanism
eddy-edge
left a comment
eddy-edge
left a comment
There was a problem hiding this comment.
Thanks for the updates. Requesting changes because there are still blocking items that should be addressed before merge. Inline comments call out specific places.
paullinator
force-pushed
the
paul/addEdgeAsset
branch
from
6ed36d2 to
7c6b78f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Moonpay
cardTypecleaner crashes on 'card' value- Restored
'card'as an allowedcardTypevalue so Moonpay transactions with that value no longer throw in the base cleaner.
- Restored
- ✅ Fixed: LetsExchange
newTxStartcauses false progress regression- Changed
newTxStartto a sentinel and only count new transactions after the first truly new item, preventing false max-limit triggers and progress rollback.
- Changed
Or push these changes by commenting:
@cursor push e8901a36d7
Preview (e8901a36d7)
diff --git a/src/partners/letsexchange.ts b/src/partners/letsexchange.ts
--- a/src/partners/letsexchange.ts
+++ b/src/partners/letsexchange.ts
@@ -381,7 +381,7 @@
let windowStart = new Date(latestIsoDate).getTime() - QUERY_ROLLBACK_MS
const now = Date.now()
let done = false
- let newTxStart: number = 0
+ let newTxStart: number = -1
// Outer loop: iterate over 30-day windows
while (windowStart < now && !done) {
@@ -417,14 +417,14 @@
const standardTx = await processLetsExchangeTx(rawTx, pluginParams)
standardTxs.push(standardTx)
if (standardTx.isoDate > latestIsoDate) {
- if (newTxStart === 0) {
- newTxStart = standardTxs.length
+ if (newTxStart < 0) {
+ newTxStart = standardTxs.length - 1
}
latestIsoDate = standardTx.isoDate
}
}
- const newTxs = standardTxs.length - newTxStart
+ const newTxs = newTxStart < 0 ? 0 : standardTxs.length - newTxStart
log(
`page ${page}/${lastPage} latestIsoDate ${latestIsoDate} newTxs: ${newTxs}/${MAX_NEW_TRANSACTIONS}`
)
diff --git a/src/partners/moonpay.ts b/src/partners/moonpay.ts
--- a/src/partners/moonpay.ts
+++ b/src/partners/moonpay.ts
@@ -142,7 +142,7 @@
baseCurrency: asMoonpayCurrency,
baseCurrencyAmount: asNumber,
baseCurrencyId: asString,
- cardType: asOptional(asValue('apple_pay', 'google_pay')),
+ cardType: asOptional(asValue('apple_pay', 'google_pay', 'card')),
country: asString,
createdAt: asDate,
id: asString,
paullinator
force-pushed
the
paul/addEdgeAsset
branch
from
7c6b78f to
053e532
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 5 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Moonpay
mobile_walletwithout cardType throws on all statuses- For Moonpay
mobile_wallet, unknown or missingcardTypenow returnsnullinstead of throwing so incomplete transactions no longer abort the query.
- For Moonpay
- ✅ Fixed: Bitrefill removed tx filtering risks query crashes
- Bitrefill now wraps each transaction conversion in a per-item try/catch and logs failures so one malformed order cannot crash the full query loop.
- ✅ Fixed: Banxa
processBanxaTxnow async butgetAssetInfocan throw on cache miss- Banxa now tolerates unknown coin/blockchain mappings for non-complete orders by logging and using undefined crypto asset fields, while still throwing for complete orders.
Or push these changes by commenting:
@cursor push 77d6df116b
Preview (77d6df116b)
diff --git a/src/partners/banxa.ts b/src/partners/banxa.ts
--- a/src/partners/banxa.ts
+++ b/src/partners/banxa.ts
@@ -530,7 +530,23 @@
await fetchBanxaCoins(partnerId, apiKeyV2, log)
- const cryptoAssetInfo = getAssetInfo(blockchainCode, coinCode)
+ let cryptoAssetInfo: EdgeAssetInfo = {
+ chainPluginId: undefined,
+ evmChainId: undefined,
+ tokenId: undefined
+ }
+ try {
+ cryptoAssetInfo = getAssetInfo(blockchainCode, coinCode)
+ } catch (e) {
+ if (banxaTx.status === 'complete') {
+ throw e
+ }
+ log.warn(
+ `Skipping crypto asset mapping for ${banxaTx.id} (${banxaTx.status}): ${String(
+ e
+ )}`
+ )
+ }
// For buy transactions: deposit is fiat (no crypto info), payout is crypto
// For sell transactions: deposit is crypto, payout is fiat (no crypto info)
diff --git a/src/partners/bitrefill.ts b/src/partners/bitrefill.ts
--- a/src/partners/bitrefill.ts
+++ b/src/partners/bitrefill.ts
@@ -213,8 +213,12 @@
}
const txs = jsonObj.orders
for (const rawTx of txs) {
- const standardTx = processBitrefillTx(rawTx, pluginParams)
- standardTxs.push(standardTx)
+ try {
+ const standardTx = processBitrefillTx(rawTx, pluginParams)
+ standardTxs.push(standardTx)
+ } catch (e) {
+ log.error(String(e))
+ }
}
if (count > MAX_ITERATIONS) {
diff --git a/src/partners/moonpay.ts b/src/partners/moonpay.ts
--- a/src/partners/moonpay.ts
+++ b/src/partners/moonpay.ts
@@ -418,6 +418,8 @@
paymentMethod = 'applepay'
} else if (tx.cardType === 'google_pay') {
paymentMethod = 'googlepay'
+ } else {
+ return null
}
break
default:
|
All inline threads from this review have been addressed — either via code changes (5 fixup commits: 685dc66, 351d711, 794e0ce, c2f93f8, 0f391d9) or by replies explaining the rationale where we're keeping the current approach (per-tx async |
Review response — all 38 threads addressedSummary of what changed in code vs. what stays: Fixup commits (5)
Already addressed in prior commits
Intentionally kept (with reasoning)
Ready for re-review. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for 3 of the 4 issues found in the latest run.
- ✅ Fixed: Moonpay sell transactions missing payment type information
- Updated Moonpay sell transaction processing to use
getFiatPaymentType(tx)so payment metadata is preserved.
- Updated Moonpay sell transaction processing to use
- ✅ Fixed: ChangeHero module-level cache shared across all partners
- Added a 24-hour TTL with timestamp tracking to refresh the ChangeHero currency cache during long-running processes.
- ✅ Resolved by another fix: ChangeHero currency cache has no TTL refresh
- This was resolved by the same ChangeHero cache TTL refresh implementation added for the related cache staleness issue.
Or push these changes by commenting:
@cursor push 9bef4f3bc8
Preview (9bef4f3bc8)
diff --git a/src/partners/changehero.ts b/src/partners/changehero.ts
--- a/src/partners/changehero.ts
+++ b/src/partners/changehero.ts
@@ -125,6 +125,8 @@
contractAddress: string | null
}
let currencyCache: Map<string, CurrencyInfo> | null = null
+let currencyCacheTimestamp = 0
+const CACHE_TTL_MS = 24 * 60 * 60 * 1000 // 24 hours
function makeCurrencyCacheKey(ticker: string, chain: string): string {
return `${ticker.toUpperCase()}_${chain.toLowerCase()}`
@@ -180,7 +182,12 @@
apiKey: string,
log: ScopedLog
): Promise<void> {
- if (currencyCache != null) return
+ if (
+ currencyCache != null &&
+ Date.now() - currencyCacheTimestamp < CACHE_TTL_MS
+ ) {
+ return
+ }
try {
const response = await retryFetch(API_URL, {
@@ -213,6 +220,7 @@
currencyCache.set(key, info)
}
}
+ currencyCacheTimestamp = Date.now()
log(`Cached ${currencyCache.size} currency entries`)
} catch (e) {
diff --git a/src/partners/moonpay.ts b/src/partners/moonpay.ts
--- a/src/partners/moonpay.ts
+++ b/src/partners/moonpay.ts
@@ -372,7 +372,7 @@
depositAmount: tx.baseCurrencyAmount,
direction,
exchangeType: 'fiat',
- paymentType: null,
+ paymentType: getFiatPaymentType(tx),
payoutTxid: undefined,
payoutAddress: undefined,
payoutCurrency: sellFields.quoteCurrency.code.toUpperCase(),You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: ChangeNow currency cache shared as global singleton
- I replaced the singleton ChangeNow currency cache with a key-scoped cache map (using API key/public key) and threaded the selected cache through transaction processing.
- ✅ Fixed: Exported function
processTxname is too generic- I renamed Moonpay’s exported processor to
processMoonpayTxand updated its internal call sites to match the established partner naming convention.
- I renamed Moonpay’s exported processor to
Or push these changes by commenting:
@cursor push 3854e5320d
Preview (3854e5320d)
diff --git a/src/partners/changenow.ts b/src/partners/changenow.ts
--- a/src/partners/changenow.ts
+++ b/src/partners/changenow.ts
@@ -117,14 +117,13 @@
// Key format: "ticker:network" -> tokenContract
interface CurrencyCache {
currencies: Map<string, string | null> // ticker:network -> tokenContract
- loaded: boolean
}
-
-const currencyCache: CurrencyCache = {
- currencies: new Map(),
- loaded: false
+interface ChangeNowCacheEntry {
+ cache: CurrencyCache
+ timestamp: number
}
-let currencyCacheTimestamp = 0
+const currencyCacheByKey: Map<string, ChangeNowCacheEntry> = new Map()
+const PUBLIC_CACHE_KEY = '__public__'
const CACHE_TTL_MS = 24 * 60 * 60 * 1000 // 24 hours
/**
@@ -133,15 +132,16 @@
async function loadCurrencyCache(
log: ScopedLog,
apiKey?: string
-): Promise<void> {
- if (
- currencyCache.loaded &&
- Date.now() - currencyCacheTimestamp < CACHE_TTL_MS
- ) {
- return
+): Promise<CurrencyCache> {
+ const cacheKey = apiKey ?? PUBLIC_CACHE_KEY
+ const existing = currencyCacheByKey.get(cacheKey)
+ if (existing != null && Date.now() - existing.timestamp < CACHE_TTL_MS) {
+ return existing.cache
}
try {
+ const cache: CurrencyCache = { currencies: new Map() }
+
// The exchange/currencies endpoint doesn't require authentication
const url = 'https://api.changenow.io/v2/exchange/currencies?active=true'
const response = await retryFetch(url, {
@@ -158,7 +158,7 @@
for (const currency of currencies) {
const key = `${currency.ticker.toLowerCase()}:${currency.network.toLowerCase()}`
- currencyCache.currencies.set(key, currency.tokenContract ?? null)
+ cache.currencies.set(key, currency.tokenContract ?? null)
// Also cache by legacyTicker if different from ticker
if (
@@ -166,13 +166,13 @@
currency.legacyTicker !== currency.ticker
) {
const legacyKey = `${currency.legacyTicker.toLowerCase()}:${currency.network.toLowerCase()}`
- currencyCache.currencies.set(legacyKey, currency.tokenContract ?? null)
+ cache.currencies.set(legacyKey, currency.tokenContract ?? null)
}
}
- currencyCache.loaded = true
- currencyCacheTimestamp = Date.now()
+ currencyCacheByKey.set(cacheKey, { cache, timestamp: Date.now() })
log(`Currency cache loaded with ${currencies.length} entries`)
+ return cache
} catch (e) {
log.error(`Error loading currency cache: ${e}`)
throw e
@@ -183,6 +183,7 @@
* Look up contract address from cache
*/
function getContractFromCache(
+ currencyCache: CurrencyCache,
ticker: string,
network: string
): string | null | undefined {
@@ -338,7 +339,11 @@
* Get the Edge asset info for a given network and currency code.
* Uses the cached currency data from the ChangeNow API.
*/
-function getAssetInfo(network: string, currencyCode: string): EdgeAssetInfo {
+function getAssetInfo(
+ currencyCache: CurrencyCache,
+ network: string,
+ currencyCode: string
+): EdgeAssetInfo {
// Map network to pluginId
const chainPluginId = CHANGENOW_NETWORK_TO_PLUGIN_ID[network.toLowerCase()]
if (chainPluginId == null) {
@@ -348,7 +353,11 @@
const evmChainId = EVM_CHAIN_IDS[chainPluginId]
// Look up contract address from cache
- const contractAddress = getContractFromCache(currencyCode, network)
+ const contractAddress = getContractFromCache(
+ currencyCache,
+ currencyCode,
+ network
+ )
// null means native token, undefined means cache miss
if (contractAddress === null) {
@@ -388,7 +397,8 @@
): Promise<StandardTx> {
const { log } = pluginParams
// Load currency cache before processing transactions
- await loadCurrencyCache(log)
+ const { apiKeys } = asChangeNowPluginParams(pluginParams)
+ const currencyCache = await loadCurrencyCache(log, apiKeys.apiKey)
const tx: ChangeNowTx = asChangeNowTx(rawTx)
const date = new Date(
@@ -397,10 +407,18 @@
const timestamp = date.getTime() / 1000
// Get deposit asset info
- const depositAsset = getAssetInfo(tx.payin.network, tx.payin.currency)
+ const depositAsset = getAssetInfo(
+ currencyCache,
+ tx.payin.network,
+ tx.payin.currency
+ )
// Get payout asset info
- const payoutAsset = getAssetInfo(tx.payout.network, tx.payout.currency)
+ const payoutAsset = getAssetInfo(
+ currencyCache,
+ tx.payout.network,
+ tx.payout.currency
+ )
const standardTx: StandardTx = {
status: statusMap[tx.status],
diff --git a/src/partners/moonpay.ts b/src/partners/moonpay.ts
--- a/src/partners/moonpay.ts
+++ b/src/partners/moonpay.ts
@@ -229,7 +229,7 @@
const txs = asMoonpayResult(await result.json())
for (const rawTx of txs) {
- const standardTx = processTx(rawTx, 'sell')
+ const standardTx = processMoonpayTx(rawTx, 'sell')
standardTxs.push(standardTx)
}
@@ -261,7 +261,7 @@
// in bulk update it throws an error for document update conflict because of this.
for (const rawTx of txs) {
- const standardTx = processTx(rawTx, 'buy')
+ const standardTx = processMoonpayTx(rawTx, 'buy')
standardTxs.push(standardTx)
}
if (txs.length > 0) {
@@ -309,7 +309,7 @@
pluginId: 'moonpay'
}
-export function processTx(
+export function processMoonpayTx(
rawTx: unknown,
direction: 'buy' | 'sell'
): StandardTx {You can send follow-ups to the cloud agent here.
- Round robin query all rates servers - Increase batch size and query frequency - Do not write unchanged docs
This properly runs 3 plugin queries in parallel. Prior to this change, 3 plugins would get launced and all run to completion before another 3 are launched.
This file was incorrectly being written to the root directory.
Older deployed versions of Rango improperly saved a full contract address as the token ID. Older deployed versions of Rango improperly saved a full contract address as the token ID.
|
|
||
| // Validate API key | ||
| try { | ||
| await validateApiKey(query.apiKey) |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
validateApiKey(query.apiKey) authenticates that the key exists, but the validated tenant/app identity is not used when querying reportsTransactions. The query still filters only by payoutAddress prefix and date range, so any valid app key can enumerate transaction metadata for other tenants.
Impact: Cross-tenant exposure of payout addresses and transaction details.
| const depositEvmChainId = EVM_CHAIN_IDS[depositChainPluginId] | ||
|
|
||
| // Payout info from last step | ||
| const payoutBlockchain = lastStep.toToken.blockchainData.blockchain |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
This error path logs String(e) after requests that include apiKey and token in the URL query string. Network/fetch error messages frequently include the request URL, which can leak partner credentials into logs.
Impact: Credential disclosure to log readers and downstream logging systems, enabling unauthorized use of the Rango API credentials.
paullinator
force-pushed
the
paul/addEdgeAsset
branch
from
3a10f17 to
af4ee23
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Unnecessary await on synchronous JSON.parse call
- Removed the unnecessary await so JSON.parse is called synchronously after awaiting result.text().
Or push these changes by commenting:
@cursor push 425de4800b
Preview (425de4800b)
diff --git a/src/partners/bitrefill.ts b/src/partners/bitrefill.ts
--- a/src/partners/bitrefill.ts
+++ b/src/partners/bitrefill.ts
@@ -207,7 +207,7 @@
headers
})
resultText = await result.text()
- const json = await JSON.parse(resultText)
+ const json = JSON.parse(resultText)
jsonObj = asBitrefillResult(json)
} catch (e) {
log.error(String(e))You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit af4ee23. Configure here.
| }) | ||
| jsonObj = asBitrefillResult(await result.json()) | ||
| resultText = await result.text() | ||
| const json = await JSON.parse(resultText) |
There was a problem hiding this comment.
Unnecessary await on synchronous JSON.parse call
Low Severity
await JSON.parse(resultText) applies await to a synchronous function. JSON.parse returns a plain value, not a Promise, so the await is misleading and suggests an async operation where there is none.
Reviewed by Cursor Bugbot for commit af4ee23. Configure here.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Make the parser agnostic to the type of asset received for revenue. Change the parser to loop from oldest to newest so we can save progress in case of a failure.
Port over yarn lock file to npm package lock. Update any dependencies that have critical vulnerabilities reported by socket to use updated versions.
paullinator
force-pushed
the
paul/addEdgeAsset
branch
2 times, most recently
from
290dacd to
658fd13
Compare
Add EOS, xRPL, qtum, rvn, Dash, Tezos, and others. Also add card type.
paullinator
force-pushed
the
paul/addEdgeAsset
branch
from
658fd13 to
ce6f360
Compare
5 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
Low Risk
Low risk since changes are limited to repo tooling/config and documentation, but they may impact local install/build behavior if teams still rely on Yarn.
Overview
Updates the repo’s tooling to prefer npm over Yarn: adds
.npmrcwithignore-scripts=true, removes.yarnrc, and updates.gitignoreto stop ignoringpackage-lock.jsonand instead ignoreyarn.lock.Adds a Husky
pre-commithook that runsnpm run precommit, and updatesREADME.mdcommands to usenpm(npm install,npm start,npm run demo, andnpm run build.lib/prepare).Reviewed by Cursor Bugbot for commit ce6f360. Bugbot is set up for automated code reviews on this repo. Configure here.