fix(noter): gRPC migration, auth race fixes, e2e suite + CI - #684
Open
harrymove-ctrl wants to merge 4 commits into
Open
fix(noter): gRPC migration, auth race fixes, e2e suite + CI#684harrymove-ctrl wants to merge 4 commits into
harrymove-ctrl wants to merge 4 commits into
Conversation
…the delegate-key error message
sessionAtom was atomWithStorage(..., { getOnInit: false }), so every fresh
page load (a window.location.href redirect after login, or a plain reload of
/note) rendered once with session=null before the async post-mount hydration
from sessionStorage caught up. useAuth's own effect read that stale null on
the same tick, concluded "logged out", and cleared isLoading before the real
session arrived, so /note's guard fired router.replace("/") on an already
authenticated user. getOnInit is safe to flip to true here: nothing renders
`session` directly, every page branches on the separate authAtom instead,
which always starts isLoading:true on both server and client, so there is no
markup for the eager read to mismatch against.
Separately, connectEnoki/connectDelegateKey flipped the global authAtom.isLoading
flag for the duration of their mutation, and app/page.tsx renders
<AuthButtonGroup /> only while !isAuthenticated && !isLoading. That unmounted
the login form mid-submit, taking its local `error` state with it before the
catch block's setError() could run, so an invalid delegate key just dropped
the user back on a silently collapsed form. isLoginPending (from the mutation
hooks themselves) already tracks per-call pending state for the submit
button, so the global flag no longer needs to move during a login attempt.
…cated Sui JSON-RPC Sui's public JSON-RPC fullnodes were deprecated in 2026; fullnode.testnet.sui.io now answers every JSON-RPC call with "Method not found ... migrate to gRPC or GraphQL endpoints" and no CORS header, which Chrome reports as a generic "blocked by CORS policy" failure. sui-providers.tsx's getJsonRpcFullnodeUrl and enoki-login-card.tsx's useSuiClient() both hit that dead endpoint, so registerEnokiWallets failed on mount ([enoki-login] Setup failed: TypeError: Failed to fetch) and Google sign-in never got past the landing page. dapp-kit's SuiClientProvider is still hard-typed to SuiJsonRpcClient even in the latest published version (1.1.17), so it can't be pointed at a gRPC client directly. Enoki's own `client` option and Transaction.build()'s `client` option both accept the broader ClientWithCoreApi interface instead, which SuiGrpcClient satisfies, so this bypasses SuiClientProvider only where it was actually blocking things: registerEnokiWallets now gets a standalone SuiGrpcClient (lib/sui/grpc-client.ts) instead of useSuiClientContext()'s client, and enoki-login-card.tsx's on-chain reads (registry lookup, dynamic field, transaction/event fetch) move to the gRPC client's include/mask-based API. SuiClientProvider itself stays in place for WalletProvider's wallet-standard connect/sign, which doesn't touch RPC directly. gRPC object/dynamic-field/event reads return raw BCS bytes instead of JSON-RPC's parsed `.fields`, so lib/sui/account-bcs.ts adds the BCS schemas needed to decode them, verified by decoding a live testnet account and matching its stored delegate public key, and round-tripping a registry dynamic-field lookup back to the same account id.
Noter had zero automated tests; the on-chain registration flow, delegate-key auth, and note CRUD were verified entirely by hand. 22 specs across app shell, auth, note lifecycle, and the memory API contract, running against a real Next.js dev server and a fresh Postgres. Mock seam for the new delegate-account binding check ------------------------------------------------------ connectDelegateKey now calls assertDelegateAccountBinding (a separate, already-merged change), which reads the claimed account off-chain via gRPC and rejects any key that isn't registered in its delegate_keys list — so a random, never-registered key/account pair can no longer reach an authenticated session the way it could before that change landed. Mirroring researcher's PR #680 pattern, delegate-account.ts now branches on lib/constants.ts's isTestEnvironment (set by playwright.config.ts passing PLAYWRIGHT=True to the webServer) and serves a fixture object from delegate-account.mock.ts instead of the gRPC read, so the real validation logic still runs meaningfully: an unknown account or an unregistered key fails the exact same way it would on-chain. Noter authenticates a fresh identity per test rather than reusing two shared identities across a whole run (researcher's approach) — with `workers: 2` and ~15 login call sites, two fixed identities would have concurrent tests collide on each other's notes. delegate-account.mock.ts and fixtures/delegate-key.ts instead generate the same 24-entry deterministic pool independently (index N -> accountId byte N repeated, privateKey byte N+0x40 repeated), and the test fixture hands out a never-yet-used entry per call, interleaved by Playwright's parallelIndex so two worker processes never claim the same one. public_key is stored base64, not hex: the binding check's parser tries fromBase64() before falling back to raw hex, and every 64-char hex string (alphabet 0-9a-f, always length-divisible-by-4) also happens to be valid-but-wrong base64, so a hex value there silently decodes to the wrong bytes instead of ever matching. The memory-write specs assert against the real relayer response for a fixture (unregistered) key, so there's no live-Walrus canary in this suite by design, same as #680 documents for researcher: the real remember -> recall round trip against production Walrus Memory stays a manual check. CI job ------ noter-e2e mirrors chatbot-e2e's shape (Postgres service container, cached Playwright browsers, report/trace upload on failure). noter-checks adds tsc --noEmit and a full `next build` so a type or build regression fails CI even on a change the e2e specs don't happen to cover.
Two separate PRs each added a job named noter-checks to the same workflow file — one from an already-merged auth-hardening change (vitest unit tests), one from this branch (tsc + next build). Neither touched the same lines, so the merge went through cleanly with no conflict markers, but the resulting file had two top-level jobs with the identical key. GitHub Actions rejects that outright: the workflow run failed in 0s with no job output at all, before any check even started, which is why this wasn't caught by the local `tsc`/`next build`/e2e verification — those ran the commands directly, never through the YAML that CI actually parses. python's yaml.safe_load didn't catch it locally either; it silently keeps the last duplicate key rather than erroring, which GitHub's stricter workflow parser does not do. Folded the tsc/build steps into the existing job instead of renaming to avoid the collision — one Noter CI job now covers unit tests, type-checking, and the production build, sharing one checkout/install/SDK-build sequence rather than paying for it twice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the "fix noter" half of WALM-355. Three fixes plus a 22-spec Playwright suite and CI job, mirroring #680's shape for researcher.
No test coverage existed for noter at all before this — the on-chain registration flow, delegate-key auth, and note CRUD were verified entirely by hand.
Bugs fixed
Google sign-in failing with
[enoki-login] Setup failed: TypeError: Failed to fetch. Sui's public JSON-RPC fullnodes were deprecated in 2026;fullnode.testnet.sui.ionow answers every JSON-RPC call withMethod not found ... migrate to gRPC or GraphQL endpointsand no CORS header, which Chrome reports as a generic CORS failure.sui-providers.tsxandenoki-login-card.tsxmigrate toSuiGrpcClient, bypassing dapp-kit'sSuiClientProvider(still hard-typed toSuiJsonRpcClienteven in the latest published version) only where it was actually blocking things —WalletProvider's wallet-standard connect/sign is untouched. Verified by decoding a live testnetMemWalAccountand matching its stored delegate public key, and round-tripping a registry dynamic-field lookup back to the same account id.An authenticated user got bounced back to
/instead of landing on/note.sessionAtomwasatomWithStorage(..., { getOnInit: false }), so a fresh page load (the post-loginwindow.location.hrefredirect, or a plain reload of/note) rendered once withsession=nullbefore the async post-mount hydration caught up —useAuth's effect read that stale null and concluded "logged out" before the real session arrived.getOnInit: trueis safe here since nothing renderssessiondirectly; every page branches on the separateauthAtom, which always startsisLoading:trueon both server and client.An invalid delegate key silently collapsed the login form with no error shown.
connectEnoki/connectDelegateKeyflipped the globalauthAtom.isLoadingfor the duration of their mutation, andapp/page.tsxrenders<AuthButtonGroup />only while!isAuthenticated && !isLoading— unmounting the form mid-submit and taking its localerrorstate with it beforesetError()could run.isLoginPending(from the mutation hooks themselves) already tracks per-call pending state for the submit button, so the global flag no longer needs to move during login.Test suite
22 specs across app shell, auth, note lifecycle, and the memory API contract, running against a real dev server and fresh Postgres.
Mock seam for the delegate-account binding check (already-merged, separate change):
connectDelegateKeynow callsassertDelegateAccountBinding, which reads the claimed account off-chain via gRPC and rejects any key not registered in itsdelegate_keyslist — a random key/account pair can no longer reach an authenticated session. Mirroring #680's pattern,delegate-account.tsbranches onisTestEnvironment(set byPLAYWRIGHT=True) and serves a fixture fromdelegate-account.mock.tsinstead of the gRPC read, so the real validation logic still runs meaningfully.Noter authenticates a fresh identity per test rather than reusing two shared identities (researcher's approach) — with
workers: 2and ~15 login call sites, two fixed identities would have concurrent tests collide on each other's notes. The mock and the test fixture instead generate the same 24-entry deterministic pool independently, and the fixture hands out a never-yet-used entry per call, interleaved by Playwright'sparallelIndexso two workers never claim the same one.One non-obvious bug found while wiring the mock:
public_keyhas to be base64, not hex — the binding check's parser triesfromBase64()first, and every 64-char hex string is also valid-but-wrong base64 (alphabet0-9a-fis a subset of base64's, length is always divisible by 4), so a hex value there silently decodes to the wrong bytes instead of ever matching.No live-Walrus canary in this suite by design, same as #680 documents for researcher —
isTestEnvironmentis unconditionally true for every Playwright run, so even real, on-chain-registered credentials would fail the binding check's mock path at login. The real remember → recall round trip against production Walrus Memory stays a manual check.CI
noter-e2emirrorschatbot-e2e's shape (Postgres service container, cached Playwright browsers, report/trace upload on failure).noter-checksaddstsc --noEmitand a fullnext build.Verification
.env.local, fresh Postgres, no real credentials) — matching what CI will actually runtsc --noEmitclean,next buildclean, both with placeholder-only envdev; this branch had gone stale mid-session and needed reconciling against unrelated auth-hardening work (19434b2d,89f22073) that landed in the meantime — the delegate-account binding check and the removal of the memory-route server-credential fallback both came from that, not from this PRNotes
services/contract/sources/account.move's source has grown migration/import fields (AccountRegistry.migration_finalized,pinned_allowlist_root, etc.;MemWalAccount.admin_quarantined,legacy_account_id, etc.) beyond what's currently deployed at the package this app points at.lib/sui/account-bcs.ts's schemas intentionally decode only the leading fields this app reads and rely on the BCS parser stopping there — verified correct against the live, currently-deployed bytecode, but will need updating if/when that contract version is published to this package.