Add missing task-level Solidity events to unblock 5 subgraph handlers #153

Activity

1. robertocarlous

   added

   enhancementNew feature or requestNew feature or request

   on Sep 21, 2026
2. umeradl commented on Sep 24, 2026

   

   umeradl

   on Sep 24, 2026

   Member

   More actions

   Review — go-ahead, but use PR #29's own event spec, fix the scope, and split contract vs. subgraph work

   Checked against develop @ d321c2a and PR #29's head (feat/din-indexer @ 866521a, 2026-07-14).

   The need is real. None of these events exist on develop. The subgraph can't index LMS submissions, T1/T2 submissions, finalization, or GI phase transitions without them. Go-ahead on adding them.

   The issue text doesn't match the spec it's meant to unblock, though, and a few details would produce the wrong events.

   1. The source of truth already exists: follow PR #29's audit §4

   PR #29 ships Documentation/technical/audits/task-contract-event-audit.md. Its §4 proposes exact signatures, and they match the commented-out lines in subgraph/subgraph.yaml. Implement those signatures, not the issue's looser "at minimum GI, batch, actor" wording, so the handlers can be uncommented without an ABI mismatch.

   There are six pending task-level handlers, not five. The issue's table lists only four events:

   Event (audit §4 signature)	Emit from	In issue?
   GIStateChanged(uint indexed GI, uint8 indexed newState)	DINTaskCoordinator, every transition	✅
   LocalModelSubmitted(uint indexed GI, uint indexed modelIndex, address indexed client, bytes32 modelCID)	DINTaskAuditor.submitLocalModel()	✅ (wrong contract)
   T1AggregationSubmitted(uint indexed GI, uint indexed batchId, address indexed aggregator, bytes32 cid)	DINTaskCoordinator.submitT1Aggregation()	✅
   T2AggregationSubmitted(uint indexed GI, uint indexed batchId, address indexed aggregator, bytes32 cid)	DINTaskCoordinator.submitT2Aggregation()	✅
   T1BatchFinalized(uint indexed GI, uint indexed batchId, bytes32 winningCID)	finalizeT1Aggregation(), per batch, after b.finalCID = winningCID (:767)	❌ missing
   T2Finalized(uint indexed GI, bytes32 globalModelCID)	finalizeT2Aggregation(), after b.finalCID = winningCID (:859)	❌ missing

   Without the two finalization events, the indexer sees votes but never the outcome: which CID won each batch, and the new global model per GI. That's arguably the most-queried fact in the whole lifecycle, so both belong in scope.

   2. Corrections to the issue text

   • No. 1: submitLocalModel lives in DINTaskAuditor (:700), not DINTaskCoordinator.
   • No. 2: Declare events in the contracts that emit them, not in DINShared.sol. DINShared.sol holds the enum, errors, and interfaces, and has zero event declarations today. Every existing event is declared in its own contract. Keep that convention.
   • No. 3: The dindao.py pending-requests endpoint isn't affected. It lists model-registration requests (ModelRegistrationRequested on the registry), which are already indexed. What these events do unblock is lms.py/aggregation.py, which iterate submissions over RPC (audit §3.2).

   3. Two correctness details for the implementer

   • GIStateChanged would carry the wrong GI on GIstarted. The audit's _setGIstate(newState) helper emits GIStateChanged(GI, …), and that's the right approach: one emit site, so no future transition can be missed. But _startGI sets GIstate = GIstates.GIstarted before GI++ (DINTaskCoordinator.sol:310-311). A mechanical find-and-replace would emit GIstarted with the previous GI. Increment first, then call the helper. Add a test asserting GIStateChanged(N, GIstarted) for GI N.
   • The enum ordinals in PR feat(indexer): implement DIN Protocol subgraph — platform + task-level contracts #29's docs are stale. The audit's example says newState == 16 is T1AggregationStarted. On develop it's 17, because LMSevaluationRevealStarted was inserted at ordinal 14 on 2026-08-27. GIstates now has 24 members. The subgraph's uint8 → GraphQL enum mapping and daemon-event-schema.md both need regenerating from current DINShared.sol, not from the July snapshot.

   4. Stale item in audit §5

   §5 calls RewardDeposited dead code and proposes deleting it. That's no longer true: DINTaskAuditor.depositRewards emits it (:488), and its signature is now RewardDeposited(uint256 indexed gi, address indexed depositor, uint256 amount), not the (address, uint256) the audit shows. Don't delete it. Add a subgraph handler for it instead, since reward-pool funding per GI is useful to index.

   5. Split the work: contracts on develop, handlers on feat/din-indexer

   PR #29 is still open and targets feat/din-indexer, not develop. There's no subgraph/ directory on develop at all. So "re-enable the five handlers" can't be part of a develop PR.

   • PR A → develop:
     • the six events + emit sites + _setGIstate refactor
     • Forge tests using vm.expectEmit for each event, including the GIstarted-carries-new-GI case and one T1BatchFinalized per batch in a multi-batch GI
     • refreshed dincli/abis/
     • update audit §4/§5 if it lands on develop with the subgraph work, or note the corrections in the PR
     • check forge build --sizes: DINTaskCoordinator is large, and a 24-transition refactor plus five new events isn't free
   • PR B → feat/din-indexer (after PR A merges):
     • rebase/refresh the branch on develop
     • regenerate subgraph/abis/
     • uncomment the six handlers, fix the enum mapping, add the RewardDeposited handler
     • ETHTreasuryWithdrawn (DinCoordinator, platform-level) is also still commented out in subgraph.yaml and doesn't exist on develop. Either drop that handler stub, or repoint it at the events DinTreasury actually emits (EthWithdrawn/ERC20Withdrawn/EthReceived) with a DinTreasury data source. It shouldn't wait on a DinCoordinator event that isn't planned.

   Revised acceptance criteria

   • Six events declared in their emitting contracts, with signatures exactly as in audit §4
   • _setGIstate helper used for every GIstate assignment; GIstarted emits the new GI
   • vm.expectEmit tests for all six, plus the GI-ordering case; full forge test green; contract size within limit
   • dincli/abis/ refreshed
   • Follow-up PR on feat/din-indexer: ABIs regenerated, six handlers enabled, enum mapping regenerated from current GIstates, RewardDeposited handler added, ETHTreasuryWithdrawn stub resolved

   Go-ahead with this scope.
3. umeradl

   added a commit that references this issue on Sep 24, 2026

   docs(tasks): add task_240926_16 — dispute seed, treasury forwarding, …

   ...

   5762556
4. umeradl commented on Sep 27, 2026

   

   umeradl

   on Sep 27, 2026

   Member

   More actions

   Closing — develop half landed via PR No. 173; subgraph half continues on PR No. 29

   Landed on develop in e9dc3f0 (merge of PR No. 173) + 04aa58d (review follow-ups); push CI green (run). Verified against origin/develop @ 04aa58d, per the revised acceptance criteria:

   • Six events declared in their emitting contracts, signatures as audit §4. DINTaskCoordinator: GIStateChanged, T1AggregationSubmitted, T2AggregationSubmitted, T1BatchFinalized, T2Finalized; DINTaskAuditor: LocalModelSubmitted (in submitLocalModel). DINShared.sol has no event declarations. Checked against the six commented-out handler lines in PR No. 29's subgraph/subgraph.yaml — canonical types, indexed flags and parameter order match exactly, so the handlers can be uncommented without an ABI mismatch.
   • _setGIstate for every GIstate assignment; GIstarted emits the new GI. 24 call sites; the only bare GIstate = is inside the helper. startGI does GI++ before _setGIstate(GIstates.GIstarted).
   • vm.expectEmit tests for all six + GI ordering; full suite green; size within limit. foundry/test/LifecycleEvents.t.sol (9 tests), including GIStateChanged(N, GIstarted) for GI 1 and 2, one T1BatchFinalized per batch in a two-batch GI, and a full GIstarted → GIended walk — each mutation-checked (the tests fail with the ordering fix reverted / emission limited to batch 0). forge test 392/392; DINTaskCoordinator runtime 20,629 → 21,335 B, 3,241 B under EIP-170.
   • dincli/abis/ refreshed. Both regenerated via dincli system dump-abi --official: coordinator +5 events, auditor +1, nothing removed.
   • Follow-up on feat/din-indexer (ABIs regenerated, six handlers enabled, enum mapping regenerated from current GIstates, RewardDeposited handler, ETHTreasuryWithdrawn stub resolved) — not done yet; it's now unblocked. Tracked as task_240926_16 C0/C5 on PR No. 29 (still at 866521a) and in Discussion No. 163, per the planned split (this issue closes with the develop half).

   The issue body's original four-event / DINShared.sol / DINTaskCoordinator.submitLocalModel wording is superseded by the review comment linked above; what landed follows the review.
5. umeradl

   closed this as completedon Sep 27, 2026