test(wallet-service): harden /version contract#428
Open
tuliomir wants to merge 5 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWiden native_token to include optional numeric version, return the schema-validated value from fullnode.version(), and add tests validating modern and legacy payloads plus schema strictness and handler error behavior. ChangesFullnode Version Schema & Handler Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds direct schema unit tests for FullnodeVersionSchema, two fullnode.version() tests, and a live-fullnode contract test that fails CI when the /version response stops matching the schema. Aligns FullNodeApiVersionResponse with the native_token.version field added in #420 and drops the type cast that was masking the drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tuliomir
force-pushed
the
chore/pr-420-hardening
branch
from
de286a6 to
df1493e
Compare
Both schemas.test.ts and fullnode.test.ts duplicated a ~17-line FullNodeApiVersionResponse fixture. Replaces both with reuse of the existing `defaultTestVersionData()` factory in tests/utils.ts — already the canonical fixture (used by `seedFullnodeVersionData` and referenced from jestSetup.ts) — spreading and overriding only the `native_token` field under test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps the three version()-related cases in a `describe('version', ...)`
block for readability. Adds a case asserting `fullnode.version()` still
parses successfully when the fullnode response omits the new
`native_token.version` field, mirroring older fullnodes that predate
#420.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/wallet-service/tests/fullnode.test.ts (1)
4-53: ⚡ Quick winAdd mock cleanup to keep tests isolated.
Each test in this
describe('version')block usesjest.spyOn(fullnode.api, 'get')but never restores it. Add anafterEach(() => jest.restoreAllMocks())to avoid cross-test leakage as this suite grows. This aligns with the cleanup patterns already used elsewhere in the test suite.Suggested addition
describe('version', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + test('returns parsed payload when native_token includes the version field', async () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/wallet-service/tests/fullnode.test.ts` around lines 4 - 53, Add test teardown to the describe('version') suite to restore spies after each test: in the same describe block that contains tests calling jest.spyOn(fullnode.api, 'get'), add an afterEach hook that calls jest.restoreAllMocks() so the spy on fullnode.api.get is cleared between tests and prevents cross-test leakage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/wallet-service/tests/fullnode.test.ts`:
- Around line 4-53: Add test teardown to the describe('version') suite to
restore spies after each test: in the same describe block that contains tests
calling jest.spyOn(fullnode.api, 'get'), add an afterEach hook that calls
jest.restoreAllMocks() so the spy on fullnode.api.get is cleared between tests
and prevents cross-test leakage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60830d12-ee76-45b4-a177-edb9e362eac9
📒 Files selected for processing (1)
packages/wallet-service/tests/fullnode.test.ts
Adds `afterEach(() => jest.restoreAllMocks())` to the `describe('version', ...)`
block so the `jest.spyOn(fullnode.api, 'get')` set up by each test is reverted
between tests. Prevents implicit cross-test leakage as the suite grows; aligns
with the cleanup pattern used in tests/auth.readonly.test.ts.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
r4mmer
approved these changes
May 14, 2026
| import { FullnodeVersionSchema } from '@src/schemas'; | ||
| import { defaultTestVersionData } from '@tests/utils'; | ||
|
|
||
| // Regression guard for the testnet outage that motivated PR 420: the |
Member
There was a problem hiding this comment.
We should not reference the PR solving the outage if possible, the comment should document the core issue this addresses instead.
I think a link to an issue describing the problem would be good if the core issue was too complex to describe in the comment.
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.
Motivation
Completes the follow-up work after #420, which was a one-line emergency bugfix (allowing
native_token.versionon the fullnode/versionpayload). The fix shipped narrow on purpose; both CodeRabbit and Copilot flagged that it left loose ends — TypeScript typing and tests. This PR closes those out, scoped tightly tonative_token.version.Changes
FullNodeApiVersionResponse.native_tokengainsversion?: numberso the TS type matches the Joi schema. Theas FullNodeApiVersionResponsecast infullnode.tsis dropped (it was masking the drift;Joi.object<T>()already types the validator's output).tests/schemas.test.ts, 4 cases) — first direct coverage ofFullnodeVersionSchema, all scoped to thenative_token.versionfield:native_token.version: 0) validates.native_token.version) validates.native_tokenfail validation (design intent — keep nested object strict).native_token.version(e.g.1.5) fails validation.fullnode.version()tests (extendtests/fullnode.test.ts) — happy-path and schema-rejection cases that verify the schema wiring insidefullnode.tsactually runs and throws. Rejection case uses anative_token.versionviolation.Out of scope (intentionally)
A live-fullnode contract test (that would fail CI when the
/versionresponse drifts) was considered and removed from this PR, since there is no existing precedent for tests making real HTTP calls in the wallet-service workspace — PR #394 went the other direction. That can be a separate PR if we want to introduce that pattern.Acceptance Criteria
FullnodeVersionSchemaaccepts the post-PR 420native_tokenpayload and rejects misuse of the newversionfield; covered by unit tests.FullNodeApiVersionResponsereflects the schema, removing the type cast that was hiding the drift.Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets mergedSummary by CodeRabbit
New Features
Refactor
Tests