Automatic verification of new proposals Reality question #72
Conversation
WalkthroughImplements validation-driven proposal notification flow. Adds Snapshot service (module address/proposal fetch), Reality service event shape change, new validation utilities, updated notification types, processing logic branching (valid/incomplete/alert), new templates for channels, DB helper for space deletion, expanded tests, and minor Docker/compose tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Reality as Reality Module
participant Proc as Processing
participant Snap as Snapshot Service
participant Val as Validation Utils
participant DB as DB
participant Notify as Notifier
Reality->>Proc: LogNewQuestion(event)
Proc->>Snap: getProposal(proposalId)
alt Proposal found
Proc->>Val: validateRealityQuestion(event, proposal)
alt isValid = true
Note right of Proc: Build ValidProposalNotification
Proc->>Notify: PROPOSAL_QUESTION_VALID
else isValid = false (INCOMPLETE_DATA)
Note right of Proc: Build InvalidProposalNotification (warning)
Proc->>Notify: PROPOSAL_QUESTION_INCOMPLETE_DATA
else isValid = false (ALERT)
Note right of Proc: Build InvalidProposalNotification (alert)
Proc->>Notify: PROPOSAL_QUESTION_ALERT
end
Proc->>DB: insert proposal
else Proposal missing
Proc-->>Proc: throw MissingSnapshotProposalError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
|
||
| ARG TARGETPLATFORM | ||
| ENV WAITDEPS_VERSION 0.0.1 | ||
| ENV WAITDEPS_VERSION=0.0.1 |
There was a problem hiding this comment.
Fixes a warning on Docker side, the previous syntax seems deprecated
| insertProposalFn: insertProposal, | ||
| afterEach(async () => { | ||
| await removeProposalByQuestionId(proposal.questionId); | ||
| await removeSpaceByEns(space.ens); |
There was a problem hiding this comment.
Promise.all can't be use here due to the database relationship between proposal and space
There was a problem hiding this comment.
The changes on this file are mainly to assert that the right notification is built depending on the proposal validation result. There is some code duplication which I left as it because it seems more clean and easy to follow than creating functions for 3 times reuse.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/links.ts (1)
67-71: txHash should be a Hash, not AddressTransaction hashes are 32-byte hashes. Adjust type to prevent misuse.
-import type { Address } from "viem"; +import type { Address, Hash } from "viem"; ... -export const getBlockExplorerLinkForTx = ( - txHash: Address, +export const getBlockExplorerLinkForTx = ( + txHash: Hash, template: string = env.BLOCK_EXPLORER_TX_LINK_TEMPLATE, ): string => {src/services/reality/index.ts (1)
225-240: Verify safe parsing of question string.The parsing logic splits
rawQuestionon the "␟" character and accessesquestionParts[0]andquestionParts[1]without bounds checking. If the question format is malformed or doesn't contain the separator, this will result inundefinedvalues being cast toHexandHashtypes.Additionally, line 239 unconditionally prefixes
questionParts[1]with"0x", which could result in double-prefixing if the value already contains the prefix.Consider adding validation:
const rawQuestion = decoded.args.question; const questionParts = rawQuestion.split("␟"); + +if (questionParts.length < 2) { + throw new Error(`Invalid question format in LogNewQuestion event: ${rawQuestion}`); +} let { opening_ts: openingTs, timeout } = decoded.args; if (openingTs === 0) { const block = await publicClient.getBlock({ blockNumber: log.blockNumber, }); openingTs = Number(block.timestamp); } return { questionId: decoded.args.question_id.toLowerCase() as Hash, question: { proposalId: questionParts[0] as Hex, - safeHash: `0x${questionParts[1]}` as Hash, + safeHash: (questionParts[1].startsWith('0x') ? questionParts[1] : `0x${questionParts[1]}`) as Hash, },
🧹 Nitpick comments (23)
Dockerfile (1)
4-4: ENV change is fine; consider hardening the download stepUsing the equals form is equivalent here. To improve supply‑chain safety, consider verifying the tarball with a checksum/signature instead of blind piping to tar.
src/notify.ts (2)
74-85: Dedup race: make insert idempotent to avoid duplicate sendsIf two processes handle the same txHash concurrently, both may send before insertUsedTransport records the transport. Ensure insertUsedTransport is idempotent (unique constraint + “on conflict do nothing”) and ignored on duplicate to prevent Promise.all from rejecting post‑send.
Confirm insertUsedTransport ignores unique violations and returns success semantics. Optionally catch/ignore duplicate errors around the insert.
35-40: Include structured error details in InvalidProposalNotification
- Replace
validationMessage: stringwithvalidationSummary: string,errorCode: string, andseverity: ValidationErrorSeverity(sourced from the caughtValidationResultFailed).- Optionally add
errors?: ValidationResultFailed[]if you plan to accumulate multiple validation issues in the future.templates/test/proposal-alert.ejs (1)
3-17: Harden template rendering for optional dates and bigint fieldsGuard nullable dates and explicitly stringify bigints to avoid runtime errors in EJS.
Apply:
-space.startBlock: <%- notification.space.startBlock %> -space.lastProcessedBlock: <%- notification.space.lastProcessedBlock %> +space.startBlock: <%- String(notification.space.startBlock) %> +space.lastProcessedBlock: <%- notification.space.lastProcessedBlock == null ? "" : String(notification.space.lastProcessedBlock) %> -event.blockNumber: <%- notification.event.blockNumber %> +event.blockNumber: <%- String(notification.event.blockNumber) %> -event.startedAt: <%- notification.event.startedAt.toISOString() %> -event.finishedAt: <%- notification.event.finishedAt.toISOString() %> +event.startedAt: <%- notification.event.startedAt?.toISOString?.() || "" %> +event.finishedAt: <%- notification.event.finishedAt?.toISOString?.() || "" %> -event.timeout: <%- notification.event.timeout %> +event.timeout: <%- String(notification.event.timeout) %> -event.happenedAt: <%- notification.event.happenedAt.toISOString() %> +event.happenedAt: <%- notification.event.happenedAt?.toISOString?.() || "" %> -validationMessage: <%- notification.validationMessage %> +validationMessage: <%- notification.validationMessage ?? "" %>src/services/reality-question-validation/eip-712-transaction-hash.test.ts (1)
6-21: Add one negative/variant case for robustnessGreat golden test. Add a second case (e.g., operation 0 vs 1, or different chainId) to ensure domain/message changes alter the hash as expected. Optionally assert type conformance by typing “to” as Address for symmetry.
src/services/reality-question-validation/eip-712-transaction-hash.ts (2)
13-16: Consider addingnameandversionto the Domain type.EIP-712 domain separators typically include
nameandversionfields in addition tochainIdandverifyingContract. While not strictly required by the EIP-712 spec, omitting these fields may cause compatibility issues with tools that expect a complete domain separator, and reduces the uniqueness guarantees of the domain.Apply this diff to add the optional fields:
type Domain = { + name?: string; + version?: string; chainId: number; verifyingContract: Address; };
3-11: Consider exporting thetypesconstant for reuse.If other parts of the codebase need to reference the Transaction type schema (e.g., for validation, testing, or documentation), exporting the
typesconstant would improve maintainability.Apply this diff if the types need to be reused elsewhere:
-const types = { +export const types = { Transaction: [ { name: "to", type: "address" }, { name: "value", type: "uint256" }, { name: "data", type: "bytes" }, { name: "operation", type: "uint8" }, { name: "nonce", type: "uint256" }, ], } as const;templates/email/proposal-valid-plain.ejs (1)
13-13: Clarify the hash description."keccak of the concatenation" is missing "hash" after "keccak" for clarity. The sentence should read "keccak hash of the concatenation" to be more precise.
Apply this diff:
-- The proposal Safes hashes are the keccak of the concatenation of the individual hashes +- The proposal Safes hashes are the keccak hash of the concatenation of the individual hashestemplates/telegram/proposal-valid.ejs (1)
11-11: Clarify the hash description."keccak of the concatenation" is missing "hash" after "keccak" for clarity.
Apply this diff:
-- The proposal Safes hashes are the keccak of the concatenation of the individual hashes +- The proposal Safes hashes are the keccak hash of the concatenation of the individual hashessrc/utils/notification-template.test.ts (1)
65-113: Remove unnecessary async and stray property in invalid cases
- describe callback doesn’t need to be async; Mocha won’t await it.
- Second case object has an extra top-level
type(outsidenotification) — dead field.Apply:
-describe("should interpolate an invalid Proposal notification into a test template", async () => { +describe("should interpolate an invalid Proposal notification into a test template", () => {{ title: "when the notification is an alert due to validation failure", - type: EventType.PROPOSAL_QUESTION_ALERT, notification: { ...randomizeProposalNotification(), type: EventType.PROPOSAL_QUESTION_ALERT, validationMessage: "This is the alert message reason", } as InvalidProposalNotification, },src/utils/links.ts (1)
5-12: Fix JSDoc typo“propoasal” → “proposal”.
- * Given a notification, generates a link to the Snapshot propoasal page. + * Given a notification, generates a link to the Snapshot proposal page.src/services/snapshot/index.ts (1)
26-32: Prefer GraphQL variables over string interpolationUse variables for ids to avoid quoting/injection pitfalls and to simplify escaping.
Example shape:
- query($id: String!) { space(id: $id) { plugins } }
- query($id: String!) { proposal(id: $id) { ... } }
Also applies to: 79-89
src/services/reality-question-validation/errors.ts (1)
8-15: Consider setting error name explicitly.The
BaseProposalValidationErrorclass extendsErrorbut doesn't setthis.name. While JavaScript automatically sets thenameproperty to the class name in modern environments, explicitly setting it ensures consistency across all runtime environments and makes the error more debuggable.Apply this diff:
export class BaseProposalValidationError extends Error { public severity: ValidationErrorSeverity; constructor(severity: ValidationErrorSeverity, message: string) { - super(message); // Pass message to the Error constructor + super(message); + this.name = this.constructor.name; this.severity = severity; + Error.captureStackTrace(this, this.constructor); } }src/services/reality-question-validation/index.ts (5)
31-33: Use strict equality for type safety.Line 31 uses loose equality (
!=) instead of strict equality (!==). Since bothproposal.idandevent.question.proposalIdare typed asHex(which is a branded string type), using strict equality is more type-safe and prevents unexpected type coercion.Apply this diff:
- if (proposal.id != event.question.proposalId) { + if (proposal.id !== event.question.proposalId) { throw new ProposalIdMismatchError(proposal.id, event.question.proposalId); }
61-61: Use strict equality for hash comparison.Line 61 uses loose equality (
!=) for hash comparison. Use strict equality (!==) for type safety.Apply this diff:
- if (hash != calculatedHash) throw new TxHashMismatchError(hash, calculatedHash); + if (hash !== calculatedHash) throw new TxHashMismatchError(hash, calculatedHash);
66-66: Use strict equality for hash comparison.Line 66 uses loose equality (
!=) for hash comparison. Use strict equality (!==) for type safety.Apply this diff:
- if (calculatedSafeHash != hash) throw new SafeHashCalculationError(hash, calculatedSafeHash); + if (calculatedSafeHash !== hash) throw new SafeHashCalculationError(hash, calculatedSafeHash);
71-71: Use strict equality for hash comparison.Line 71 uses loose equality (
!=) for hash comparison. Use strict equality (!==) for type safety.Apply this diff:
- if (proposalNetworkSafe.hash != event.question.safeHash) throw new SafeHashMismatchError(); + if (proposalNetworkSafe.hash !== event.question.safeHash) throw new SafeHashMismatchError();
47-63: Potential issue: Destructuringhashfromtxshadows the outerhashfromsafe.On line 49, the code destructures
hashfromtx, which shadows thehashvariable from the outer scope (line 40). This could lead to confusion and potential bugs if the wronghashis referenced later. While the current code works correctly, this pattern reduces readability.Consider renaming the inner
hashto make the code clearer:const txHashes = txs.map((tx) => { const { - hash, + hash: txHash, mainTransaction: { to, data, nonce, value, operation }, } = tx; const calculatedHash = calculateTxHash(domain, { to: to as Address, data: data as Hex, nonce: BigInt(nonce), value: BigInt(value), operation: Number(operation), }); - if (hash !== calculatedHash) throw new TxHashMismatchError(hash, calculatedHash); + if (txHash !== calculatedHash) throw new TxHashMismatchError(txHash, calculatedHash); return calculatedHash; });templates/slack/proposal-alert.ejs (1)
1-1: Sanitize unescaped fields to avoid Slack formatting/injection issues
space.ensandvalidationMessageare rendered unescaped. If any part is user/externally sourced, escape or sanitize for Slack Markdown (e.g., escape* _ \< > &`). If you intend rich formatting, document that these strings are trusted/internal.Also applies to: 5-6
templates/email/proposal-alert-plain.html.ejs (1)
5-5: Escape HTML and make links clickable
- Escape
validationMessageandspace.ensto prevent HTML injection.- Use anchor tags for reliable clickability in email clients.
- > <%- notification.event.validationMessage %> + > <%= notification.event.validationMessage %>-A new Reality question has been created for space <%- notification.space.ens %>. +A new Reality question has been created for space <%= notification.space.ens %>.-- Snapshot proposal: <%- getSnapshotProposalLink(notification) %> -- Reality question: <%- getRealityQuestionLink(notification) %> -- Transaction: <%- getBlockExplorerLinkForTx(notification.event.txHash) %> +- Snapshot proposal: <a href="<%= getSnapshotProposalLink(notification) %>">Snapshot proposal</a> +- Reality question: <a href="<%= getRealityQuestionLink(notification) %>">Reality question</a> +- Transaction: <a href="<%= getBlockExplorerLinkForTx(notification.event.txHash) %>">Transaction</a>Also applies to: 11-11, 15-17
templates/telegram/proposal-incomplete-data.ejs (2)
6-6: Escape/sanitize for Telegram Markdown
validationMessageis unescaped and may contain characters that break Telegram Markdown (e.g.,_ * [ ] ( ) ~ \> # + - = | { } . !`). Ensure it’s escaped or sanitized for the chosen parse mode (MarkdownV2/HTML), or render via a helper that returns a safe string.
18-25: Grammar fix“has PASSED” → “have PASSED”.
-The following automatic checks has PASSED: +The following automatic checks have PASSED:templates/email/proposal-valid-html.ejs (1)
7-7: Escape space name to prevent HTML injectionUse escaped interpolation for
space.ens.- <h1 ...>A new Reality question has been created for a Snapshot proposal in the space <%- notification.space.ens %>.</h1> + <h1 ...>A new Reality question has been created for a Snapshot proposal in the space <%= notification.space.ens %>.</h1>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
Dockerfile(1 hunks)docker-compose.yml(1 hunks)src/notify.ts(2 hunks)src/processing.integration-test.ts(3 hunks)src/processing.ts(3 hunks)src/services/db/proposal.integration-test.ts(3 hunks)src/services/db/spaces.ts(1 hunks)src/services/reality-question-validation/eip-712-transaction-hash.test.ts(1 hunks)src/services/reality-question-validation/eip-712-transaction-hash.ts(1 hunks)src/services/reality-question-validation/errors.ts(1 hunks)src/services/reality-question-validation/index.test.ts(1 hunks)src/services/reality-question-validation/index.ts(1 hunks)src/services/reality/index.integration-test.ts(1 hunks)src/services/reality/index.ts(5 hunks)src/services/snapshot/index.integration-test.ts(1 hunks)src/services/snapshot/index.ts(1 hunks)src/utils/errors.ts(2 hunks)src/utils/links.test.ts(3 hunks)src/utils/links.ts(4 hunks)src/utils/notification-template.test.ts(4 hunks)src/utils/test-mocks.ts(2 hunks)templates/email/proposal-alert-plain.html.ejs(1 hunks)templates/email/proposal-alert-subject.ejs(1 hunks)templates/email/proposal-incomplete-data-html.ejs(1 hunks)templates/email/proposal-incomplete-data-plain.ejs(1 hunks)templates/email/proposal-incomplete-data-subject.ejs(1 hunks)templates/email/proposal-valid-html.ejs(1 hunks)templates/email/proposal-valid-plain.ejs(1 hunks)templates/slack/proposal-alert.ejs(1 hunks)templates/slack/proposal-incomplete-data.ejs(1 hunks)templates/telegram/proposal-alert.ejs(1 hunks)templates/telegram/proposal-incomplete-data.ejs(1 hunks)templates/telegram/proposal-valid.ejs(1 hunks)templates/test/proposal-alert.ejs(1 hunks)templates/test/proposal-incomplete-data.ejs(1 hunks)templates/test/proposal-valid.ejs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
src/services/db/spaces.ts (1)
src/services/db/connection.ts (1)
getConnection(40-46)
src/services/reality-question-validation/eip-712-transaction-hash.test.ts (1)
src/services/reality-question-validation/eip-712-transaction-hash.ts (1)
calculateTxHash(36-43)
src/utils/notification-template.test.ts (3)
src/utils/tests-setup.ts (1)
expect(9-9)src/notify.ts (2)
TransportName(9-9)InvalidProposalNotification(35-40)src/utils/test-mocks.ts (1)
randomizeProposalNotification(74-81)
src/utils/links.ts (1)
src/notify.ts (2)
ValidProposalNotification(29-33)AnswerNotification(42-46)
src/utils/links.test.ts (1)
src/notify.ts (1)
ValidProposalNotification(29-33)
src/utils/test-mocks.ts (1)
src/notify.ts (1)
ValidProposalNotification(29-33)
src/notify.ts (3)
src/services/reality/index.ts (2)
ProposalQuestionCreated(118-125)LogNewAnswer(255-264)src/services/db/proposals.ts (1)
Proposal(10-10)src/types.ts (1)
Space(3-7)
src/services/reality-question-validation/index.ts (4)
src/services/reality/index.ts (1)
LogNewQuestion(174-187)src/services/snapshot/index.ts (1)
SnapshotProposal(76-76)src/services/reality-question-validation/errors.ts (7)
ProposalIdMismatchError(17-24)SafeSnapPluginConfigurationError(25-32)TxHashMismatchError(34-41)SafeHashCalculationError(43-50)SafeNotFoundForProposalNetworkError(52-59)SafeHashMismatchError(61-68)BaseProposalValidationError(8-15)src/services/reality-question-validation/eip-712-transaction-hash.ts (1)
calculateTxHash(36-43)
src/processing.ts (5)
src/services/snapshot/index.ts (1)
getProposal(78-92)src/services/reality-question-validation/index.ts (2)
validateRealityQuestion(98-112)ValidationResult(85-85)src/notify.ts (4)
notify(58-65)Notification(47-47)ValidProposalNotification(29-33)InvalidProposalNotification(35-40)src/services/reality/index.ts (3)
ProposalQuestionCreated(118-125)LogNewQuestion(174-187)getLogNewQuestion(204-253)src/utils/errors.ts (1)
MissingSnapshotProposalError(12-19)
src/services/reality-question-validation/index.test.ts (4)
src/services/reality/index.ts (1)
LogNewQuestion(174-187)src/services/snapshot/index.ts (1)
SnapshotProposal(76-76)src/services/reality-question-validation/index.ts (1)
assertValidRealityQuestion(30-72)src/services/reality-question-validation/errors.ts (6)
TxHashMismatchError(34-41)SafeHashCalculationError(43-50)SafeHashMismatchError(61-68)SafeSnapPluginConfigurationError(25-32)ProposalIdMismatchError(17-24)SafeNotFoundForProposalNetworkError(52-59)
src/services/snapshot/index.integration-test.ts (2)
src/services/snapshot/index.ts (2)
getRealityModuleAddress(25-36)getProposal(78-92)src/utils/tests-setup.ts (2)
expect(9-9)ONEINCH_MODULE_ADDRESS(40-40)
src/services/db/proposal.integration-test.ts (1)
src/notify.ts (1)
ValidProposalNotification(29-33)
src/services/snapshot/index.ts (2)
src/utils/fetch-graphql.ts (1)
graphQLFetch(2-17)src/utils/env.ts (1)
env(193-193)
src/processing.integration-test.ts (7)
src/utils/test-mocks.ts (1)
randomizeSpace(21-27)src/utils/tests-setup.ts (2)
ONEINCH_ORACLE_ADDRESS(41-41)expect(9-9)src/services/db/spaces.ts (2)
insertSpaces(30-33)removeSpaceByEns(59-62)src/services/db/proposals.ts (3)
removeProposalByQuestionId(57-61)insertProposal(24-28)findProposalByQuestionId(39-47)src/services/reality/index.ts (1)
getLogNewQuestion(204-253)src/services/reality-question-validation/index.ts (2)
validateRealityQuestion(98-112)ValidationResult(85-85)src/services/snapshot/index.ts (1)
getProposal(78-92)
🔇 Additional comments (27)
docker-compose.yml (1)
80-81: Good hardening: bind MailDev UI to loopback onlyLimiting the MailDev web UI to 127.0.0.1 reduces exposure while containers still reach smtp:1080 internally. LGTM.
src/notify.ts (1)
19-22: Enum change verified – no stale references remain
No occurrences ofPROPOSAL_QUESTION_CREATEDorProposalNotificationalias detected; new enum values are correctly used throughout.templates/email/proposal-incomplete-data-plain.ejs (1)
1-23: LGTM!The plain text email template is well-structured and clear. The warning block effectively highlights incomplete validation, and the use of unescaped interpolation (
<%- %>) is appropriate for plain text emails.templates/email/proposal-incomplete-data-subject.ejs (1)
1-1: LGTM!The subject line template is concise and informative. The ENS and question ID fields are typically well-formed (ENS domains and hex hashes), so unescaped interpolation is acceptable here.
templates/telegram/proposal-alert.ejs (1)
1-14: LGTM!The alert template effectively highlights validation failures with clear visual indicators (🚨 emojis). The structure is consistent with other Telegram templates in the codebase.
templates/email/proposal-alert-subject.ejs (1)
1-1: LGTM!The alert subject line clearly communicates the severity with the 🚨 emoji and provides essential context.
src/services/db/spaces.ts (1)
59-62: LGTM!The implementation is clean and follows the existing patterns in the codebase. The function correctly uses Drizzle ORM to delete a space by ENS.
src/services/db/proposal.integration-test.ts (1)
3-3: LGTM!The type updates from
ProposalNotificationtoValidProposalNotificationalign with the PR's introduction of validation-specific notification types. The changes are minimal and maintain test integrity.Also applies to: 12-12
src/utils/test-mocks.ts (1)
7-7: LGTM!The updates to use
ValidProposalNotificationand the new event typePROPOSAL_QUESTION_VALIDare consistent with the PR's validation-driven notification architecture. The changes maintain the existing test data generation patterns.Also applies to: 74-81
templates/test/proposal-valid.ejs (1)
1-16: LGTM!The test template comprehensively captures all notification fields in a structured, parseable format. The use of
toISOString()for timestamps ensures consistent date formatting across test outputs.src/services/reality-question-validation/eip-712-transaction-hash.ts (1)
36-43: LGTM: Hash calculation implementation is correct.The function correctly delegates to viem's
hashTypedDatawith the appropriate domain, types, primaryType, and message. The implementation follows EIP-712 standards for typed data hashing.src/utils/links.test.ts (1)
1-41: LGTM: Test updates align with notification type changes.The test correctly updates the import and type assertion to use
ValidProposalNotificationinstead ofProposalNotification, consistent with the broader API changes in the PR.templates/slack/proposal-incomplete-data.ejs (1)
1-16: LGTM: Slack template is well-structured.The template correctly formats the incomplete data notification with appropriate warning indicators, includes all relevant links and details, and follows Slack markdown conventions.
templates/email/proposal-incomplete-data-html.ejs (1)
1-47: LGTM: HTML email template is well-structured.The template correctly uses shared layout components, follows the established email template structure, and properly displays all required notification details with appropriate styling.
src/services/snapshot/index.integration-test.ts (1)
1-48: LGTM: Integration tests are comprehensive.The test suite thoroughly validates both
getRealityModuleAddressandgetProposalfunctions with appropriate positive and negative test cases. The assertions cover multiple levels of nested data, ensuring the GraphQL query and response parsing work correctly.templates/test/proposal-incomplete-data.ejs (1)
17-17: validationMessage path is correct validationMessage is a top-level field on InvalidProposalNotification (see src/notify.ts:39) and tests reference it accordingly—no change needed.Likely an incorrect or invalid review comment.
src/utils/notification-template.test.ts (1)
15-23: Template path assertions updated correctlyPaths now target proposal-valid variants; assertions look right.
src/services/reality-question-validation/index.test.ts (1)
71-126: Good, comprehensive validation coverageHappy path + clear alert/warn cases (tx hash, safe hash, plugin config, proposal id/network). Reads well and is deterministic.
src/processing.integration-test.ts (4)
145-162: LGTM!The new beforeEach/afterEach pattern properly sets up and tears down test data for each test, ensuring test isolation and preventing data leakage between tests.
164-193: LGTM!The test properly validates the happy path where a valid proposal passes validation and results in a
PROPOSAL_QUESTION_VALIDnotification. The test verifies both the notification structure and that the proposal is persisted to the database.
195-235: LGTM!The test properly validates the incomplete data path where validation fails due to missing data, resulting in a
PROPOSAL_QUESTION_INCOMPLETE_DATAnotification with the validation message.
237-277: LGTM!The test properly validates the security alert path where validation fails due to a security concern, resulting in a
PROPOSAL_QUESTION_ALERTnotification with the validation message.src/services/reality-question-validation/index.ts (2)
98-112: LGTM!The
validateRealityQuestionwrapper properly catchesBaseProposalValidationErrorinstances and maps them to a structuredValidationResult, while re-throwing unexpected errors. This provides a clean API for callers.
39-67: Review comment incorrect: verifyingContract correctly uses the Safe address (realityAddress), field conversions will error on invalid inputs, andkeccak256(concat(txHashes))matches the SafeSnap spec.Likely an incorrect or invalid review comment.
src/processing.ts (2)
219-249: LGTM!The
buildProposalNotificationfunction cleanly separates the notification construction logic and properly maps validation results to the appropriate notification types (VALID, INCOMPLETE_DATA, or ALERT).
282-285: LGTM!The processing flow properly fetches the snapshot proposal and validates the reality question before notification. The error handling for missing snapshot proposals is appropriate.
src/services/reality/index.integration-test.ts (1)
102-105: LGTM: updated event shape assertionAsserting
questionas an object withproposalIdandsafeHashmatches the new event schema.
| const logNewQuestionFields = { | ||
| snapshotId: question.proposalId, | ||
| startedAt, | ||
| finishedAt, | ||
| timeout, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify correct field assignment in logNewQuestionFields.
Line 288 assigns snapshotId: question.proposalId, where question is destructured from newQuestionEvent on line 280. This means snapshotId is set to the proposalId from the Reality question event.
However, this should be validated against the actual event.proposalId from the ProposalQuestionCreated event to ensure consistency. While the validation logic in assertValidRealityQuestion checks that proposal.id matches event.question.proposalId, it doesn't verify that this matches event.proposalId.
Run the following script to verify that proposalId is consistent across events:
If event.proposalId (from ProposalQuestionCreated) and question.proposalId (from LogNewQuestion) can differ, consider adding a validation check to ensure they match.
🏁 Script executed:
#!/bin/bash
# Description: Verify that proposalId from ProposalQuestionCreated matches proposalId in LogNewQuestion
# Search for ProposalQuestionCreated event structure
ast-grep --pattern $'type ProposalQuestionCreated = {
$$$
proposalId: $_;
$$$
}'
# Search for LogNewQuestion question field structure
ast-grep --pattern $'question: {
proposalId: $_;
$$$
}'Length of output: 414
🏁 Script executed:
#!/bin/bash
sed -n '260,330p' src/processing.tsLength of output: 2151
🏁 Script executed:
#!/bin/bash
rg -n "configurableProcessProposals" -n .Length of output: 421
🏁 Script executed:
#!/bin/bash
sed -n '160,240p' src/processing.ts
rg -n "type ConfigurableProcessProposalsDeps" -n src/processing.tsLength of output: 2351
🏁 Script executed:
#!/bin/bash
# Show imports and early context in processing.ts
sed -n '1,80p' src/processing.ts
# Locate validateRealityQuestion implementation or declaration
rg -n "validateRealityQuestion" -n src
# Locate LogNewQuestion type definition
rg -n "type LogNewQuestion" -n srcLength of output: 4100
🏁 Script executed:
#!/bin/bash
# Show LogNewQuestion type definition with context
sed -n '160,220p' src/services/reality/index.tsLength of output: 1513
🏁 Script executed:
#!/bin/bash
# Inspect getProposalQuestionsCreated implementation and its event argument mapping
rg -n "export const getProposalQuestionsCreated" -n src/services/reality/index.ts -C10Length of output: 915
Validate matching proposal IDs before using them. Add an explicit check in configurableProcessProposals so that
if (newQuestionEvent.question.proposalId !== event.proposalId) {
throw new Error(`Mismatched proposalId: logNewQuestion=${newQuestionEvent.question.proposalId} vs createdEvent=${event.proposalId}`);
}before calling getSnapshotProposalFn and inserting into the database.
🤖 Prompt for AI Agents
In src/processing.ts around lines 287 to 292, add an explicit validation that
the proposalId on the newQuestionEvent matches the proposalId on the created
event before calling getSnapshotProposalFn or inserting into the DB; if they
differ, throw an Error with a clear message such as "Mismatched proposalId:
logNewQuestion=<value> vs createdEvent=<value>" to fail fast and avoid inserting
inconsistent data.
| /** | ||
| * Removes an space by its ens |
There was a problem hiding this comment.
Fix typo in documentation.
The doc comment contains a grammatical error: "an space" should be "a space".
Apply this diff:
/**
- * Removes an space by its ens
+ * Removes a space by its ENS
*📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Removes an space by its ens | |
| /** | |
| * Removes a space by its ENS | |
| * |
🤖 Prompt for AI Agents
In src/services/db/spaces.ts around lines 51 to 52, the doc comment has a typo
"an space" — update the comment to read "a space" so it correctly states
"Removes a space by its ens"; ensure spacing and punctuation remain consistent
with surrounding JSDoc style.
There was a problem hiding this comment.
Noted. I will fix it.
There was a problem hiding this comment.
@fcanela, thank you for acknowledging this! I appreciate your attention to the documentation quality. 🙏
(\(\
( -.-)
o_(")(")
Happy to help!
| type SnapshotSpaceResponse = { | ||
| space: { | ||
| plugins: { | ||
| safeSnap: { | ||
| address: Address; | ||
| }; | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Harden null-safety when reading plugins.safeSnap.address
Avoid runtime exceptions when any layer is missing.
-type SnapshotSpaceResponse = {
- space: {
- plugins: {
- safeSnap: {
- address: Address;
- };
- };
- };
-};
+type SnapshotSpaceResponse = {
+ space: {
+ plugins?: {
+ safeSnap?: {
+ address?: Address;
+ };
+ } | null;
+ } | null;
+};
...
- const { space } = await graphQLFetch<SnapshotSpaceResponse>(env.SNAPSHOT_GRAPHQL_URL, query);
-
- return space ? space.plugins.safeSnap.address : null;
+ const { space } = await graphQLFetch<SnapshotSpaceResponse>(env.SNAPSHOT_GRAPHQL_URL, query);
+ return space?.plugins?.safeSnap?.address ?? null;Also applies to: 33-36
There was a problem hiding this comment.
Good suggestion. I will implement it.
There was a problem hiding this comment.
@fcanela Thank you for confirming! Looking forward to seeing the null-safety improvements implemented. 👍
| export class MissingSnapshotProposalError extends Error { | ||
| constructor(proposalId: Hex, questionId: Hex, txHash: Hash) { | ||
| const message = `Unable to resolve proposal ${proposalId} related to Reality question ${questionId} present in the LogNewQuestion event in tx ${txHash}`; | ||
| super(message); | ||
| this.name = this.constructor.name; | ||
| Error.captureStackTrace(this, InvalidLogNewQuestionArgsEventError); | ||
| Error.captureStackTrace(this, MissingLogNewQuestionEventError); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Wrong class passed to captureStackTrace
Use the current class to get a clean stack.
- Error.captureStackTrace(this, MissingLogNewQuestionEventError);
+ Error.captureStackTrace(this, MissingSnapshotProposalError);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class MissingSnapshotProposalError extends Error { | |
| constructor(proposalId: Hex, questionId: Hex, txHash: Hash) { | |
| const message = `Unable to resolve proposal ${proposalId} related to Reality question ${questionId} present in the LogNewQuestion event in tx ${txHash}`; | |
| super(message); | |
| this.name = this.constructor.name; | |
| Error.captureStackTrace(this, InvalidLogNewQuestionArgsEventError); | |
| Error.captureStackTrace(this, MissingLogNewQuestionEventError); | |
| } | |
| } | |
| } | |
| export class MissingSnapshotProposalError extends Error { | |
| constructor(proposalId: Hex, questionId: Hex, txHash: Hash) { | |
| const message = `Unable to resolve proposal ${proposalId} related to Reality question ${questionId} present in the LogNewQuestion event in tx ${txHash}`; | |
| super(message); | |
| this.name = this.constructor.name; | |
| Error.captureStackTrace(this, MissingSnapshotProposalError); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/utils/errors.ts around lines 12 to 19, the Error.captureStackTrace call
passes the wrong class (MissingLogNewQuestionEventError); replace it to use the
current class so the stack is captured correctly—call
Error.captureStackTrace(this, MissingSnapshotProposalError) or
Error.captureStackTrace(this, this.constructor) after super(...) so the stack
trace is associated with MissingSnapshotProposalError.
There was a problem hiding this comment.
Copy paste error :) I will fix it!
There was a problem hiding this comment.
@fcanela Thank you for confirming! No problem—easy fix. Looking forward to seeing it corrected in the next commit. 👍
| The following automatic checks has PASSED: | ||
| - The proposal Safes hashes are the keccak of the concatenation of the individual hashes | ||
| - The hash included on the Reality.eth question is the Proposal safe hash |
There was a problem hiding this comment.
Fix grammar: "checks has" → "checks have"
The subject "checks" is plural and requires the plural verb form "have".
Apply this diff:
-The following automatic checks has PASSED:
+The following automatic checks have PASSED:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The following automatic checks has PASSED: | |
| - The proposal Safes hashes are the keccak of the concatenation of the individual hashes | |
| - The hash included on the Reality.eth question is the Proposal safe hash | |
| The following automatic checks have PASSED: | |
| - The proposal Safes hashes are the keccak of the concatenation of the individual hashes | |
| - The hash included on the Reality.eth question is the Proposal safe hash |
🤖 Prompt for AI Agents
In templates/email/proposal-valid-plain.ejs around lines 12 to 14, the sentence
"The following automatic checks has PASSED:" uses the singular verb "has" with
plural subject "checks"; change it to "The following automatic checks have
PASSED:" so the verb agrees with the plural subject (replace "has" with "have").
There was a problem hiding this comment.
Noted. I will fix this!
There was a problem hiding this comment.
@fcanela, thank you for confirming! I appreciate you addressing the grammar issue. 👍
| The following automatic checks has PASSED: | ||
| - The proposal Safes hashes are the keccak of the concatenation of the individual hashes | ||
| - The hash included on the Reality.eth question is the Proposal safe hash |
There was a problem hiding this comment.
Fix grammar: "checks has" → "checks have"
The subject "checks" is plural and requires the plural verb form "have".
Apply this diff:
-The following automatic checks has PASSED:
+The following automatic checks have PASSED:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The following automatic checks has PASSED: | |
| - The proposal Safes hashes are the keccak of the concatenation of the individual hashes | |
| - The hash included on the Reality.eth question is the Proposal safe hash | |
| The following automatic checks have PASSED: | |
| - The proposal Safes hashes are the keccak of the concatenation of the individual hashes | |
| - The hash included on the Reality.eth question is the Proposal safe hash |
🤖 Prompt for AI Agents
templates/telegram/proposal-valid.ejs lines 10-12: fix subject-verb agreement in
the message by changing "The following automatic checks has PASSED:" to "The
following automatic checks have PASSED:" (update the template text accordingly
so "checks" uses the plural verb "have").
There was a problem hiding this comment.
Noted. I will fix this!
There was a problem hiding this comment.
@fcanela, thank you for confirming! I appreciate you addressing the grammar issue. 🙏
| } | ||
| const safes = proposal.plugins?.safeSnap?.safes; | ||
| if (!safes || safes.length < 1) { | ||
| throw new SafeSnapPluginConfigurationError(); |
There was a problem hiding this comment.
I scraped all the SafeSnap proposals for inspection to see what could go wrong. I noticed some of them had a plugins.safeSnap field but set to null.
| operation: Number(operation), | ||
| }); | ||
|
|
||
| if (hash != calculatedHash) throw new TxHashMismatchError(hash, calculatedHash); |
There was a problem hiding this comment.
This kind of error shouldn't happen, but as we are using Snapshot GraphQL instead of an on-chain contract, I validated the integrity of the result.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/processing.ts (1)
282-304: Guard against mismatched proposal IDs before persisting.Line 288 currently trusts
event.proposalIdwithout confirming it matchesquestion.proposalId. If those ever diverge (upstream bug, tampering, partial sync), we would emit a notification containing one proposalId while validating/persisting another, and we would store a DB row whoseproposalIdno longer corresponds to the validated Snapshot proposal. That is a data-integrity break and undermines the whole validation flow flagged for special review. Please fail fast here (ideally with a dedicated validation error/severity) before callinggetSnapshotProposalFn/insertProposalFn.+ if (event.proposalId !== question.proposalId) { + throw new Error( + `Mismatch between ProposalQuestionCreated.proposalId (${event.proposalId}) and LogNewQuestion.proposalId (${question.proposalId})`, + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/processing.integration-test.ts(3 hunks)src/processing.ts(3 hunks)src/utils/reality-question-validation/eip-712-transaction-hash.test.ts(1 hunks)src/utils/reality-question-validation/eip-712-transaction-hash.ts(1 hunks)src/utils/reality-question-validation/errors.ts(1 hunks)src/utils/reality-question-validation/index.test.ts(1 hunks)src/utils/reality-question-validation/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/utils/reality-question-validation/eip-712-transaction-hash.test.ts (1)
src/utils/reality-question-validation/eip-712-transaction-hash.ts (1)
calculateTxHash(36-43)
src/utils/reality-question-validation/index.test.ts (4)
src/services/reality/index.ts (1)
LogNewQuestion(174-187)src/services/snapshot/index.ts (1)
SnapshotProposal(76-76)src/utils/reality-question-validation/index.ts (1)
assertValidRealityQuestion(30-72)src/utils/reality-question-validation/errors.ts (6)
TxHashMismatchError(40-47)SafeHashCalculationError(49-56)SafeHashMismatchError(67-74)SafeSnapPluginConfigurationError(31-38)ProposalIdMismatchError(23-30)SafeNotFoundForProposalNetworkError(58-65)
src/processing.integration-test.ts (10)
src/services/db/schema.ts (2)
space(3-7)proposal(24-44)src/types.ts (1)
Space(3-7)src/utils/test-mocks.ts (1)
randomizeSpace(21-27)src/utils/tests-setup.ts (2)
ONEINCH_ORACLE_ADDRESS(41-41)expect(9-9)src/services/db/spaces.ts (2)
insertSpaces(30-33)removeSpaceByEns(59-62)src/services/db/proposals.ts (3)
removeProposalByQuestionId(57-61)insertProposal(24-28)findProposalByQuestionId(39-47)src/services/reality/index.ts (1)
getLogNewQuestion(204-253)src/utils/reality-question-validation/index.ts (2)
validateRealityQuestion(98-112)ValidationResult(85-85)src/services/snapshot/index.ts (1)
getProposal(78-92)src/notify.ts (1)
Notification(47-47)
src/utils/reality-question-validation/index.ts (4)
src/services/reality/index.ts (1)
LogNewQuestion(174-187)src/services/snapshot/index.ts (1)
SnapshotProposal(76-76)src/utils/reality-question-validation/errors.ts (7)
ProposalIdMismatchError(23-30)SafeSnapPluginConfigurationError(31-38)TxHashMismatchError(40-47)SafeHashCalculationError(49-56)SafeNotFoundForProposalNetworkError(58-65)SafeHashMismatchError(67-74)BaseProposalValidationError(14-21)src/utils/reality-question-validation/eip-712-transaction-hash.ts (1)
calculateTxHash(36-43)
src/processing.ts (8)
src/services/snapshot/index.ts (1)
getProposal(78-92)src/utils/reality-question-validation/index.ts (2)
validateRealityQuestion(98-112)ValidationResult(85-85)src/notify.ts (4)
notify(58-65)Notification(47-47)ValidProposalNotification(29-33)InvalidProposalNotification(35-40)src/services/db/proposals.ts (1)
insertProposal(24-28)src/services/db/schema.ts (3)
space(3-7)notification(11-22)proposal(24-44)src/types.ts (1)
Space(3-7)src/services/reality/index.ts (3)
ProposalQuestionCreated(118-125)LogNewQuestion(174-187)getLogNewQuestion(204-253)src/utils/errors.ts (1)
MissingSnapshotProposalError(12-19)
| if (calculatedSafeHash != hash) throw new SafeHashCalculationError(hash, calculatedSafeHash); | ||
| }); | ||
|
|
||
| const proposalNetworkSafe = safes.find((safe) => safe.network === proposal.network); |
There was a problem hiding this comment.
I have a question here.
After scrapping all the proposals and investigating some edge cases, I noticed that there are proposals that have multiple safes. When that happens, the safes are in different networks.
My understanding is that in those cases with multiple safes, the Reality question contains the hash of the safe in the proposal network and that is the one to compare against.
I lack knowledge here to understand how that multi-safe setup works and if there is any extra validation required. A quick answer indicating if I should pursue the topic further will suffice.
There was a problem hiding this comment.
These templates are placeholder. Feel free to indicate if you want to change anything. I tried to make the message alerting, which the alert as quickly noticeable as possible.
There was a problem hiding this comment.
These files are just for testing, it validates that the values are reaching the templates
Replaces the generic notification for a new Reality question with the following two:
The validation logic resides in
src/utils/reality-question-validation/index.ts, so we may want to review that file with special attention.Summary by CodeRabbit
New Features
Configuration
Chores