-
Notifications
You must be signed in to change notification settings - Fork 49
feat: finish implementation for near intents revenue tracking #1230
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: 11435_graceful_errors
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe changes introduce nearIntents as a new provider for affiliate revenue collection, replace Promise.all with Promise.allSettled to enable resilient partial failure handling, track and report failed providers in responses, and extend the Fees and AffiliateRevenueResponse data structures with additional asset metadata fields (assetId, chainId, service, timestamp, txHash). Changes
Sequence DiagramsequenceDiagram
participant Client
participant AffiliateRevenue as Affiliate Revenue<br/>Aggregator
participant Providers as Provider<br/>(bebop, chainflip, etc.)
participant ErrorHandler as Error<br/>Handler
Note over Client,ErrorHandler: New Flow: Promise.allSettled + Partial Success
Client->>AffiliateRevenue: GET /affiliate-revenue
AffiliateRevenue->>AffiliateRevenue: Add nearIntents to providers
par Collect All Providers
AffiliateRevenue->>Providers: getFees (Promise.allSettled)
Note right of Providers: Now includes nearIntents
alt Provider Success
Providers-->>AffiliateRevenue: fees data
else Provider Failure
Providers-->>ErrorHandler: error
ErrorHandler->>AffiliateRevenue: formatError()
AffiliateRevenue->>AffiliateRevenue: Track in failedProviders
end
end
Note over AffiliateRevenue: Resolve all promises<br/>(not stopping on first error)
AffiliateRevenue->>AffiliateRevenue: Aggregate successful fees<br/>Track failures
AffiliateRevenue->>AffiliateRevenue: Extend Fees with:<br/>assetId, chainId, service,<br/>timestamp, txHash
AffiliateRevenue-->>Client: AffiliateRevenueResponse<br/>(totalUsd, byService,<br/>failedProviders)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
node/proxy/api/src/affiliateRevenue/nearIntents.ts (3)
51-51: Fallback toslip44:0(Bitcoin) may produce incorrect asset IDs.When
SLIP44_BY_NETWORK[network]is undefined, defaulting to0(Bitcoin's slip44) could silently produce incorrect asset identifiers for networks not in the mapping. Consider either:
- Returning
nullto skip unknown networks (consistent with line 46/56)- Throwing/logging when an unmapped network has a token address
- return { chainId, assetId: `${chainId}/slip44:${SLIP44_BY_NETWORK[network] ?? 0}` } + const slip44 = SLIP44_BY_NETWORK[network] + if (slip44 === undefined) return null + return { chainId, assetId: `${chainId}/slip44:${slip44}` }
62-63: Same fallback issue for native assets.This has the same concern as line 51 - falling back to
slip44:0for unknown networks could produce incorrect asset IDs.- const slip44 = SLIP44_BY_NETWORK[network] ?? 0 - return { chainId, assetId: `${chainId}/slip44:${slip44}` } + const slip44 = SLIP44_BY_NETWORK[network] + if (slip44 === undefined) return null + return { chainId, assetId: `${chainId}/slip44:${slip44}` }
106-119: Consider adding a timeout to the axios request.The axios call has no timeout configured. If the Near Intents API becomes slow or unresponsive, this could cause the request to hang indefinitely, potentially blocking the entire revenue aggregation.
const { data } = (await axios.get<TransactionsResponse>( 'https://explorer.near-intents.org/api/v0/transactions-pages', { params: { referral: 'shapeshift', page, perPage: 100, statuses: 'SUCCESS', startTimestampUnix: startTimestamp, endTimestampUnix: endTimestamp, }, headers: { Authorization: `Bearer ${NEAR_INTENTS_API_KEY}` }, + timeout: 30000, } )) as { data: TransactionsResponse }node/proxy/api/src/affiliateRevenue/index.ts (1)
11-11: Index-based provider mapping is fragile.The
providerNamesarray must stay in sync with the order ofgetFeescalls inPromise.allSettled. If someone adds or reorders providers, the mapping will silently break, attributing errors to wrong providers.Consider using a more robust approach:
-const providerNames: Service[] = ['zrx', 'bebop', 'thorchain', 'mayachain', 'chainflip', 'portals', 'nearintents'] +const providers: { name: Service; getFees: (start: number, end: number) => Promise<Fees[]> }[] = [ + { name: 'zrx', getFees: zrx.getFees }, + { name: 'bebop', getFees: bebop.getFees }, + { name: 'thorchain', getFees: thorchain.getFees }, + { name: 'mayachain', getFees: mayachain.getFees }, + { name: 'chainflip', getFees: chainflip.getFees }, + { name: 'portals', getFees: portals.getFees }, + { name: 'nearintents', getFees: nearintents.getFees }, +]Then iterate:
const results = await Promise.allSettled( providers.map(p => p.getFees(startTimestamp, endTimestamp)) ) results.forEach((result, index) => { const provider = providers[index].name // ... })Also applies to: 41-59
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
node/proxy/api/src/affiliateRevenue/index.ts(3 hunks)node/proxy/api/src/affiliateRevenue/nearIntents.ts(2 hunks)node/proxy/api/src/models.ts(1 hunks)node/proxy/api/src/swagger.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
node/proxy/api/src/affiliateRevenue/index.ts (1)
node/proxy/api/src/models.ts (1)
Service(6-6)
🪛 Checkov (3.2.334)
node/proxy/api/src/swagger.json
[medium] 131-135: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check
🔇 Additional comments (5)
node/proxy/api/src/models.ts (1)
10-10: LGTM!The
failedProvidersfield is properly typed usingService[], maintaining type safety and consistency with the rest of the response structure.node/proxy/api/src/swagger.json (1)
114-136: Schema additions are consistent with TypeScript types.The
Serviceenum andfailedProvidersarray are correctly defined and align withmodels.ts. The array is naturally bounded by the number of providers (currently 7), so while addingmaxItems: 7could satisfy the static analysis hint (CKV_OPENAPI_21), it's a minor consideration.node/proxy/api/src/affiliateRevenue/nearIntents.ts (1)
129-129: Empty string fallback fortxHashmay cause data quality issues.When
originChainTxHashesis empty,txHashbecomes an empty string. Verify this is acceptable for downstream consumers of theFeesdata structure, or consider logging a warning similar to the asset parsing failure.node/proxy/api/src/affiliateRevenue/index.ts (2)
13-24: Good error formatting implementation.The
formatErrorhelper properly handles axios-specific errors, genericErrorinstances, and unknown error types. This provides consistent and useful error messages in logs.
51-59: Good use ofPromise.allSettledfor graceful degradation.This approach ensures that failures in individual providers don't break the entire revenue aggregation. Failed providers are tracked and reported, while successful results are still processed and returned.
Summary
This finishes the implemention for near intents in our revenue tracking tool
Testing
If you want to properly test this locally:
Install dependencies (from repo root)
cd /home/sean/Repos/shapeshift-unchained
yarn install
Copy and configure env
cp node/proxy/sample.env node/proxy/.env
// Edit node/proxy/.env with real API keys (BEBOP_API_KEY, ZRX_API_KEY, etc.)
Start reverse proxy (from repo root)
docker-compose up -d reverse-proxy
Start proxy API
cd node/proxy
docker-compose up
Test endpoint (in another terminal)
curl "http://api.proxy.localhost/api/v1/affiliate/revenue?startTimestamp=$(( $(date +%s) - 604800 ))&endTimestamp=$(date +%s)"This will fail right now for nearintents until we get an explorer API key
Screenshots
Near intents errors right now until we get an explorer API key. But it doesn't crash and gives a proper error