Feat/new offchain proposal notification#224
Conversation
|
@LeonardoVieira1630 must be a member of the Blockful team on Vercel to deploy. Learn more about collaboration on Vercel and other options here. |
pikonha
left a comment
pikonha
left a comment
There was a problem hiding this comment.
Code Review
Overall
Well-structured PR that follows existing patterns closely. Excellent test coverage (100% on trigger + handler). Clean separation of concerns across the stack.
Issues
1. initialTimestamp parsing may silently produce wrong cursor values
new-offchain-proposal-trigger.ts:18 — parseInt(initialTimestamp, 10) assumes a numeric string (epoch seconds), but the integration test passes an ISO string via new Date(Date.now() - timeouts.wait.long).toISOString(). parseInt("2026-03-05T...") returns 2026, which is incorrect. Worth verifying this doesn't produce bad cursor values in practice.
2. Dist files committed
packages/anticapture-client/dist/ changes add ~1500 lines of generated code. If there's no CI build step this is understandable, but it bloats the PR significantly and makes review harder. Consider adding a build step to CI instead.
3. Unrelated schema field-order churn in dist/schemas.d.ts
Large chunks of the diff are just reordering value/from/to fields — likely a TypeScript/codegen artifact. Not a bug, but adds noise.
4. Potential graphql version conflict
Adding msw@2.12.10 pulls in graphql@16.13.0 while the project already has graphql@16.11.0. Verify there are no runtime conflicts from duplicate GraphQL versions.
5. pnpm-lock.yaml churn
~200 lines of lockfile changes beyond msw, with resolution flips between @types/node@20.17.46 and @types/node@24.3.1. May indicate different Node versions across contributors — consider pinning @types/node.
Minor / Nits
- Missing trailing newline in
anticapture-client.tsandfixtures/index.ts - Sequential DAO querying in
listOffchainProposalsis consistent with existing code but could benefit fromPromise.allSettledfor parallelization in the future cryptoimport innew-offchain-proposal-trigger.service.tsjust forrandomUUID()— available globally in Node 19+
Verdict
Looks good overall. Main item to address is #1 (timestamp parsing). The rest are minor.
| import { AnticaptureClient } from '@notification-system/anticapture-client'; | ||
| import { OffchainProposalDataSource, OffchainProposal, ListOffchainProposalsOptions } from '../interfaces/offchain-proposal.interface'; | ||
|
|
||
| export class OffchainProposalRepository implements OffchainProposalDataSource { |
There was a problem hiding this comment.
this is more like a service than a repository to me
There was a problem hiding this comment.
i dont agree hehe
| async listOffchainProposals( | ||
| variables?: ListOffchainProposalsQueryVariables, | ||
| daoId?: string | ||
| ): Promise<(OffchainProposalItem & { daoId: string })[]> { | ||
| if (!daoId) { | ||
| const allDAOs = await this.getDAOs(); | ||
| const allProposals: (OffchainProposalItem & { daoId: string })[] = []; | ||
|
|
||
| for (const dao of allDAOs) { | ||
| try { | ||
| const validated = await this.query(ListOffchainProposalsDocument, SafeOffchainProposalsResponseSchema, variables, dao.id); | ||
| const items = validated.offchainProposals.items.map(item => ({ ...item, daoId: dao.id })); | ||
| if (items.length > 0) { | ||
| allProposals.push(...items); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Skipping offchain proposals for ${dao.id} due to API error: ${error instanceof Error ? error.message : error}`); | ||
| } | ||
| } | ||
|
|
||
| // Sort by created timestamp desc (most recent first) | ||
| allProposals.sort((a, b) => b.created - a.created); | ||
| return allProposals; | ||
| } | ||
|
|
||
| try { | ||
| const validated = await this.query(ListOffchainProposalsDocument, SafeOffchainProposalsResponseSchema, variables, daoId); | ||
| return validated.offchainProposals.items.map(item => ({ ...item, daoId })); | ||
| } catch (error) { | ||
| console.warn(`Error querying offchain proposals for DAO ${daoId}: ${error instanceof Error ? error.message : error}`); | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
im pretty sure this could be simplified by relying on TS types like ReturnType. not something we should worry now, but nice to have in mind
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
Summary
listOffchainProposalsGraphQL query and Zod validation schemaTest coverage
new-offchain-proposal-trigger.ts(logic-system)new-offchain-proposal-trigger.service.ts(dispatcher)