-
Notifications
You must be signed in to change notification settings - Fork 43
Add xgram support #405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add xgram support #405
Conversation
| 'x-api-key': apiKey | ||
| } | ||
|
|
||
| async function fetchSupportedAssets(): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is non-compliant with API requirements. There should not be a separate call to fetch assets. The single quoting endpoint should directly accept a chain name and contract address. Otherwise network latency could cause the quoting to take too long.
| fromCurrency: xgramCodes.fromCurrencyCode, | ||
| toCurrency: xgramCodes.toCurrencyCode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, these two lines are non-compliant. chain name/code and contract address should be specified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, these two lines are non-compliant. chain name/code and contract address should be specified
@paullinator We don’t specify a contract address when creating exchanges. Each coin has a unique name, which is defined on line 50 of this plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't adhere to our api requirements. Please update the API and resubmit the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API requirements have been taken and fixed in the latest commit.
|
Commit title is poor. Should be "Add Xgram swap support" |
src/swap/central/xgram.ts
Outdated
|
|
||
| if (orderResponseJson.result !== true) { | ||
| const errMsg = String(orderResponseJson.error ?? '') | ||
| throw new Error(`Xgram call returned error message: ${errMsg}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is insufficient. API needs to provide all the errors and properly throw an error code like SwapAboveLimitError. API should return all errors at once and the plugin should surface the errors in this order or priority
- asset not supported
- region restricted
- above/below limit errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API should return all errors at once and the plugin should surface the errors in this order or priority
@paullinator We have added error handling for asset not supported and above/below limit errors, but the region restricted error is not currently handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API must return all error types at once in an array and the code can choose which error to surface. Please update API and re-submit PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API requirements have been taken and fixed in the latest commit.
|
Our workflow is to never merge master into a feature branch. Always rebase your commits on top of master. Please squash this all into one commit for review. |
c5aaf5a to
62d8e3f
Compare
|
cursor review verbose=true |
|
Bugbot request id: serverGenReqId_e478b76d-79f0-4afa-828f-74563cb5b902 |
src/swap/central/xgram.ts
Outdated
|
|
||
| async function swapExchange(isSelling: boolean): Promise<SwapOrder> { | ||
| const buyUrl = `fromCcy=${xgramCodes.toCurrencyCode}&toCcy=${xgramCodes.fromMainnetCode}` | ||
| const sellUrl = `fromCcy=${xgramCodes.fromMainnetCode}&toCcy=${xgramCodes.toCurrencyCode}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Rate lookup uses wrong code types causing token swap failures
The rate lookup URL construction incorrectly uses fromMainnetCode (chain/network code) where currency/token codes are expected. In sellUrl, fromCcy uses fromMainnetCode instead of fromCurrencyCode, and in buyUrl, toCcy uses fromMainnetCode instead of a currency code. This causes token swaps to fail because the API receives the chain code (e.g., "ETH") instead of the actual token code (e.g., "USDT"). Native coin swaps may work by coincidence since mainnet and currency codes are often identical, but all token swaps will fail or return incorrect rates.
src/swap/central/xgram.ts
Outdated
|
|
||
| if (marketRangeResponseJson.error === 'Pair not found') { | ||
| throw new SwapCurrencyError(swapInfo, request) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Cleaner throws before "Pair not found" error can be handled
The asMarketRange cleaner is called before checking for the "Pair not found" error. When the API returns an unsupported pair error, the response likely won't include minFromCcyAmount and maxFromCcyAmount fields. The cleaner expects these as required numbers and will throw an unhandled cleaner error before the code reaches the "Pair not found" check on line 275. This causes users to see a cryptic cleaner error instead of the proper SwapCurrencyError for unsupported currency pairs. The "Pair not found" check needs to happen before calling asMarketRange.
|
cursor review verbose=true Use all *.md files in the https://github.com/EdgeApp/edge-conventions repo to determine coding and PR commit style. Use any *.md files in each repo to find docs and coding conventions to adhere to when doing reviews. For this repo use this file to determine if the PR adheres to API requirements https://github.com/EdgeApp/edge-exchange-plugins/blob/paul/apiReq/docs/API_REQUIREMENTS.md |
src/swap/central/xgram.ts
Outdated
| isSelling ? undefined : 'to' | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing throw allows fallthrough when result is false
When marketRangeResponseJson.result is false but none of the specific error conditions are met (not "Pair not found", not below min, not above max), execution falls through the if block and continues to createOrder. This allows attempting to create an order even though the rate API indicated failure, potentially leading to unexpected behavior or invalid orders being submitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see all comments. and re-review
https://github.com/EdgeApp/edge-exchange-plugins/blob/paul/apiReq/docs/API_REQUIREMENTS.md
62d8e3f to
c9987a7
Compare
|
@paullinator All API requirements have been taken and fixed in the latest commit. |
paullinator
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Requirements Review
This PR does not yet comply with the API Requirements document. Please address the following before this can be merged:
Section 1: Chain and Token Identification
The current implementation relies on:
- A separate
fetchSupportedAssets()call tolist-currency-optionsendpoint - Currency codes/ticker symbols (
fromCcy,toCcy) for asset identification
Required changes:
- The API must support quoting using chain identifiers (e.g., 'ethereum', 'polygon') and token identifiers (contract addresses) directly
- Remove the need for a pre-fetch call to list supported assets
- For EVM chains, support
chainIdparameter for generic EVM chain identification
What's working well
- Error handling (Section 3) appears compliant - errors are returned in a structured array with proper priority handling for REGION_UNSUPPORTED, CURRENCY_UNSUPPORTED, BELOW_LIMIT, and ABOVE_LIMIT
Please refer to the inline comments for specific lines that need attention.
| async function fetchSupportedAssets(): Promise<void> { | ||
| if (lastUpdated > Date.now() - EXPIRATION && lastUpdated !== 0) return | ||
| try { | ||
| const response = await fetchCors(`${uri}list-currency-options`, { | ||
| headers | ||
| }) | ||
|
|
||
| const json = await response.json() | ||
| const jsonArr = Object.entries(json).map(([key, data]) => ({ | ||
| ...(data as object), | ||
| coinName: key | ||
| })) | ||
| const assets = asXgramAssets(jsonArr) | ||
| const chaincodeArray = Object.values(MAINNET_CODE_TRANSCRIPTION) | ||
| .filter((v): v is string => v != null) | ||
| .map(v => v.toLowerCase()) | ||
| const out: ChainCodeTickerMap = new Map() | ||
|
|
||
| for (const asset of assets) { | ||
| const chain = asset.network.toLowerCase() | ||
| if (chaincodeArray.includes(chain)) { | ||
| const tokenCodes = out.get(chain) ?? [] | ||
|
|
||
| tokenCodes.push({ | ||
| tokenCode: asset.coinName, | ||
| contractAddress: | ||
| asset.contract != null && asset.contract !== '' | ||
| ? asset.contract | ||
| : null | ||
| }) | ||
| out.set(chain, tokenCodes) | ||
| } | ||
| } | ||
|
|
||
| chainCodeTickerMap = out | ||
| lastUpdated = Date.now() | ||
| } catch (e) { | ||
| log.warn('Xgram: Error updating supported assets', e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-compliant with API Requirements - Section 1: Chain and Token Identification
This fetchSupportedAssets() function makes a separate network call to list-currency-options to fetch supported assets. Per API_REQUIREMENTS.md:
"Providing a separate endpoint that lists assets is NOT sufficient for this need. This is because separate API calls add significant latency."
The API should support quoting directly using chain identifiers and token contract addresses without requiring a pre-fetch of supported assets. This entire function should be removed in favor of passing chain/token identifiers directly to the quote endpoint.
| const orderBody = { | ||
| fromCurrency: xgramCodes.fromCurrencyCode, | ||
| toCurrency: xgramCodes.toCurrencyCode, | ||
| fromAmount: isSelling ? largeDenomAmount : '', | ||
| toAmount: isSelling ? '' : largeDenomAmount, | ||
| address: toAddress, | ||
| refundAddress: fromAddress | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-compliant with API Requirements - Section 1: Chain and Token Identification
The orderBody uses xgramCodes.fromCurrencyCode and xgramCodes.toCurrencyCode (currency codes) instead of the required chain network identifiers and token contract addresses.
Per API_REQUIREMENTS.md:
"Quotes must be requestable using chain identifiers (e.g., 'ethereum', 'polygon') and token identifiers (e.g., contract address '0x...'), not just ticker symbols."
The API needs to accept parameters like fromChain, fromTokenAddress, toChain, toTokenAddress to uniquely identify assets.
| const qs = new URLSearchParams({ | ||
| toAddress: String(toAddress), | ||
| refundAddress: String(fromAddress), | ||
| fromCcy: orderBody.fromCurrency, | ||
| toCcy: orderBody.toCurrency, | ||
| ccyAmount: largeDenomAmount, | ||
| type: swapType | ||
| }).toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-compliant with API Requirements - Section 1: Chain and Token Identification
The query parameters use fromCcy and toCcy which are currency codes/ticker symbols. This is ambiguous - the same ticker (e.g., "USDT") exists on multiple chains.
Per API_REQUIREMENTS.md:
"It is NOT sufficient to provide only a ticker to identify an asset. This is NOT unique since the same ticker can be used by tokens on multiple chains."
The API must support distinct parameters for chain network and token contract address to unambiguously identify assets.
| ): Promise<EdgeSwapQuote> { | ||
| const request = convertRequest(req) | ||
|
|
||
| await fetchSupportedAssets() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-compliant with API Requirements - Section 1: Chain and Token Identification
This call to fetchSupportedAssets() adds latency before every quote request. Per the API requirements, the quote endpoint should accept chain/token identifiers directly, eliminating the need for this pre-fetch step.
Remove this call once the API is updated to support direct chain/token identification in quote requests.
paullinator
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required Fixes: Data Validation with Cleaners
All network responses must be validated using cleaners, and the cleaned data must be used throughout the code.
Issues Found:
-
Incomplete cleaner definition -
asTemplateQuoteis missingccyAmountFromandexpiresAtfields that are accessed from the API response -
Raw JSON used instead of cleaned data - The code calls
asTemplateQuoteReply()but then accesses fields from the raworderResponseJsoninstead of the validatedquoteReplyresult
Why This Matters:
- Unvalidated API responses can cause runtime crashes if the API changes or returns unexpected data
- Type safety is lost when bypassing cleaners
- This is a standard requirement for all Edge plugins
Please update the cleaner to include all accessed fields, and use the cleaned result for all field access.
| const asTemplateQuote = asObject({ | ||
| ccyAmountToExpected: asNumber, | ||
| depositAddress: asString, | ||
| depositTag: asString, | ||
| id: asString, | ||
| result: asBoolean | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required Fix: Incomplete cleaner definition
The asTemplateQuote cleaner is missing fields that are accessed from the API response:
ccyAmountFrom- Used on line 230 but not defined in the cleanerexpiresAt- Used on line 220 but not defined in the cleaner
All fields accessed from API responses must be defined in a cleaner to validate the data type and presence. Add the missing fields:
const asTemplateQuote = asObject({
ccyAmountFrom: asOptional(asNumber),
ccyAmountToExpected: asNumber,
depositAddress: asString,
depositTag: asString,
expiresAt: asOptional(asString),
id: asString,
result: asBoolean
})| let parsedValidUntil: Date | null = null | ||
| if (orderResponseJson.expiresAt != null) { | ||
| const maybe = new Date(orderResponseJson.expiresAt) | ||
| if (!Number.isNaN(maybe.getTime())) parsedValidUntil = maybe | ||
| } | ||
|
|
||
| return { | ||
| id: orderResponseJson.id, | ||
| validUntil: parsedValidUntil, | ||
| fromAmount: isSelling | ||
| ? orderBody.fromAmount | ||
| : orderResponseJson.ccyAmountFrom, | ||
| toAmount: isSelling | ||
| ? orderResponseJson.ccyAmountToExpected | ||
| : orderBody.toAmount, | ||
| payinAddress: orderResponseJson.depositAddress, | ||
| payinExtraId: orderResponseJson.depositTag | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required Fix: Use cleaned data instead of raw JSON
This code block accesses fields directly from orderResponseJson (the raw, unvalidated JSON) instead of using the cleaned quoteReply data.
The cleaner asTemplateQuoteReply is called on line 182 but its result is only used for error checking. The success case should use the cleaned data to ensure type safety.
Current (incorrect):
const quoteReply = asTemplateQuoteReply(orderResponseJson)
// ... error handling uses quoteReply ...
// ... but success case uses raw orderResponseJson:
id: orderResponseJson.id,
toAmount: orderResponseJson.ccyAmountToExpected,Required fix:
After validating with the cleaner, use the cleaned result for all field access:
const quoteReply = asTemplateQuoteReply(orderResponseJson)
if ('errors' in quoteReply) {
// ... error handling ...
}
// Use quoteReply (now known to be the quote type) for all fields:
return {
id: quoteReply.id,
validUntil: quoteReply.expiresAt != null ? new Date(quoteReply.expiresAt) : null,
fromAmount: isSelling ? orderBody.fromAmount : String(quoteReply.ccyAmountFrom),
toAmount: isSelling ? String(quoteReply.ccyAmountToExpected) : orderBody.toAmount,
payinAddress: quoteReply.depositAddress,
payinExtraId: quoteReply.depositTag
}This ensures all API data is validated before use, preventing runtime errors from unexpected API responses.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
Adds Xgram swap provider and new helpers for denomination/native conversions and contract-address lookups.
src/swap/central/xgram.tsintegrating Xgram API to fetch rates, create orders, enforce min/max limits, handle unsupported pairs, set memos/tags, and build spend actions with expiration handling.xgraminsrc/index.tsplugins map.src/util/swapHelpers.ts):nativeToDenomination,denominationToNative,getCurrencyMultiplier, andgetContractAddressesplus required math imports.Written by Cursor Bugbot for commit 62d8e3f. This will update automatically on new commits. Configure here.