-
Notifications
You must be signed in to change notification settings - Fork 1
feat: dune ids as env vars #196
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
Conversation
📝 WalkthroughWalkthroughThe changes migrate static configuration values from hardcoded constants to dynamic configuration functions that read environment variables. Two services are refactored: AffiliateProgramExportService and AffiliateStatsService, with configuration constants moved to config.ts modules featuring environment variable validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
| @@ -0,0 +1,27 @@ | |||
| export function getDuneQueryIds(): { | |||
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.
Just wondering if validation should be re-run every time we need to read the value, or if it could run only once at server startup. I've usually wrapped env variable in a class with some schema validation or something similar, but a more naive approach might be:
const duneQueryIds = getDuneQueryIds();
export const DUNE_QUERY_ID_TRADER_STATS = duneQueryIds.traderStats;
export const DUNE_QUERY_ID_AFFILIATE_STATS = duneQueryIds.affiliateStats;
The same applies to getAffiliateProgramTableName.
Minor issue, so I'm approving anyway.
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.
Yes, of course, the problem is that this server is also run by other teams, e.g. Aave so the behavior I wanted to achieve was to error at runtime, when hitting the endpoints, and not at start-up.
This is needed because we will have 2 sets of dashboards for each environment
Summary by CodeRabbit