Rename from introspector to emulator (fixes #89)#90
Conversation
8203eca to
3b4e9ae
Compare
There was a problem hiding this comment.
🔍 Arkana Code Review — introspector → emulator rename
Reviewed full diff (965+/938-), read all changed files for context, checked cross-repo consumers.
Wire Format / Protocol Safety ✅
Confirmed safe. No changes to:
- Packet type byte
0x01 OP_INSPECT*opcode values- Tagged hashes (
ArkScriptHash,ArkWitnessHash,ArkadeTapSighash) - ARK magic bytes
0x41524b - Serialization/deserialization logic
- REST endpoint paths (
/v1/info,/v1/tx, etc.) - Default port 7073
Issues Found
🔴 P0 — Breaking change with no migration: data directory
internal/config/config.go: defaultDatadir changes from AppDataDir("introspector", false) to AppDataDir("emulator", false). Existing deployments will silently lose access to their data on upgrade. Need either:
- A migration that moves/symlinks the old dir, or
- A fallback that checks the old path if the new one doesn't exist
🔴 P0 — Breaking change with no migration: env prefix
internal/config/config.go: viper.SetEnvPrefix("INTROSPECTOR") → "EMULATOR". The new test TestLoadConfigIgnoresOldEnvPrefix explicitly confirms the old prefix is rejected. Every existing deployment's env config breaks silently (defaults kick in instead of erroring). At minimum, add a startup warning if INTROSPECTOR_* vars are detected.
🔴 P0 — No companion PRs for downstream consumers
The gRPC service rename (IntrospectorService → EmulatorService, package introspector.v1 → emulator.v1) and Go module path change break at least 4 repos at compile time:
| Repo | Impact |
|---|---|
| bancod | Go imports of introspector/pkg/*, Docker image ghcr.io/arklabshq/introspector, INTROSPECTOR_* env vars in test/docker-compose.yml and internal/config/config.go |
| fulmine | Go imports in go.mod, pkg/vhtlc/, pkg/swap/ |
| layerzero-usdt0-arkade-demo | Go imports, Docker build context URL github.com/ArkLabsHQ/introspector.git#master, env vars, shell scripts |
| banco | docker-compose.introspector.yml image + env vars, package.json scripts |
These repos will fail to compile/deploy the moment this merges. Companion PRs should be prepared (or at least tracked in an issue) before merging.
🟡 P1 — buf.Dockerfile: duplicate install + sneaked Go version bump
-FROM golang:1.24-alpine3.20 AS builder
+FROM golang:1.25-alpine AS builder
RUN apk add --no-cache git
+RUN go install github.com/bufbuild/buf/cmd/buf@v1.55.1
RUN go install github.com/bufbuild/buf/cmd/buf@v1.55.1- Go base image bumped
1.24-alpine3.20→1.25-alpine— unrelated to the rename, should be a separate commit or called out in the PR description. bufis installed twice (duplicateRUNline). Harmless but sloppy — remove one.
🟢 P2 — Cosmetic: pkg/arkade/psbt_fields.go whitespace change
Alignment padding changed on ArkFieldPrevoutTx line. Nit, no impact.
Rename Completeness ✅
The rename itself is thorough and consistent. All type names, function names, variable names, comments, error messages, module paths, proto definitions, generated code, test files, CI workflows, and Docker configs have been updated. The OP_INSPECT* opcode names are correctly preserved (they refer to the Bitcoin introspection concept, not the project name).
Recommendation
Request changes. The rename itself is clean, but merging without addressing the data directory migration and downstream breakage is dangerous. Specifically:
- Add a migration/fallback for the data directory path change
- Add a startup warning (or fallback) for old
INTROSPECTOR_*env vars - Prepare companion PRs for bancod, fulmine, layerzero-usdt0-arkade-demo, and banco — or at minimum open a tracking issue
- Fix the duplicate
buf installinbuf.Dockerfile - Split or document the Go 1.25 bump
There was a problem hiding this comment.
Follow-up review — new commit 801679b
✅ Fixed since last review
P1 buf.Dockerfile — Fixed. Go version reverted to 1.24-alpine3.20, duplicate buf install removed. Good.
🔴 Still outstanding (P0s from previous review)
The three P0 blockers remain unaddressed:
-
Data directory migration (
internal/config/config.go:29) —AppDataDir("introspector", false)→AppDataDir("emulator", false). Existing deployments silently lose their data on upgrade. Needs a migration or fallback. -
Env prefix breakage (
internal/config/config.go:49) —SetEnvPrefix("INTROSPECTOR")→"EMULATOR". Every existing deployment's env config silently stops working (defaults kick in, no error). At minimum detect and warn onINTROSPECTOR_*vars at startup. -
Downstream consumers — bancod, fulmine, layerzero-usdt0-arkade-demo, and banco all import from the old module path / use the old Docker image / set
INTROSPECTOR_*env vars. They break at compile/deploy time when this merges. Need companion PRs or a tracking issue.
These should be resolved before merge. The rename itself remains clean and thorough.
Summary
introspector/v1/IntrospectorServicetoemulator/v1/EmulatorService, while preserving existing REST endpoint paths.0x01, serialization behavior, orOP_INSPECT...opcode names.EMULATOR_environment prefix and changes the default app data namespace to emulator.