Update sync for active proposals: votes and proposal updates#40
Update sync for active proposals: votes and proposal updates#40gov-dmitry wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the “active proposals” syncing logic to react to Snapshot message activity (votes/proposal messages) and refines which proposals are considered eligible for refresh.
Changes:
- Extend the “spaces with updates” finder to support proposal-update messages (in addition to vote messages).
- Rework the active proposals worker to fetch/update proposals for spaces with recent proposal-update messages, in paged batches.
- Tighten the DB query for “proposals eligible for update” to only consider recently-ended proposals (last 7 days).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/updates/votes.go | Extends the spaces-finder interface; adjusts proposal-selection interval in active vote sync. |
| internal/updates/active_proposals.go | Switches active proposal updates to be driven by recent proposal-update messages per space. |
| internal/db/proposal.go | Narrows ended-proposal update eligibility to proposals ended within the last 7 days. |
| internal/db/message.go | Adds repo/service support for “spaces with proposal updates” and factors shared query logic. |
| internal/app.go | Wires messagesService into ActiveProposalsWorker. |
| CHANGELOG.md | Notes the behavior change under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| func (w *ActiveProposalsWorker) loop(ctx context.Context) error { | ||
| ids, err := w.proposals.GetProposalIDsForUpdate(nil, gap, proposalsPerRequest, false) | ||
| spaces, err := w.unrpocessedSpaces.FindSpacesWithProposalsUpdates(time.Now().Add(-gapMessages)) |
There was a problem hiding this comment.
Using time.Now().Add(-gapMessages) as a fixed lookback window means proposal updates can be silently missed if this worker is down longer than gapMessages (e.g., restart/outage). Consider tracking a persisted watermark (last processed message timestamp/MCI) or otherwise ensuring the query covers all unprocessed updates.
| spaces, err := w.unrpocessedSpaces.FindSpacesWithProposalsUpdates(time.Now().Add(-gapMessages)) | |
| spaces, err := w.unrpocessedSpaces.FindSpacesWithProposalsUpdates(time.Time{}) |
| spaces, err := w.unrpocessedSpaces.FindSpacesWithProposalsUpdates(time.Now().Add(-gapMessages)) | ||
| if err != nil { | ||
| return err | ||
| return fmt.Errorf("get paces with new votes: %w", err) |
There was a problem hiding this comment.
Error message text is misleading and contains typos: this path is fetching spaces with proposal updates, but the message says "paces" and "new votes". Update the message so logs/errors accurately describe the failure source.
| return fmt.Errorf("get paces with new votes: %w", err) | |
| return fmt.Errorf("get spaces with proposal updates: %w", err) |
| } | ||
|
|
||
| ids, err := w.proposals.GetProposalIDsForUpdate(spaces, gap, proposalsPerRequest, true) | ||
| ids, err := w.proposals.GetProposalIDsForUpdate(spaces, 0, proposalsPerRequest, true) |
There was a problem hiding this comment.
Passing interval=0 to GetProposalIDsForUpdate makes the updated_at < now() filter effectively match almost all proposals in the selected spaces, which can cause this loop to select random proposals and issue many unnecessary Snapshot API calls (often returning no new votes). Use a non-zero interval (or a dedicated constant for active-vote sync) so only stale proposals are revisited.
| ids, err := w.proposals.GetProposalIDsForUpdate(spaces, 0, proposalsPerRequest, true) | |
| ids, err := w.proposals.GetProposalIDsForUpdate(spaces, votesCreatedAtGap, proposalsPerRequest, true) |
| Where(r.conn. | ||
| Where("to_timestamp((snapshot->'start')::double precision) <= now() and to_timestamp((snapshot->'end')::double precision) >= now()"). | ||
| Or("updated_at < to_timestamp((snapshot->'end')::double precision) and to_timestamp((snapshot->'end')::double precision) < now()"), | ||
| Or("updated_at < to_timestamp((snapshot->'end')::double precision) AND to_timestamp((snapshot->'end')::double precision) < now() AND to_timestamp((snapshot->'end')::double precision) > now() - interval '7 days'"), | ||
| ). |
There was a problem hiding this comment.
The new SQL condition hard-codes a 7-day cutoff (interval '7 days'). This magic value isn’t connected to the interval argument of GetProposalIDsForUpdate, making behavior harder to reason about and tune. Consider extracting it as a named constant/config (or parameterizing it) so callers can control the retention window explicitly.
| checkInterval time.Duration | ||
| unrpocessedSpaces unprocessedSpacesFinder | ||
| } | ||
|
|
||
| func NewActiveProposalsWorker(sdk *snapshot.SDK, proposals *db.ProposalService, checkInterval time.Duration) *ActiveProposalsWorker { | ||
| func NewActiveProposalsWorker(sdk *snapshot.SDK, proposals *db.ProposalService, unrpocessedSpaces unprocessedSpacesFinder, checkInterval time.Duration) *ActiveProposalsWorker { | ||
| return &ActiveProposalsWorker{ | ||
| sdk: sdk, | ||
| proposals: proposals, | ||
|
|
||
| checkInterval: checkInterval, | ||
| sdk: sdk, | ||
| proposals: proposals, | ||
| unrpocessedSpaces: unrpocessedSpaces, | ||
| checkInterval: checkInterval, |
There was a problem hiding this comment.
Field/parameter name unrpocessedSpaces appears to be a misspelling (likely unprocessedSpaces). Since this worker just introduced this dependency, consider correcting the identifier now to avoid propagating the typo across the codebase (and keep it consistent with unprocessedSpacesFinder).
No description provided.