Skip to content

serviceability-instruction: RFC-26 R0 post-review follow-ups#4062

Open
juan-malbeclabs wants to merge 2 commits into
mainfrom
feat/rfc26-r0-review-followups
Open

serviceability-instruction: RFC-26 R0 post-review follow-ups#4062
juan-malbeclabs wants to merge 2 commits into
mainfrom
feat/rfc26-r0-review-followups

Conversation

@juan-malbeclabs

Copy link
Copy Markdown
Contributor

Post-approval follow-ups to the RFC-26 R0 scaffold (#4049), split into their own PR so #4049 stays at the state Nik approved. Stacked on #4049 — GitHub will retarget this to main once #4049 merges.

Summary

  • Add the RFC-26 R0 entry to CHANGELOG.md (the scaffold PR had skipped it via label).
  • delete_device: spell out DeviceDeleteArgs { resource_count: 0 } instead of ::default(), so a future field addition becomes a compile error that forces the builder to take a position.
  • create_device: guard the caller-set resource_count write-back with a debug_assert (parity with create_subscribe_user's dz_prefix_count).
  • Builder tests: assert the full decoded args struct (clone input + set expected count) instead of only the count field, pinning that the mut args write-back touches only that field and auto-covering future fields.

These address the non-blocking items and post-approval inline nits from the review on #4049.

RFC: rfcs/rfc26-rust-instruction-builder-library.md

Testing Verification

  • 7 crate unit tests pass, including the strengthened full-args-struct assertions for create_device, delete_device (atomic), and create_subscribe_user.

Base automatically changed from feat/rfc26-r0-instruction-builder-scaffold to main July 14, 2026 21:59
…ource_count assert

Addresses non-blocking review follow-ups: land the RFC-26 R0 changelog
entry, and guard create_device's caller-set resource_count with a
debug_assert (parity with create_subscribe_user's dz_prefix_count).
- delete_device: spell out DeviceDeleteArgs { resource_count: 0 } instead of
  ::default() so a future field addition is a compile error
- builder tests: assert the full decoded args struct (clone input + set expected
  count) instead of only the count field, pinning that the write-back touches
  only that field and auto-covering future fields
@juan-malbeclabs juan-malbeclabs force-pushed the feat/rfc26-r0-review-followups branch from 42dc76c to f56bcd7 Compare July 14, 2026 22:11

@nikw9944 nikw9944 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.

Small, well-scoped follow-up to the approved #4049 scaffold. All four changes check out: the CHANGELOG entry is correctly placed and attributed to #4049; the create_device debug_assert is exact parity with create_subscribe_user's (and resource_total >= 1 means a caller-set 0 can never falsely pass); spelling out DeviceDeleteArgs { resource_count: 0 } is behavior-identical to ::default() with a sound future-proofing rationale; and the full-args-struct test assertions correctly pin that the write-back touches only the count field. create_link was rightly left alone (no write-back to pin). Verified locally: 7 crate unit tests pass, clippy (-Dclippy::all -Dwarnings) and nightly rustfmt clean. One optional test-symmetry nit below.

  • Low: test_delete_device_legacy (crates/doublezero-serviceability-instruction/src/device.rs:314-340) asserts the tag byte and account list but never decodes the instruction data, so resource_count: 0 — the exact value this PR made explicit at src/device.rs:170 — is unpinned. A matching assert_eq!(a, DeviceDeleteArgs { resource_count: 0 }) after an unpack would complete the symmetry with the Atomic test. Non-blocking.

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