Skip to content

fix: event filter dropped all events — GraphQL renders TypeName as plain string - #42

Merged
jangid merged 2 commits into
mainfrom
bugfix/graphql-typename-string
Jul 29, 2026
Merged

fix: event filter dropped all events — GraphQL renders TypeName as plain string#42
jangid merged 2 commits into
mainfrom
bugfix/graphql-typename-string

Conversation

@11felix

@11felix 11felix commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

The GraphQL migration kept the JSON-RPC-era typename: {name} shape, but GraphQL's contents.json renders Move's TypeName as a plain string. The filter in events.ts read .typename.nameundefined, so every event was silently dropped: fetchStSuiAPR/fetchStSuiAPY returned 0 and getMintEvents/updateTotalStakers found nothing.

Fixes the filter and corrects the four event types to match the actual runtime shape.

Verified against live GraphQL: fetchStSuiAPR(7) now returns the same APR as the pre-shutdown JSON-RPC path (17-digit match). tsc/lint/tests clean.

Needed before publishing 2.0.3.

@11felix
11felix requested review from Zorag44 and jangid July 29, 2026 08:10
@jangid

jangid commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

operate.md review — PR #42 (bugfix/graphql-typename-string)

Fix verified correct against the current HEAD. queryMoveEvents maps
contents.json straight into parsedJson (src/common/blockchain.ts:481), so the
event objects handed to the filter carry GraphQL's rendering, not the JSON-RPC
{name} wrap — events.ts:71 was comparing "0x" + undefined, dropping every
event unconditionally. The type edits in types.ts cover all four event structs,
and EventType/getEvents<T> stay consistent. No other .typename reader exists
in the repo, and no consumer in the metarepo reads .typename.name, so the
type narrowing is inert for downstream TypeScript. CI green (lint/test/build).

🟡 No regression guard for the failure mode this PR fixes.
__tests__/getAprs.test.ts asserts only expect(apr).toBeDefined() — and
fetchStSuiAPR returns the string "0" both when the filter matches nothing and
when it throws (utils.ts catch → return "0"). That is exactly why a 100% event
drop shipped with a green suite; the same test will stay green if the shape drifts
again. Worth adding in this PR (or a follow-up) one of:

  • assert a non-empty/non-zero result, e.g. expect(Number(apr)).toBeGreaterThan(0)
    for fetchStSuiAPR(7), and likewise a non-empty getMintEvents window; or
  • a unit test on Events.getEvents with a fixture parsedJson (mocking
    queryMoveEvents), asserting a matching typename is kept and a
    non-matching one is dropped — this also pins the "0x" + prefix contract.

scripts/migration-snapshot.ts snapshots the three event queries but records
whatever it gets, so an all-empty GraphQL result serialized as a valid snapshot;
a non-empty assertion there would close the same gap for future migrations.

🟢 Non-blocking, FYI: the filter still fails silently — a future rendering
change degrades to "APR 0" rather than an error. If these numbers are surfaced to
users, a guard (log/throw when a full page of events matches the queried
eventType but zero pass the typename filter) would turn the next occurrence
into a visible failure instead of a silent zero.

Security: secrets-in-diff scan clean; not a contract PR (no *.move / Move.toml).


Update (same run): the 🟡 regression guard was added to this PR at the request of
the maintainers — __tests__/eventFilter.test.ts (commit 425e0b3). It mocks
queryMoveEvents and asserts the filter keeps matching events, drops non-matching
ones, and drops the legacy {name} shape. Verified to fail against the pre-fix
comparison (all three cases) and pass against this branch; lint/test/build green on
425e0b3.

Because that commit is mine, this PR now needs two reviewers other than me
my earlier approval was auto-dismissed by the new push and I am not re-approving
code I wrote. The 🟢 silent-failure note above remains open as a follow-up
suggestion, not a blocker.

jangid
jangid previously approved these changes Jul 29, 2026

@jangid jangid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operate.md: clean — fix verified against current HEAD (GraphQL contents.json renders TypeName as a plain string; all four event types updated consistently, no other .typename readers in repo or downstream). CI green (lint/test/build). Non-blocking note in the review comment: the existing APR test only asserts toBeDefined(), so a total event drop passes CI — worth a non-empty/non-zero assertion or a fixture unit test on the filter. Approvals: 1/2 — needs 1 more before merge.

The typename shape change silently dropped every event while the suite
stayed green — the live tests only assert toBeDefined() and the callers
swallow an empty result as "0". Mock the GraphQL layer and assert the
filter keeps matching events, drops non-matching ones, and drops the
legacy {name} shape, so the next rendering change fails CI instead of
zeroing out APR in production.

@jangid jangid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at head 425e0b3. Fix verified against the current branch: queryMoveEvents passes GraphQL's contents.json straight through as parsedJson, so the old .typename.name read evaluated "0x" + undefined and dropped every event; all four event types are updated consistently, and no other reader of .typename exists in this repo or in downstream consumers. CI green (lint/test/build).

Note on provenance: the regression test in 425e0b3 was added by this review pass at the maintainers' request, so this approval covers a commit authored during review — flagging it rather than leaving it implicit. The fix commit f5a592c is the author's.

Approvals: 1/2 — needs one more before merge.

@jangid
jangid merged commit 5765255 into main Jul 29, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants