feat: add dune support and export hooks #145
Conversation
📝 WalkthroughWalkthroughAdds a Dune-backed Hooks feature: new Dune repository and implementation, HooksService and type guard, DI wiring gated by DUNE feature flag, a new Changes
Sequence Diagram(s)sequenceDiagram
participant API as API Route
participant HooksService as HooksServiceImpl
participant Repo as DuneRepositoryImpl
participant Dune as Dune API
API->>HooksService: getHooks({blockchain, period, maxWaitTimeMs, limit, offset})
HooksService->>Repo: executeQuery({queryId, parameters})
Repo->>Dune: POST /query/{id}/execute
Dune-->>Repo: { execution_id }
HooksService->>Repo: waitForExecution({executionId, maxWaitTimeMs, typeAssertion})
loop poll
Repo->>Dune: GET /execution/{executionId}/results
Dune-->>Repo: execution result (pending/finished)
end
Repo-->>HooksService: validated result rows
HooksService-->>API: HookData[]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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
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: 7
🧹 Nitpick comments (4)
libs/services/src/HooksService/HooksServiceImpl.ts (1)
20-41: Consider adding service-level error handling.While the implementation is correct, wrapping errors would provide better context for debugging.
async getHooks(params: GetHooksParams): Promise<HookData[]> { const { blockchain, period, maxWaitTimeMs } = params; - // Execute the query with parameters - const execution = await this.duneRepository.executeQuery({ - queryId: DEFAULT_QUERY_ID, - parameters: { - blockchain, - period, - }, - }); - - // Wait for execution to complete with type assertion - const result = await this.duneRepository.waitForExecution<HookData>({ - executionId: execution.execution_id, - typeAssertion: isHookData, - maxWaitTimeMs, - }); - - // The data is already typed as HookData from the generic repository - return result.result.rows; + try { + // Execute the query with parameters + const execution = await this.duneRepository.executeQuery({ + queryId: DEFAULT_QUERY_ID, + parameters: { + blockchain, + period, + }, + }); + + // Wait for execution to complete with type assertion + const result = await this.duneRepository.waitForExecution<HookData>({ + executionId: execution.execution_id, + typeAssertion: isHookData, + maxWaitTimeMs, + }); + + // The data is already typed as HookData from the generic repository + return result.result.rows; + } catch (error) { + throw new Error(`Failed to fetch hooks data: ${error.message}`); + } }apps/api/src/app/routes/hooks.ts (2)
30-47: Avoid duplicating the HookData interface.The response interface duplicates the
HookDatatype definition. Consider importing and reusing it.+import { HookData } from '@cowprotocol/services'; interface HooksResponse { - hooks: Array<{ - environment: string; - block_time: string; - is_bridging: boolean; - success: boolean; - app_code: string; - destination_chain_id: number | null; - destination_token_address: string | null; - hook_type: string; - app_id: string | null; - target: string; - gas_limit: number; - app_hash: string; - tx_hash: string; - }>; + hooks: HookData[]; count: number; }
164-197: Extract duplicate response schema.The response schema is identical to the
/hooksendpoint. Consider extracting it to reduce duplication.+const hooksResponseSchema = { + type: 'object', + properties: { + hooks: { + type: 'array', + items: { + type: 'object', + properties: { + environment: { type: 'string' }, + block_time: { type: 'string' }, + is_bridging: { type: 'boolean' }, + success: { type: 'boolean' }, + app_code: { type: 'string' }, + destination_chain_id: { type: ['number', 'null'] }, + destination_token_address: { type: ['string', 'null'] }, + hook_type: { type: 'string' }, + app_id: { type: ['string', 'null'] }, + target: { type: 'string' }, + gas_limit: { type: 'number' }, + app_hash: { type: 'string' }, + tx_hash: { type: 'string' }, + }, + }, + }, + count: { type: 'number' }, + }, +}; // Then use it in both endpoints: response: { - 200: { /* current schema */ } + 200: hooksResponseSchema }libs/services/src/HooksService/HooksService.ts (1)
61-62: Track the temporary PoC method removal.The TODO indicates
getLatestHooksis temporary due to parameter issues withgetHooks. This aligns with the PR objectives mentioning parameterization problems.Would you like me to create an issue to track the removal of this method once the parameterization issue is resolved?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/api/src/app/inversify.config.ts(2 hunks)apps/api/src/app/routes/hooks.ts(5 hunks)libs/repositories/src/index.ts(1 hunks)libs/repositories/src/repos/DuneRepository/DuneRepository.spec.ts(1 hunks)libs/repositories/src/repos/DuneRepository/DuneRepository.ts(1 hunks)libs/repositories/src/repos/DuneRepository/DuneRepositoryImpl.ts(1 hunks)libs/services/src/HooksService/HooksService.ts(2 hunks)libs/services/src/HooksService/HooksServiceImpl.spec.ts(1 hunks)libs/services/src/HooksService/HooksServiceImpl.ts(1 hunks)libs/services/src/HooksService/utils/isHookData.ts(1 hunks)libs/services/src/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
libs/repositories/src/repos/DuneRepository/DuneRepository.spec.ts (1)
Learnt from: anxolin
PR: #127
File: libs/repositories/src/datasources/orm/postgresOrm.ts:7-12
Timestamp: 2025-04-23T09:18:47.273Z
Learning: The ensureEnvs utility from @cowprotocol/shared causes issues when used with TypeORM migrations in the repositories module, so direct assert statements are used instead for environment variable validation in the ORM datasource configuration.
🧬 Code Graph Analysis (3)
apps/api/src/app/inversify.config.ts (1)
libs/services/src/HooksService/HooksServiceImpl.ts (1)
HooksServiceImpl(13-55)
libs/services/src/HooksService/utils/isHookData.ts (1)
libs/services/src/HooksService/HooksService.ts (1)
HookData(31-45)
libs/services/src/HooksService/HooksServiceImpl.ts (3)
libs/services/src/HooksService/HooksService.ts (4)
HooksService(58-63)GetHooksParams(47-51)HookData(31-45)GetLatestHooksParams(53-56)libs/repositories/src/repos/DuneRepository/DuneRepository.ts (1)
DuneRepository(58-72)libs/services/src/HooksService/utils/isHookData.ts (1)
isHookData(4-58)
⏰ 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). (4)
- GitHub Check: docker (apps/telegram)
- GitHub Check: docker (apps/api)
- GitHub Check: docker (apps/notification-producer)
- GitHub Check: docker (apps/twap)
🔇 Additional comments (19)
apps/api/src/app/inversify.config.ts (1)
33-33: LGTM! Service implementation updated correctly.The import and binding changes properly integrate the new
HooksServiceImplwith itsduneRepositorydependency.Also applies to: 102-102
libs/repositories/src/index.ts (1)
60-60: Export addition follows established pattern.The new export for
DuneRepositoryImplis correctly placed and consistent with other repository exports.libs/services/src/index.ts (1)
11-11: Service export added correctly.The export for
HooksServiceImplfollows the established pattern for service exports.libs/services/src/HooksService/utils/isHookData.ts (1)
4-58: Well-implemented type guard with comprehensive validation.The type guard correctly validates all required and nullable fields of the
HookDatainterface with proper type checking.libs/repositories/src/repos/DuneRepository/DuneRepository.spec.ts (1)
1-518: Comprehensive test coverage with realistic scenarios.The test suite thoroughly covers all methods with success cases, error handling, and edge cases. Good use of realistic mock data and proper test isolation.
libs/services/src/HooksService/HooksServiceImpl.spec.ts (3)
10-151: Well-structured mock implementation!The
MockDuneRepositoryclass provides a comprehensive mock with proper type validation simulation and helpful error messages for debugging test failures.
153-204: Comprehensive test coverage for successful hook retrieval!The test properly validates all fields of the HookData interface, including nullable fields.
326-416: Excellent test coverage for edge cases and enum combinations!The tests properly handle empty results and validate all supported blockchain/period combinations, ensuring comprehensive coverage.
libs/repositories/src/repos/DuneRepository/DuneRepositoryImpl.ts (4)
1-26: Well-structured repository implementation!The class setup with configurable base URL and reasonable default constants provides good flexibility.
27-57: Correct implementation of query execution!The method properly handles parameter serialization and includes helpful logging.
125-138: Clean implementation of paginated query results!The method correctly handles pagination parameters.
140-181: Robust API request handling!The method includes proper authentication, comprehensive logging, and clear error messages.
libs/services/src/HooksService/HooksServiceImpl.ts (1)
46-51: Ignore removal oftypeAssertionand renaming of the constantThe
getQueryResultsmethod accepts atypeAssertionparameter because itsparamstype (GetQueryResultsParams<T>) extendsWithTypeAssertion<T>. The constant nameDEFAULT_QUERY_LATESTSmatches its declaration and should not be renamed.Evidence:
- In
libs/repositories/src/repos/DuneRepository/DuneRepository.ts:export interface GetQueryResultsParams<T> extends WithTypeAssertion<T> { queryId: number; limit?: number; offset?: number; }- The repository interface declares:
getQueryResults<T>( params: GetQueryResultsParams<T> ): Promise<DuneResultResponse<T>>;Please disregard the original suggestion.
Likely an incorrect or invalid review comment.
apps/api/src/app/routes/hooks.ts (1)
50-140: Well-implemented hooks endpoint!The endpoint properly validates parameters using enums, handles errors gracefully, and includes appropriate caching.
libs/services/src/HooksService/HooksService.ts (1)
5-29: Excellent type definition pattern!The single source of truth approach for enum values with both compile-time types and runtime arrays is a best practice.
libs/repositories/src/repos/DuneRepository/DuneRepository.ts (4)
31-37: Well-structured parameter object patternThe
PerformanceTiertype andExecuteQueryParamsinterface are well-designed. The use of a union type for performance tiers provides type safety while maintaining flexibility.
39-46: Good use of interface compositionThe interfaces properly utilize TypeScript's extension mechanism to compose functionality. The optional
maxWaitTimeMsparameter provides sensible timeout configuration.
48-56: Excellent type assertion pattern for runtime safetyThe
WithTypeAssertion<T>interface provides a clean way to validate data at runtime using TypeScript's type predicate pattern. The pagination support inGetQueryResultsParamsis well-structured.
58-72: executeQuery correctly forwards parameters and performanceThe
executeQueryimplementation appends bothparameters(asquery_parameters) andperformanceto the request URL, and the existing Jest tests confirm all combinations pass through as intended. No changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/repositories/src/repos/DuneRepository/DuneRepositoryImpl.ts`:
- Around line 36-46: The code currently appends query_parameters and performance
to the URL query string in executeQuery; instead build a JSON body and send it
in the POST request. Update executeQuery (in DuneRepositoryImpl) to stop calling
queryParams.append(...) and instead construct a body object like {
query_parameters: parameters, performance } (omit keys that are undefined), then
call makeRequest with a body arg; modify makeRequest to accept an optional body
parameter and when present set fetch options.method='POST', set
headers['Content-Type']='application/json', and set body=JSON.stringify(body)
before calling fetch. Ensure any existing URL query construction is only used
for endpoints that truly require querystring and remove the
query_parameters/performance usage from the URL.
🧹 Nitpick comments (8)
libs/repositories/src/repos/DuneRepository/DuneRepositoryImpl.ts (3)
27-57: Consider sending parameters in request body instead of URL.Based on typical REST API patterns and the PR author's note that parameters are being ignored, the execute endpoint likely expects parameters in the POST body rather than URL query params.
♻️ Proposed fix
async executeQuery( params: ExecuteQueryParams ): Promise<DuneExecutionResponse> { const { queryId, parameters, performance } = params; - // Build URL with query parameters - let url = `/query/${queryId}/execute`; - const queryParams = new URLSearchParams(); - - if (parameters) { - queryParams.append('query_parameters', JSON.stringify(parameters)); - } - - if (performance) { - queryParams.append('performance', performance); - } - - if (queryParams.size > 0) { - url += `?${queryParams.toString()}`; - } + const url = `/query/${queryId}/execute`; + + const body: Record<string, unknown> = {}; + if (parameters) { + body.query_parameters = parameters; + } + if (performance) { + body.performance = performance; + } logger.info( `Executing Dune query ${queryId} with parameters: ${JSON.stringify( parameters )} and performance: ${performance || 'medium'}` ); return this.makeRequest<DuneExecutionResponse>(url, { method: 'POST', + body: Object.keys(body).length > 0 ? JSON.stringify(body) : undefined, }); }
157-161: Consider using debug level for request logging.Logging every API request at
infolevel can be noisy in production. Consider usingdebugfor the request details and reservinginfofor significant events.♻️ Suggested change
- logger.info( + logger.debug( `Making Dune API request: ${options.method || 'GET'} ${url}${ options.body ? ` with body: ${options.body}` : '' }` );
167-171: Consider including response body in error for debugging.When the Dune API returns an error, including the response body would help with debugging (e.g., rate limits, invalid query ID, etc.).
♻️ Suggested enhancement
if (!response.ok) { + const errorBody = await response.text().catch(() => 'Unable to read error body'); throw new Error( - `Dune API request failed: ${response.status} ${response.statusText}` + `Dune API request failed: ${response.status} ${response.statusText}. Body: ${errorBody}` ); }libs/services/src/factories.ts (1)
216-224: Consider singleton pattern for consistency.Other external service clients in this file (e.g.,
postgresPool,telegramBot) use singleton caching. The Dune repository could benefit from the same pattern to avoid creating multiple instances.♻️ Suggested singleton pattern
+let duneRepository: DuneRepository | undefined = undefined; + export function getDuneRepository(): DuneRepository { + if (duneRepository) { + return duneRepository; + } + const apiKey = process.env.DUNE_API_KEY; if (!apiKey) { throw new Error('DUNE_API_KEY is not set'); } - return new DuneRepositoryImpl(apiKey); + duneRepository = new DuneRepositoryImpl(apiKey); + return duneRepository; }apps/api/src/app/routes/hooks.ts (3)
83-117: Consider extracting shared response schema.The response schema for hooks is duplicated between both endpoints. Extract it to a constant for maintainability.
♻️ Suggested refactor
const hookItemSchema = { type: 'object', properties: { environment: { type: 'string' }, block_time: { type: 'string' }, is_bridging: { type: 'boolean' }, success: { type: 'boolean' }, app_code: { type: 'string' }, destination_chain_id: { type: ['number', 'null'] }, destination_token_address: { type: ['string', 'null'] }, hook_type: { type: 'string' }, app_id: { type: ['string', 'null'] }, target: { type: 'string' }, gas_limit: { type: 'number' }, app_hash: { type: 'string' }, tx_hash: { type: 'string' }, }, }; const hooksResponseSchema = { type: 'object', properties: { hooks: { type: 'array', items: hookItemSchema }, count: { type: 'number' }, }, };Also applies to: 170-204
138-144: Error responses hide useful debugging information.Returning a generic 500 with empty data hides the actual error cause from API consumers. Consider including an error message field while being careful not to leak sensitive information.
♻️ Suggested enhancement
} catch (error) { fastify.log.error('Error fetching hooks:', error); return reply.status(500).send({ hooks: [], count: 0, + error: 'Internal server error while fetching hooks', }); }Note: You'd also need to update the
HooksResponseinterface and response schema to include the optionalerrorfield.Also applies to: 224-230
31-48: Interface duplicates HookData from service layer.The
HooksResponseinterface defines hook fields that duplicateHookDatafrom@cowprotocol/services. Consider reusing the existing type.♻️ Suggested change
+import { HookData } from '@cowprotocol/services'; + -interface HooksResponse { - hooks: Array<{ - environment: string; - // ... all fields - }>; - count: number; -} +interface HooksResponse { + hooks: HookData[]; + count: number; +}libs/services/src/HooksService/HooksService.ts (1)
61-62: Track the TODO for cleanup.The comment indicates
getLatestHooksis a temporary workaround. Consider opening an issue to track its removal oncegetHooksparameters are working.Would you like me to open an issue to track removing
getLatestHooksonce the parameter passing issue inexecuteQueryis resolved?
libs/repositories/src/repos/DuneRepository/DuneRepositoryImpl.ts
Outdated
Show resolved
Hide resolved
The fix was to send the params in the request body |
|
@anxolin as per your TODO, I removed the
Example: |
kernelwhisperer
changed the title
This PR adds a repository to access dune queries, or execute them and get results.
It also exposes hook!
Motivation
We don't have a way to easily read in CoW Swap from Dune.
Soon we will need to implement the affiliate program, so I did this experimental PR to see if I can get the hooks (since we worked recently in creating some query to return the hooks for the bridge and swap feature 😃
Scope
This is experimental, I don't plan to use it yet in CoW Swap, but leave this easy to add any data we want in CoW Swap now, I think it will be very handy.
Repository
It can:
getQueryResults: Return the cached last execution for a given query. Allows paginationexecuteQuery: Execute a query. The query returns anexecution_idgetExecutionResults: Return the result for a givenexecution_idif readywaitForExecution: Waits for aexecution_idto be ready, and then return the resultHooks Service
getHooks: Returns hooks given some parameters (blockchainandperiod).getLatestHooks: Returns all the cached hooks for the last 30 days. This is kind of experimental query. In this PR I'm only interested on implementing a generic way to access dune and extract data so it can be exported nicely in our endpoints.New Endpoints
/hooks
/hooks/latest
Caveats
The parametrization of
getHooksis not working for me. I pass the parameters toexecuteQueryas I understood from the API, but somehow they are ignored. It's likely something silly, and since this is easily fixable by having a query with the parameters we want, and we don't plan to use this right now I didn't loose more time on this.Test
add API key for dune
Start service:
Go to:
http://localhost:3001
Get all hooks:
http://localhost:3001/hooks/latest?limit=1000&offset=0
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.