feat: add graceful error handling to affiliate revenue endpoint #1229
Conversation
📝 WalkthroughWalkthroughThe affiliate revenue aggregator is enhanced to use Changes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
node/proxy/api/src/swagger.json (1)
131-145: Consider addingmaxItemsconstraint to the array.The
failedProvidersfield is correctly defined and marked as required. The array is naturally bounded by the number of services (7), but adding an explicitmaxItemsconstraint would make this clear in the API contract.Based on the static analysis hint, you could apply this diff:
"failedProviders": { "items": { "$ref": "#/components/schemas/Service" }, - "type": "array" + "type": "array", + "maxItems": 7 },
📜 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 (3)
node/proxy/api/src/affiliateRevenue/index.ts(4 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 (4)
node/proxy/api/src/models.ts (1)
10-10: LGTM!The addition of
failedProvidersfield correctly tracks which services failed, enabling the UI to display appropriate indicators.node/proxy/api/src/affiliateRevenue/index.ts (2)
24-43: LGTM!The switch to
Promise.allSettledcorrectly implements graceful error handling. Failed providers are properly tracked and logged, while successful results are aggregated.
57-61: LGTM!The return object correctly includes
failedProviders, matching the updatedAffiliateRevenueResponseinterface.node/proxy/api/src/swagger.json (1)
114-125: LGTM!The
Serviceenum schema correctly reflects all defined services and aligns with the TypeScript type definition.
| import * as portals from './portals' | ||
| import { AffiliateRevenueResponse, Service, services } from '../models' | ||
|
|
||
| const providerNames: Service[] = ['zrx', 'bebop', 'thorchain', 'mayachain', 'chainflip', 'portals'] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to eliminate fragile order-dependent mapping.
The providerNames array is tightly coupled to the order of promises in Promise.allSettled (lines 26-33). If a developer adds or reorders providers, they must remember to update both locations identically, or failures will be attributed to the wrong provider.
Apply this diff to use a more maintainable structure:
-const providerNames: Service[] = ['zrx', 'bebop', 'thorchain', 'mayachain', 'chainflip', 'portals']
+const providers = [
+ { name: 'zrx' as Service, getFees: zrx.getFees },
+ { name: 'bebop' as Service, getFees: bebop.getFees },
+ { name: 'thorchain' as Service, getFees: thorchain.getFees },
+ { name: 'mayachain' as Service, getFees: mayachain.getFees },
+ { name: 'chainflip' as Service, getFees: chainflip.getFees },
+ { name: 'portals' as Service, getFees: portals.getFees },
+]Then update the Promise.allSettled call:
- const results = await Promise.allSettled([
- zrx.getFees(startTimestamp, endTimestamp),
- bebop.getFees(startTimestamp, endTimestamp),
- thorchain.getFees(startTimestamp, endTimestamp),
- mayachain.getFees(startTimestamp, endTimestamp),
- chainflip.getFees(startTimestamp, endTimestamp),
- portals.getFees(startTimestamp, endTimestamp),
- ])
+ const results = await Promise.allSettled(
+ providers.map(p => p.getFees(startTimestamp, endTimestamp))
+ )
- results.forEach((result, index) => {
+ results.forEach((result, index) => {
+ const provider = providers[index].name
if (result.status === 'fulfilled') {
fees.push(...result.value)
} else {
- const provider = providerNames[index]
failedProviders.push(provider)
console.error(`[AffiliateRevenue] ${provider} failed:`, result.reason)
}
})Committable suggestion skipped: line range outside the PR's diff.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the affiliateRevenue directory structure
cd node/proxy/api/src/affiliateRevenue 2>/dev/null && find . -type f -name "*.ts" | head -20 || echo "Directory not found or no TypeScript files"Repository: shapeshift/unchained
Length of output: 184
🏁 Script executed:
# Check for models.ts to see the Service enum
find . -name "models.ts" -path "*/affiliateRevenue/*" -exec cat {} \; 2>/dev/null || find . -name "models.ts" | grep -i affiliate | head -1 | xargs cat 2>/dev/null || echo "models.ts not found"Repository: shapeshift/unchained
Length of output: 46
🏁 Script executed:
# Check for nearintents directory or files
fd -t f "nearintents" node/proxy/api/src/affiliateRevenue/ 2>/dev/null || echo "No nearintents files found in affiliateRevenue"Repository: shapeshift/unchained
Length of output: 114
🏁 Script executed:
# Examine the index.ts file to see the actual providerNames and Promise.allSettled implementation
find . -path "*/affiliateRevenue/index.ts" -exec head -60 {} \; 2>/dev/null || echo "index.ts not found"Repository: shapeshift/unchained
Length of output: 1980
🏁 Script executed:
find . -path "*/affiliateRevenue/../models.ts" -o -path "*/affiliateRevenue/models.ts" 2>/dev/null | xargs grep -A 20 "Service" | head -40Repository: shapeshift/unchained
Length of output: 46
🏁 Script executed:
# Alternative search for models.ts
find . -name "models.ts" | xargs grep -B 2 -A 15 "type Service\|enum Service\|const services" 2>/dev/null | head -50Repository: shapeshift/unchained
Length of output: 671
Add the missing nearintents provider to the affiliate revenue collection.
The nearintents service is defined in the Service enum but disconnected from the revenue collection flow. Import the nearIntents module, add it to the providerNames array, and include its getFees() call in the Promise.allSettled() array.
🤖 Prompt for AI Agents
In node/proxy/api/src/affiliateRevenue/index.ts around line 9, the providerNames
array is missing the 'nearintents' service and its module isn't imported or
invoked; import the nearIntents module at the top, add 'nearintents' to the
providerNames: Service[] array, and include a call to nearIntents.getFees() in
the Promise.allSettled() array so the nearintents provider participates in the
affiliate revenue collection flow.
This adds graceful error handling to the affiliate revenue endpoint. Ensuring that if one swapper api fails we at least get partial data. It also will indicate in the API response which failed, ensuring we can show some kind of indicator in the UI.
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)"Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.