Conversation
| 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.
8466888 to
ddfaa68
Compare
| 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
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
| } 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)
| 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.
|
|
||
| // 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
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.
| const checkUpdateTx = ( | ||
| oldTx: StandardTx, | ||
| newTx: StandardTx | ||
| ): string[] | undefined => { |
There was a problem hiding this comment.
Why is undefined needed for the return type?
| /** 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.
| 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.
| const depositCurrency = firstStep.fromToken.symbol | ||
| const depositTokenId = firstStep.fromToken.address ?? null | ||
| const payoutCurrency = lastStep.toToken.symbol | ||
| const payoutTokenId = lastStep.toToken.address ?? null |
| 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)
| } | ||
| 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.
Add revolut
Have all plugins just set to undefined for now
Allows backfilling of pluginId/tokenId of all old transactions
Do not error if fiat currency is USD.
- 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.
c81d6fb to
e2cebf6
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 7 potential issues.
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,| 6 | ||
| ) ?? ''} ${payoutAmount}` | ||
| ) | ||
| } |
There was a problem hiding this comment.
Debug console.log left in production code
Medium Severity
A raw console.log statement is used for every completed LiFi transaction instead of the scoped log() function. Every other log call in this file and across all partner plugins uses the scoped logger, which prefixes messages with timestamps and app/partner identifiers. This bypasses the structured logging pattern and will produce unformatted output in production logs.
| 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.
Rango tokenId uses raw addresses without normalization
Medium Severity
The tokenId for Rango assets is set directly from raw contract addresses (firstStep.fromToken.address) without transformation via createTokenId(). All other partner plugins use createTokenId(tokenType, currencyCode, address) which normalizes addresses (e.g., lowercasing and removing the 0x prefix for EVM tokens). This produces inconsistent tokenId values in the database compared to every other plugin.
Additional Locations (1)
| 'overdue', | ||
| 'error', | ||
| 'canceled', | ||
| 'refund' |
There was a problem hiding this comment.
LetsExchange status cleaner lacks fallback for unknown values
Medium Severity
The asLetsExchangeStatus cleaner was changed from using asMaybe(asValue(...), 'other') to a bare asValue(...). Without the asMaybe fallback, any new or unknown status value from the LetsExchange API will cause the transaction cleaner to throw, halting processing for the entire batch instead of gracefully mapping to 'other'.
| 'overdue', | ||
| 'error', | ||
| 'canceled', | ||
| 'refund' |
There was a problem hiding this comment.
LetsExchange asValue has duplicate status entries
Low Severity
The asLetsExchangeStatus definition lists 'overdue' twice (lines 53 and 60) and 'refund' twice (lines 54 and 63). While not a runtime error, duplicates in asValue indicate copy-paste carelessness and make the value list harder to audit for completeness.
| evmChainId, | ||
| tokenId: null | ||
| } | ||
| } |
There was a problem hiding this comment.
ChangeNow conflates missing cache entry with native token
Medium Severity
getContractFromCache explicitly returns undefined for currencies not in the cache (vs. null for native tokens), but getAssetInfo checks contractAddress == null which treats both identically as native tokens with tokenId: null. Tokens missing from the API cache (e.g., delisted or newly added) will silently get incorrect native-token tokenId values.
Additional Locations (1)
| // 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 NETWORK_FIELDS_AVAILABLE_DATE defined
Low Severity
The constant NETWORK_FIELDS_AVAILABLE_DATE is defined but never referenced anywhere in the codebase. Similar date-based cutoff constants in other plugins (like CHAIN_FIELDS_REQUIRED_DATE in changehero.ts and NETWORK_FIELDS_REQUIRED_DATE in exolix.ts) are actively used to gate asset-info lookups for older transactions.
| log.error('Error loading coins cache:', e) | ||
| } | ||
| godexCoinsCache = cache | ||
| return cache |
There was a problem hiding this comment.
Godex coins cache persists incomplete state on API failure
Medium Severity
getGodexCoinsCache sets godexCoinsCache = cache unconditionally after the try/catch block, even when the API fetch fails. On failure, the cache contains only DELISTED_TOKENS entries. Because the guard if (godexCoinsCache != null) return prevents re-fetching, the incomplete cache persists for the entire process lifetime. Every other partner plugin with a similar cache pattern either re-throws the error or only sets the cache inside the success path.



CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
High Risk
High risk because it expands the core
StandardTxschema and updates many partner ingestors plus rates calculation/indexing, so mapping/caching or missing network metadata could change persisted transaction shape and USD valuation behavior.Overview
Transactions are now enriched with chain/token identity.
StandardTxaddsdeposit/payout{ChainPluginId,EvmChainId,TokenId}and partner ingestors are updated to populate these fields (or explicitly set themundefined/null), with substantial new asset/network mapping and contract-address caching in plugins likebanxa,moonpay,lifi,changenow,changehero,exolix,godex,letsexchange,bitrefill, and newrangosupport.Rates and DB setup are updated to support the new fields.
ratesEngineprefers/v3/rateswhen plugin/token metadata is present and falls back to the/v2/exchangeRateAPI otherwise; CouchDB initialization adds Mango indexes fororderIdand the new chain/token fields, andinitDbsnow requirescouchUris(with a localhost default in config) and always uses the Couch pool.Operational/dev tooling adjustments. Logging is standardized via
createScopedLogpassed inPluginParams(includingpagination), Moonpay payment types addrevolut, demo analytics calls switch fromappIdtoapiKey, and npm scripts are renamed fromstart:*tostart.*.Written by Cursor Bugbot for commit e2cebf6. This will update automatically on new commits. Configure here.