Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| 6 | ||
| ) ?? ''} ${payoutAmount}` | ||
| ) | ||
| } |
There was a problem hiding this comment.
Bug: Debug console.log left in production code
A console.log statement logs detailed transaction information (orderId, currencies, chain IDs, token IDs, amounts) for every completed Lifi transaction. This appears to be debug/development code that was accidentally left in. It will pollute production logs, potentially leak sensitive transaction data, and cause unnecessary I/O overhead on every completed transaction processed.
| ): 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.
paullinator
force-pushed
the
paul/addEdgeAsset
branch
3 times, most recently
from
8466888 to
ddfaa68
Compare
src/partners/rango.ts
Outdated
| const depositCurrency = firstStep.fromToken.symbol | ||
| const depositTokenId = firstStep.fromToken.address ?? null | ||
| const payoutCurrency = lastStep.toToken.symbol | ||
| const payoutTokenId = lastStep.toToken.address ?? null |
There was a problem hiding this comment.
TokenId uses raw address without proper formatting
The rango.ts partner uses raw contract addresses directly as tokenId values (firstStep.fromToken.address ?? null), while all other partners consistently use createTokenId() to properly format token addresses. For EVM chains, createTokenId() removes the '0x' prefix and lowercases the address. Without this normalization, the tokenId format will be inconsistent with the rest of the codebase, potentially causing token lookup/matching failures.
Additional Locations (1)
| } else if (tx.cardType === undefined) { | ||
| paymentMethod = 'applepay' | ||
| } | ||
| break |
There was a problem hiding this comment.
Unhandled cardType 'card' causes error for mobile_wallet payments
The getFiatPaymentType function doesn't handle the 'card' value for cardType when paymentMethod is 'mobile_wallet'. The cleaner on line 147 allows cardType to be 'apple_pay', 'google_pay', or 'card', but the switch statement only handles the first two plus undefined. If a transaction has paymentMethod: 'mobile_wallet' with cardType: 'card', paymentMethod remains null and the function throws an error.
There was a problem hiding this comment.
ok. need to remove 'card' completely.
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.
| currency: asMoonpayCurrency, | ||
| id: asString, | ||
| // Common amount field (used by both buy and sell) | ||
| quoteCurrencyAmount: asOptional(asNumber), |
There was a problem hiding this comment.
Why does the comment mention that it's used for both buy and sell yet is optional? It wasn't optional before.
There was a problem hiding this comment.
I'm not entirely sure merging the types makes sense because we then lose the type strictness. But w/e
There was a problem hiding this comment.
Good catch optional probably was a mistake. However, we need one single type since the process*Tx. Could be run on either buy or sell.
The better solution is to have one cleaner that can validate whether the transaction is a buy or a sell. And then the independent buyer sell cleaners to validate it afterwards. Basically just check this condition
tx.paymentMethod != null
and if true consider it a buy and use an asMoonpayBuyTx cleaner, good one and a false use asMoonpaySellTx.
I'll refactor to implement this
src/partners/moonpay.ts
Outdated
| } else if (tx.cardType === 'google_pay') { | ||
| paymentMethod = 'googlepay' | ||
| } else if (tx.cardType === undefined) { | ||
| paymentMethod = 'applepay' |
There was a problem hiding this comment.
Why do we assume applypay by default?
| if (chainPluginId != null) { | ||
| if ( | ||
| contractAddress != null && | ||
| contractAddress !== '0x0000000000000000000000000000000000000000' |
There was a problem hiding this comment.
No provider follows Native Asset Address Convention I hope? (https://eips.ethereum.org/EIPS/eip-7528)
There was a problem hiding this comment.
Some do and they are special cased. See Exolix and Let's Exchange. Seems like many providers have their own special nomenclature that we have to sift through.
| paymentMethod = 'googlepay' | ||
| } else if (tx.cardType === undefined) { | ||
| paymentMethod = 'applepay' | ||
| } |
There was a problem hiding this comment.
Unjustified default to Apple Pay when cardType undefined
Medium Severity
When tx.paymentMethod is 'mobile_wallet' and tx.cardType is undefined, the code arbitrarily defaults paymentMethod to 'applepay'. As highlighted by the reviewer's comment "Why do we assume applypay by default?", there's no justification for this assumption. When the data doesn't indicate the payment type, defaulting to Apple Pay could lead to incorrect reporting. Additionally, if cardType is 'card' (a valid type per the cleaner), paymentMethod remains null and the function throws an error.
There was a problem hiding this comment.
This is fine as that is the default for a mobile wallet.
src/partners/moonpay.ts
Outdated
|
|
||
| // Determine direction based on paymentMethod vs payoutMethod | ||
| // Buy transactions have paymentMethod, sell transactions have payoutMethod | ||
| const direction = tx.paymentMethod != null ? 'buy' : 'sell' |
There was a problem hiding this comment.
Sell transactions with paymentMethod are misclassified as buy
High Severity
The direction determination logic tx.paymentMethod != null ? 'buy' : 'sell' can misclassify transactions. The old asMoonpaySellTx cleaner included paymentMethod: asOptional(asString), meaning sell transactions could have this field. The old code hardcoded direction: 'sell' regardless of paymentMethod presence. Now, if a sell transaction has paymentMethod set, it would be incorrectly classified as buy, causing the code to look for tx.currency (buy-specific) instead of tx.quoteCurrency (sell-specific), likely resulting in a "Missing payout currency" error or incorrect data. This relates to the reviewer's concern about losing type strictness when merging the cleaners.
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
Finally finished with this. Whew
src/partners/changenow.ts
Outdated
| pluginParams?: PluginParams | ||
| ): Promise<StandardTx> { | ||
| // Load currency cache before processing transactions | ||
| await loadCurrencyCache() |
There was a problem hiding this comment.
Why make this an async function and why not just call this once before the processChangeNowTx calls?
src/partners/changenow.ts
Outdated
|
|
||
| export async function processChangeNowTx( | ||
| rawTx: unknown, | ||
| pluginParams?: PluginParams |
There was a problem hiding this comment.
Can be removed if loadCurrencyCache is moved out of processChangeNowTx
| const evmChainId = EVM_CHAIN_IDS[chainPluginId] | ||
|
|
||
| // Get contract address from cache | ||
| const coinsCache = await fetchSideshiftCoins() |
There was a problem hiding this comment.
Same suggestion here: call the async function outside of the processSideshiftTx routine and pass in the cache state as a parameter. This prevents turning this method into an async function. I suppose the main motivation for this is to avoid the overhead of promises when the async is only needed. Just makes sense to keep it sync if we can for performance and lesser complexity.
| } | ||
|
|
||
| export function processBanxaTx(rawTx: unknown): StandardTx { | ||
| export async function processBanxaTx( |
There was a problem hiding this comment.
Same here, can we keep processBanxaTx sync to be consistent with other plugins. This is a internal pattern.
src/queryEngine.ts
Outdated
| const checkUpdateTx = ( | ||
| oldTx: StandardTx, | ||
| newTx: StandardTx | ||
| ): string[] | undefined => { |
There was a problem hiding this comment.
Why is undefined needed for the return type?
src/queryEngine.ts
Outdated
| /** Local datelog for engine-level logs not associated with a specific app/partner */ | ||
| const datelog = (...args: unknown[]): void => { | ||
| const date = new Date().toISOString() | ||
| console.log(date, ...args) | ||
| } |
There was a problem hiding this comment.
Can't you just import this from ./util like before?
There was a problem hiding this comment.
I'm happy to hear that the rates server bookmarking is fixed. Not sure what was broken about it. It looks like it has something to do with concurrency, but it's not clear.
src/ratesEngine.ts
Outdated
| currencyB = isFiatCurrency(currencyB) ? `iso:${currencyB}` : currencyB | ||
|
|
||
| const url = `https://rates2.edge.app/v2/exchangeRate?currency_pair=${currencyA}_${currencyB}&date=${hourDate}` | ||
| const server = RATES_SERVERS[Math.floor(Math.random() * RATES_SERVERS.length)] |
There was a problem hiding this comment.
Useful abstraction would be a pickRandomRatesServer function. Optional.
Also this isn't round-robin as the commit message suggests.
| promises.push(promise) | ||
| } | ||
| datelog(partnerStatus) | ||
| await Promise.all(promises) |
There was a problem hiding this comment.
Why are we awaiting all the promises that have already resolved?
I suppose there could be 2 promises unresolved, and we want to wait for those two promises to be resolved before doing 3 more promises for the next app in apps?
This sounds like the thing you're trying to avoid, although limited to one edge case (the end of the plugins array), it seems like it's still possible to be blocked on 3 until they all resolve before doing the next 3.
The only other way I can think of solving this is to remove this await Promise.all(promise) and move the semaphore to outside of the for-loop block for the apps.
src/partners/rango.ts
Outdated
| const depositCurrency = firstStep.fromToken.symbol | ||
| const depositTokenId = firstStep.fromToken.address ?? null | ||
| const payoutCurrency = lastStep.toToken.symbol | ||
| const payoutTokenId = lastStep.toToken.address ?? null |
src/partners/rango.ts
Outdated
| const depositCurrency = firstStep.fromToken.symbol | ||
| const depositTokenId = firstStep.fromToken.address ?? null | ||
| const payoutCurrency = lastStep.toToken.symbol | ||
| const payoutTokenId = lastStep.toToken.address ?? null |
There was a problem hiding this comment.
Inconsistent tokenId format
The tokenId for Rango assets uses raw contract addresses directly without transformation via createTokenId(). Consider using createTokenId(tokenTypes[chainPluginId], currencyCode, address) for consistency with other partner plugins.
| contractAddress: string | null | ||
| pluginId: string | undefined | ||
| } | ||
| let banxaCoinsCache: Map<string, CachedAssetInfo> | null = null |
There was a problem hiding this comment.
Global cache could become stale
The module-level cache banxaCoinsCache persists for the process lifetime. Since the query engine runs in an infinite loop, this cache never refreshes after the first successful fetch. Consider adding TTL-based invalidation or clearing caches periodically.
This pattern also appears in sideshift, changenow, and letsexchange plugins.
| // transactions back to approximately Feb 23, 2022. | ||
| // Transactions before this date may be missing network fields and won't be backfilled. | ||
| // Transactions on or after this date MUST have network fields or will throw. | ||
| const NETWORK_FIELDS_AVAILABLE_DATE = '2022-02-24T00:00:00.000Z' |
There was a problem hiding this comment.
Unused constant
The constant NETWORK_FIELDS_AVAILABLE_DATE is defined but never used in the code. If this date is important for backwards compatibility (e.g., skipping asset info for older transactions), consider implementing that check. Otherwise, this unused constant should be removed.
| }) | ||
| // Map Moonpay status to Edge status | ||
| // Only 'completed' and 'pending' were found in 3 years of API data | ||
| const statusMap: Record<string, Status> = { |
There was a problem hiding this comment.
Limited statusMap coverage
The statusMap only handles completed and pending statuses. While the code comment notes these are the only two statuses found in 3 years of API data, Moonpay docs list additional statuses like failed, waitingAuthorization, etc.
Consider either:
- Adding mappings for documented Moonpay statuses to prevent future data loss
- Logging when an unknown status is encountered (currently defaults silently to
other)
There was a problem hiding this comment.
There's never any data loss, since we save the rawTx, we can always rerun the processTx and backfill the data. Good future todo
| } | ||
| const { swap } = tx.metadata | ||
| if (swap?.affiliateAddress !== affiliateAddress) { | ||
| return null |
There was a problem hiding this comment.
Silent null returns hinder debugging
The makeThorchainProcessTx function silently returns null for several conditions without any logging:
- affiliateAddress mismatch
- status is not
success - no affiliate output found
- source/dest asset match (refund)
- missing output when pools.length === 2
Consider adding debug-level logging to indicate why transactions are being filtered out, especially for less obvious cases like refund detection.
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.
| } | ||
| for (const rawTx of txs) { | ||
| const standardTx = processChangeNowTx(rawTx) | ||
| const standardTx = await processChangeNowTx(rawTx, pluginParams) |
There was a problem hiding this comment.
This is still processing each tx asynchronously and loading dependencies per-tx via processChangeNowTx. Prior review requested keeping this path synchronous by loading cache once at query start and passing it through. Please preload once and make tx processing sync in the hot loop.
There was a problem hiding this comment.
This is necessary since we have upcoming the scripts which rely on a standard process transaction type function and will only be able to pass a raw tx and plugin parameters and cannot pass in a cache.
I will add a comment that describes this. In line with the declaration of the process*Tx
| const evmChainId = EVM_CHAIN_IDS[chainPluginId] | ||
|
|
||
| // Get contract address from cache | ||
| const coinsCache = await fetchSideshiftCoins() |
There was a problem hiding this comment.
fetchSideshiftCoins() is called inside per-tx asset resolution, which makes tx processing async and potentially re-fetches the same data repeatedly. Please move cache fetch outside the tx loop and pass the cache/object into processing helpers so this remains synchronous and predictable.
There was a problem hiding this comment.
This is necessary since we have upcoming the scripts which rely on a standard process transaction type function and will only be able to pass a raw tx and plugin parameters and cannot pass in a cache.
| let standardTx: StandardTx | ||
| try { | ||
| standardTx = processBanxaTx(rawTx) | ||
| standardTx = await processBanxaTx(rawTx, pluginParams) |
There was a problem hiding this comment.
processBanxaOrders still awaits per-tx processing here. Prior feedback requested keeping this processing sync by preparing lookup data once and passing it into tx conversion. Please avoid async work in the inner loop unless strictly necessary.
There was a problem hiding this comment.
This is necessary since we have upcoming the scripts which rely on a standard process transaction type function and will only be able to pass a raw tx and plugin parameters and cannot pass in a cache.
| const standardTx = processMoonpayTx(rawTx) | ||
| standardTxs.push(standardTx) | ||
| } | ||
| const standardTx = processTx(rawTx) |
There was a problem hiding this comment.
We now append every Moonpay buy tx regardless of status. Previously this path only persisted completed txs, which kept analytics datasets cleaner. Please restore explicit status filtering (or document why non-complete records are required and adjust downstream queries/indexes).
There was a problem hiding this comment.
This is correct, as we want to collect all transactions, so we have insight into those that don't complete.
src/partners/moonpay.ts
Outdated
| } else if (tx.cardType === 'google_pay') { | ||
| paymentMethod = 'googlepay' | ||
| } else if (tx.cardType === undefined) { | ||
| paymentMethod = 'applepay' |
There was a problem hiding this comment.
Defaulting unknown mobile_wallet transactions to applepay when cardType is undefined can misclassify data. This should be removed or mapped to a neutral/unknown type instead of forcing applepay.
There was a problem hiding this comment.
Apple Pay is the default mobile wallet, so this should be okay.
- 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.
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,| baseCurrencyAmount: asNumber, | ||
| baseCurrencyId: asString, | ||
| cardType: asOptional(asValue('apple_pay', 'google_pay', 'card')), | ||
| cardType: asOptional(asValue('apple_pay', 'google_pay')), |
There was a problem hiding this comment.
Moonpay cardType cleaner crashes on 'card' value
High Severity
The cardType cleaner was changed from asOptional(asValue('apple_pay', 'google_pay', 'card')) to asOptional(asValue('apple_pay', 'google_pay')), removing 'card'. Since asOptional only handles undefined values (not inner cleaner errors), if the Moonpay API returns cardType: 'card', the asValue cleaner will throw, and asOptional won't catch it. This crashes the entire asMoonpayTxBase cleaner, halting all Moonpay transaction processing for the batch. Using asMaybe instead of asOptional would safely handle unrecognized values.
| let windowStart = new Date(latestIsoDate).getTime() - QUERY_ROLLBACK_MS | ||
| const now = Date.now() | ||
| let done = false | ||
| let newTxStart: number = 0 |
There was a problem hiding this comment.
LetsExchange newTxStart causes false progress regression
High Severity
newTxStart is initialized to 0, so newTxs = standardTxs.length - 0 counts ALL accumulated transactions (old + new) toward the MAX_NEW_TRANSACTIONS limit. When no new transactions are found (common in the rollback window), if >100K old transactions accumulate across windows, the limit triggers. This sets latestIsoDate = windowStartIso (which is earlier than the saved progress), causing the query to regress and potentially loop forever re-fetching the same old transactions.
Additional Locations (2)
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.
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).
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:| paymentMethod = 'applepay' | ||
| } else if (tx.cardType === 'google_pay') { | ||
| paymentMethod = 'googlepay' | ||
| } |
There was a problem hiding this comment.
Moonpay mobile_wallet without cardType throws on all statuses
Medium Severity
The mobile_wallet case in getFiatPaymentType only matches apple_pay or google_pay for cardType. If cardType is undefined (or any other value), paymentMethod remains null and the function throws. Previously, only completed transactions were processed via the asPreMoonpayTx status filter; now ALL statuses are processed, significantly increasing the chance of hitting a mobile_wallet transaction with no cardType, crashing the entire Moonpay query.
There was a problem hiding this comment.
We want the function to throw so that we know that we need to add support for another card type.
| standardTxs.push(standardTx) | ||
| } | ||
| const standardTx = processBitrefillTx(rawTx, pluginParams) | ||
| standardTxs.push(standardTx) |
There was a problem hiding this comment.
Bitrefill removed tx filtering risks query crashes
Medium Severity
The old code filtered transactions, only including those where paymentReceived && !expired && sent. Now all transactions are pushed through processBitrefillTx, which throws on unknown paymentMethod values or missing countryCode mappings. Since this throw is outside any try/catch in queryBitrefill, it crashes the entire partner query, losing all progress for that run.
There was a problem hiding this comment.
This is intentional as we want to make sure that we know all the different payment methods. So if we throw, we can update the code appropriately.
|
|
||
| await fetchBanxaCoins(partnerId, apiKeyV2, log) | ||
|
|
||
| const cryptoAssetInfo = getAssetInfo(blockchainCode, coinCode) |
There was a problem hiding this comment.
Banxa processBanxaTx now async but getAssetInfo can throw on cache miss
Low Severity
getAssetInfo throws when the coin/blockchain combination isn't found in the cache or historical fallback. Since processBanxaTx is called for every transaction (including non-complete ones like pending or expired), encountering an unknown coin will throw and abort the entire Banxa query via the processBanxaOrders re-throw pattern.
There was a problem hiding this comment.
This is intentional as we want to make sure that any unknown chain or asset gets added and not silently failing to be detected. Otherwise we fail to map the asset to internal Edge values and therefore also fail to get exchange rates for it. Better to fail and throw so that we know to update the plugins
4 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
Medium Risk
Medium risk because it changes ingestion/normalization of partner transaction data (chain plugin IDs, EVM chain IDs, token IDs) across multiple providers and adds scripts that bulk-update CouchDB documents, which could affect analytics correctness if mappings/caches are wrong.
Overview
Adds Edge asset metadata (chain + token identification) across partner ingestion. Multiple partner plugins now derive and populate
depositChainPluginId/payoutChainPluginId,*EvmChainId, and*TokenIdusing new network/chain mappings plus provider coin/currency lookups (with 24h in-memory caches and delisted/historical fallbacks), and switches partner logging fromdatelogto scopedpluginParams.log.Introduces new utilities and maintenance scripts. Adds a new
rangopartner plugin, extends DB indexes to support chain/token queries, updates the demo client to useapiKeyinstead ofappIdfor analytics/plugin-id requests, and adds CLI toolsfixTokenIdsandresetUsdValuesfor bulk normalizing tokenIds and forcing USD recomputation. Also updatespaginationto require a scoped logger and modernizestestpartnerto dynamically load any partner plugin.Written by Cursor Bugbot for commit 053e532. This will update automatically on new commits. Configure here.