Skip to content

Conversation

@paullinator
Copy link
Member

@paullinator paullinator commented Dec 19, 2025

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Description

none

Note

Implements chain-aware asset metadata and expands provider coverage.

  • Add deposit/payoutChainPluginId, deposit/payoutEvmChainId, and deposit/payoutTokenId to StandardTx; populate across partners (Banxa, ChangeNow, ChangeHero, Exolix, Godex, LetsExchange, LiFi, Moonpay, Sideshift, etc.) using chain mappings and token-id creation
  • New rango partner integration; update lifi to include chainId/tokenId and improve amount handling
  • Upgrade providers to newer APIs (e.g., LetsExchange v2, Banxa v2 coins, ChangeNow currencies) and add robust logging via createScopedLog
  • Add CouchDB indexes for orderId and new chain/token fields; require couchUris in initDbs
  • Fix payment-type mapping (e.g., Moonpay Revolut/ACH/PIX), improve pagination/logging, and adjust npm scripts (start.*)

Written by Cursor Bugbot for commit c81d6fb. This will update automatically on new commits. Configure here.


6
) ?? ''} ${payoutAmount}`
)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

): Promise<EdgeAssetInfo> {
if (network == null) {
throw new Error(`Missing network for asset: ${asset}`)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

- 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.
const depositCurrency = firstStep.fromToken.symbol
const depositTokenId = firstStep.fromToken.address ?? null
const payoutCurrency = lastStep.toToken.symbol
const payoutTokenId = lastStep.toToken.address ?? null
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Fix in Cursor Fix in Web

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point.

} else if (tx.cardType === undefined) {
paymentMethod = 'applepay'
}
break
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

This is just an idea to use functions over objects for mappings. More
flexible to encode more about the conversion possibilities.

It results in simpler calls:

```ts
// Determine chainPluginId from networkCode or chainId
  const chainPluginId =
    moonpayNetworkToPluginId(networkCode) ?? reverseEvmChainId(chainIdNum)
```

instead of:

```
  const chainPluginId =
    (networkCode != null
      ? MOONPAY_NETWORK_TO_PLUGIN_ID[networkCode]
      : undefined) ??
    (chainIdNum != null ? REVERSE_EVM_CHAIN_IDS[chainIdNum] : undefined)
 ```

 In general, I think using functions for mapping use-cases results in
 clearer code.

 Just an idea.
Copy link
Collaborator

@samholmes samholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the comment mention that it's used for both buy and sell yet is optional? It wasn't optional before.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we assume applypay by default?

if (chainPluginId != null) {
if (
contractAddress != null &&
contractAddress !== '0x0000000000000000000000000000000000000000'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No provider follows Native Asset Address Convention I hope? (https://eips.ethereum.org/EIPS/eip-7528)

This file was incorrectly being written to the root directory.
paymentMethod = 'googlepay'
} else if (tx.cardType === undefined) {
paymentMethod = 'applepay'
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web


// Determine direction based on paymentMethod vs payoutMethod
// Buy transactions have paymentMethod, sell transactions have payoutMethod
const direction = tx.paymentMethod != null ? 'buy' : 'sell'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Copy link
Collaborator

@samholmes samholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally finished with this. Whew

pluginParams?: PluginParams
): Promise<StandardTx> {
// Load currency cache before processing transactions
await loadCurrencyCache()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make this an async function and why not just call this once before the processChangeNowTx calls?


export async function processChangeNowTx(
rawTx: unknown,
pluginParams?: PluginParams
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is undefined needed for the return type?

Comment on lines +47 to +51
/** 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)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just import this from ./util like before?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point.

const depositCurrency = firstStep.fromToken.symbol
const depositTokenId = firstStep.fromToken.address ?? null
const payoutCurrency = lastStep.toToken.symbol
const payoutTokenId = lastStep.toToken.address ?? null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Adding mappings for documented Moonpay statuses to prevent future data loss
  2. Logging when an unknown status is encountered (currently defaults silently to other)

}
const { swap } = tx.metadata
if (swap?.affiliateAddress !== affiliateAddress) {
return null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants