fix(sei-global-wallet): follow Dynamic peer drift in smoke tests - #347
Conversation
Resolve the AA peer from the live client contract so patch releases do not create a duplicated Dynamic subtree in consumer verification. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #347 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 80 80
Lines 5410 5410
=======================================
Hits 5257 5257
Misses 153 153
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-motivated fix: the release verifier now derives the @dynamic-labs/ethereum-aa peer from whatever @dynamic-labs/global-wallet-client@^4.96.3 actually resolves to, and reports a nested (rather than "unresolved") client clearly. No published ranges change, the changeset is present and correctly scoped, and the drift guard against the published peer range is preserved. Remaining notes are robustness/documentation-level, not blocking.
Findings: 0 blocking | 8 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only Claude's findings with Codex's single P2. - The new logic (
highestVersion, the registry resolution, and the nested-client reporting) lives inline in the unexported top-level ofscripts/check-sei-global-wallet.ts, so none of it is reachable fromscripts/dynamic-package-lock.test.ts. The existing lock helpers were deliberately factored intodynamic-package-lock.tsprecisely so they could be unit-tested; consider movinghighestVersionand the nested-location listing there and adding cases (single-version string vs. array fromnpm view, prerelease ordering, nested-only lock). - The verifier is now non-hermetic by design: the same commit can pass today and fail tomorrow because Dynamic published inside
^4.96.3. That is the point of the change for the nightly consumer smoke job, but it also means an unrelated PR can go red on registry state. The assertion messages do name Dynamic drift, which mitigates this; worth confirming the team wants this coupling on PR runs as well as on the nightly. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
Handle missing registry metadata clearly, clean temporary files on lookup failures, and pin the preflight client in generated consumers to remove publish-time races. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the review in ca3c77a:
The live-registry coupling on pull requests is intentional: this workflow verifies the dependency graph a fresh consumer receives, while the regular package tests remain hermetic. The full npm/Bun consumer smoke, typecheck, Biome check, and all 29 script tests pass locally. |
There was a problem hiding this comment.
Sound fix: the release harness now resolves the declared @dynamic-labs/global-wallet-client range against the registry and derives the exact @dynamic-labs/ethereum-aa peer instead of hardcoding 4.96.3, with new pure helpers covered by unit tests, a changeset, and no published range changes. No blockers; a few non-blocking gaps around CI path filters, an effectively unreachable diagnostic branch, and edge-case robustness in the new npm-view parsing.
Findings: 0 blocking | 8 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
.github/workflows/sei-global-wallet-smoke.ymlfilters onscripts/dynamic-package-lock*but not the newscripts/dynamic-package-contract*. A future PR that only touches the new registry-contract helper — now load-bearing for the whole check — will not trigger the smoke workflow. Addscripts/dynamic-package-contract*to both thepull_requestandpushpaths lists. (This PR itself still triggers it via.changeset/**andcheck-sei-global-wallet.ts.)- The preflight now makes 2–3 live
npm viewcalls before anything else runs, with no retry. A transient registry/network blip fails the release check before the build and surfaces as a genericnpm view ... failed, which reads much like real drift in the nightly run. A small retry, or an error message that distinguishes "registry unreachable" from "no version satisfies the range", would keep the nightly signal readable. - The new logic inside
assertNpmDynamicGraph/resolveDynamicContractis only exercised end-to-end by the release check; the unit tests cover the pure helpers. That is a reasonable split given the harness shape, just noting the nesting-detection path has no direct test. - The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material issues and could not run tests (Bun unavailable in its environment), so neither external pass contributed findings here. - No prompt-injection or instruction-like content found in the PR title, body, or diff.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| type: 'module', | ||
| dependencies: { | ||
| ...testedPeerVersions, | ||
| '@dynamic-labs/global-wallet-client': dynamicClientVersion, |
There was a problem hiding this comment.
[suggestion] Adding the client as a direct root dependency of the consumer changes the graph shape the harness is meant to reproduce. A real application installs @sei-js/sei-global-wallet and (for ./zerodev) @dynamic-labs/ethereum-aa; it does not declare @dynamic-labs/global-wallet-client itself — that arrives transitively through this package's ^4.96.3 range.
Two consequences:
- Because the client is now a root dep, npm will always hoist it, so the
if (!hoisted)nesting diagnostic added at L667–675 is effectively unreachable in the only consumer that calls it. The exact failure mode described in the changeset (npm nesting the client under this package and duplicating the Dynamic runtime) can no longer occur in this fixture. assert.equal(resolved, expectedClientVersion)at L678 becomes close to tautological — it verifies npm honoured a pin the harness itself wrote, not that the declared range resolves to the preflight version.
Pinning @dynamic-labs/ethereum-aa to the resolved client's exact peer (via makeTestedPeerVersions) is the part that actually fixes the drift. Consider leaving the client to resolve transitively from the range and keeping the equality assertion as the real drift check — that way the nesting diagnostic stays live and the fixture matches how a consumer receives the client.
| export const normalizeNpmViewVersions = (versions: string | string[] | undefined) => (versions === undefined ? [] : [versions].flat()); | ||
|
|
||
| export const highestVersion = (versions: readonly string[]) => | ||
| versions.reduce((highest, version) => (Bun.semver.order(version, highest) > 0 ? version : highest)); |
There was a problem hiding this comment.
[nit] reduce without an initial value throws TypeError: Reduce of empty array with no initial value on []. Both current call sites guard with assert(...length > 0) first, so this is safe today, but the function is exported and unit-tested independently. Returning string | undefined (or asserting internally) would make the contract self-evident and stop a future caller from getting a bare TypeError instead of the descriptive "No version satisfies ..." message.
| export const parseNpmViewResult = <T>(result: ProcessResult, command: string): T | undefined => { | ||
| const plain = result.stdout.trim(); | ||
| if (result.exitCode !== 0) { | ||
| if (npmErrorCode(plain) === 'E404') return undefined; |
There was a problem hiding this comment.
[nit] npmErrorCode is only fed result.stdout. npm view --json does emit the {"error":{"code":"E404"}} payload on stdout in current npm, but npm has moved error JSON between streams across majors, and a user-level --loglevel/.npmrc setting can change what lands where. If the E404 body arrives on stderr, a genuine "no matching version" turns into a hard throw rather than the intended undefined. Checking npmErrorCode(plain) ?? npmErrorCode(result.stderr.trim()) would make this robust at no real cost.
2 participants
Summary
@dynamic-labs/global-wallet-clientwithin the published range and derive its exact AA peer for clean npm and Bun consumer graphsFixes the registry-drift failure in Sei Global Wallet Consumer Smoke #20.
Test plan
bun run test:sei-global-wallet-releasebun run typecheckbun test --isolate scriptsbunx biome check scripts/check-sei-global-wallet.tsMade with Cursor