cli: allow removing multicast roles in user subscribe by juan-malbeclabs · Pull Request #3914 · malbeclabs/doublezero

juan-malbeclabs · 2026-06-19T02:45:31Z

Summary of Changes

doublezero user subscribe now updates multicast roles rather than only adding them. --publisher/--subscriber accept an explicit value: --publisher false drops the role, --publisher (or --publisher true) adds it, and an omitted flag preserves the user's current role for each group.
The command now errors when neither --publisher nor --subscriber is given, so it never issues a silent no-op.
Output wording changed from Subscribed to <group> to Updated roles for <group> to reflect add/remove semantics.
check_accesspass returns Ok(false) when the access pass is missing instead of bailing with a generic "not found" error, letting the caller render its own diagnostic (client IP and UserPayer).
doublezero connect uses spinner messages instead of printing separate lines while subscribing to groups.

Diff Breakdown

Category	Files	Lines (+/-)	Net
Core logic	2	+46 / -23	+23
Tests	1	+125 / -8	+117
Scaffolding	1	+6 / -8	-2
Docs	1	+2 / -0	+2
Total	4	+173 / -31	+142

Small core-logic change to subscribe semantics, the bulk of additions are new unit tests covering role removal and the required-flag guard.

Key files (click to expand)

smartcontract/cli/src/user/subscribe.rs — --publisher/--subscriber become Option<bool> with num_args = 0..=1; resolve each role per-group (omitted = preserve current), require at least one flag, plus tests for role-removal and the required-flag guard
smartcontract/cli/src/requirements.rs — check_accesspass returns Ok(false) on a missing access pass instead of erroring
client/doublezero/src/command/connect.rs — use spinner.set_message instead of spinner.println during group subscription

Testing Verification

Added unit tests: removing only the publisher role preserves the existing subscriber role (test_cli_user_unsubscribe_publisher_preserves_subscriber), and the command errors when neither role flag is supplied (test_cli_user_subscribe_requires_role_flag).
Updated existing subscribe tests for the new Option<bool> flags and Updated roles for ... output.

Change --publisher/--subscriber from bare bool flags to optional booleans so they accept an explicit value (--publisher false / --subscriber false) to remove a role. When a flag is omitted, the user's current role for the group is preserved, allowing each role to be added or removed independently. Error when neither flag is provided instead of issuing a silent no-op.

ben-dz

Off-chain CLI change extending doublezero user subscribe to add/remove multicast roles via tri-state Option<bool> flags, plus a fail-closed check_accesspass Err→Ok(false) migration and connect.rs spinner output tweaks. The logic is correct, backward-compatible, and the four CLI unit tests pass. No blocking defects and no security issues (the missing-AccessPass path is fail-closed; preserve-role resolution cannot bypass onchain allowlist/payer checks). Findings are usability sharp-edges and test-quality gaps. Top items: (M1) preserving an already-held role re-asserts true and re-runs the onchain allowlist gate, so an unrelated removal can be rejected if the allowlist drifted — but this is a pre-existing processor property the PR inherits, not a regression; (M2) the no-silent-no-op guard only catches the both-omitted case. Recommend tightening tests L1/L2 (and optionally a parse test L3) in this PR and tracking M1/M2 as docs/follow-up.

Changelog present and correct under ### Changes, referencing (#3914). Cosmetic nit: there are now two separate - CLI bullets in the same block; consider merging.
Security: no findings. check_accesspass returning Ok(false) on a missing pass (requirements.rs:84-105) is fail-closed — the sole caller connect.rs:133 gates on if !check_accesspass(...)? and bails, so false is treated as unauthorized; epoch enforcement is unchanged; grep confirms connect.rs:133 is the only consumer. The preserve-role resolution reads client-fetched state but cannot bypass the authoritative onchain allowlist/payer checks. Logged client IP/payer are public per the project trust model.
smartcontract/cli/src/user/subscribe.rs:99-101 — Low/tidiness: the --wait path round-trips the already-resolved group_pk back through GetMulticastGroupCommand's code-resolution (pubkey_or_code: group_pk.to_string()). Harmless and pre-existing.

ben-dz · 2026-06-23T16:00:56Z

        for group_pk in &group_pks {
+            let publisher = self
+                .publisher
+                .unwrap_or_else(|| user.publishers.contains(group_pk));


Preserve-on-omit re-asserts an already-held role as true: --subscriber false on a current publisher resolves publisher=true and sends it. Both the SDK (sdk/rs/src/commands/multicastgroup/subscribe.rs:57-62) and the onchain processor gate publisher==true on the current allowlist before the idempotent add/remove check, so if the pub allowlist was narrowed since the user subscribed, an unrelated subscriber-removal is rejected with NotAllowed. Note this is NOT a regression — the old CLI's --publisher (subscriber absent) hit the same gate. Recommend documenting the coupling, or a follow-up so the processor skips the allowlist check when the role is already present. Non-blocking.

Documented the coupling in a code comment at the role-resolution site (892baf0): preserving an already-held role re-asserts true, which re-runs the onchain allowlist gate, so an unrelated removal can be rejected with NotAllowed if the allowlist drifted. Noted it's an inherited processor property, not a regression. A processor-side skip of the allowlist check when the role is already present is left as a follow-up.

ben-dz · 2026-06-23T16:00:56Z

        client.check_requirements(CHECK_ID_JSON | CHECK_BALANCE)?;

+        // Require at least one role flag so we never issue a silent no-op.
+        if self.publisher.is_none() && self.subscriber.is_none() {


The "never a silent no-op" guard only rejects the both-omitted case. --publisher true against a user who is already a publisher still issues a transaction the processor treats as a no-op (the contains checks short-circuit any write), yet it still costs a signature/credits. Either tighten the changelog wording or add a comment acknowledging this paid-no-op gap.

Extended the "never a silent no-op" comment to acknowledge the gap: the guard only rejects the both-omitted case, so re-asserting a role the user already holds still issues a transaction the processor treats as a no-op, costing a signature/credits. (892baf0)

ben-dz · 2026-06-23T16:00:57Z

+            }
+            .execute(&ctx, &client, &mut output),
        );
+        assert!(res.is_err());


test_cli_user_subscribe_requires_role_flag asserts only res.is_err(). Per the repo's testing guidance (assert specific errors, not .is_err()), assert the bail message contains the user-facing guidance ("Specify at least one of --publisher...") so the test can't pass on an unrelated future failure.

Done in 892baf0 — the test now asserts the bail message contains "Specify at least one of --publisher" instead of just is_err().

ben-dz · 2026-06-23T16:00:57Z

+            }
+            .execute(&ctx, &client, &mut output),
+        );
+        assert!(res.is_ok());


test_cli_user_unsubscribe_publisher_preserves_subscriber verifies the mock call args and Ok result but does not assert the new "Updated roles for ..." output line (the two earlier tests do). Add the output assertion, and consider the symmetric case (preserve publisher while removing subscriber).

Done in 892baf0 — added the "Updated roles for ..." output assertion, and added the symmetric test test_cli_user_unsubscribe_subscriber_preserves_publisher (preserve publisher while removing subscriber).

ben-dz · 2026-06-23T16:00:57Z

+    /// Add (`--subscriber` or `--subscriber true`) or remove (`--subscriber false`) the
+    /// subscriber role. When omitted, the current subscriber role is left unchanged.
+    #[arg(long, num_args = 0..=1, default_missing_value = "true")]
+    pub subscriber: Option<bool>,


The new clap surface (num_args = 0..=1, default_missing_value = "true") coexisting with the variadic --group (num_args = 1..) is never exercised — all tests construct the struct directly. A try_parse_from test covering bare --publisher, --publisher false, and --group g1 g2 --publisher ordering is cheap insurance against argument-ambiguity regressions.

Added test_cli_user_subscribe_parses_role_flags in 892baf0 using a try_parse_from wrapper, covering bare --publisher, --publisher false, and --group g1 g2 --publisher ordering (both groups land in groups, publisher resolves to true).

ben-dz · 2026-06-23T16:00:57Z

-        // Subscribe to each group
+        // Update roles for each group. An omitted flag preserves the user's
+        // current role for that group; the processor toggles add/remove based on
+        // the resolved boolean values.


Nit: comment says the processor "toggles add/remove based on the resolved boolean values," but the processor sets absolute state (idempotent add when true, idempotent remove when false), not a relative toggle. Reword to avoid implying relative state.

Reworded in 892baf0 to say the processor sets absolute state (idempotent add when true, idempotent remove when false), not a relative toggle.

ben-dz · 2026-06-23T16:00:57Z

                // Subscribe to remaining groups
                for group_pk in all_group_pks.iter().skip(1) {
-                    spinner.println(format!("    Subscribing to group: {group_pk}"));
+                    spinner.set_message(format!("Subscribing to group: {group_pk}"));


spinner.println(...) → spinner.set_message(...) makes per-group subscribe progress transient (overwritten by the next set_message, then cleared by finish_and_clear() at line 172), so a multi-group connect no longer prints a line per group. Consistent with the preceding commit's intent and acceptable, but confirm no operator/CI flow relied on those persistent lines in captured output.

Confirmed — the per-group lines were diagnostic progress output, not machine-parsed by any operator/CI flow, so making them transient is fine. Leaving as-is, consistent with the preceding commit's intent.

Tighten tests (assert error message, output line, symmetric preserve case, clap parse coverage), reword the processor toggle comment to absolute state, document the allowlist re-assertion coupling and paid-no-op gap, and merge the duplicate CLI changelog bullet groups.

juan-malbeclabs · 2026-06-23T17:33:12Z

Thanks for the review. Addressed all findings in 892baf0:

L1/L2/L3: tightened the tests — assert the bail message, assert the Updated roles for ... output line, added the symmetric preserve-publisher case, and added a try_parse_from test for the clap surface (bare --publisher, --publisher false, --group g1 g2 --publisher ordering).
M1/M2: documented the allowlist re-assertion coupling and the paid-no-op gap as code comments; processor-side allowlist skip left as a follow-up.
Wording: reworded the toggle comment to absolute state.
Changelog nit: merged the two - CLI bullet groups in the ### Changes block into one.

juan-malbeclabs added 4 commits June 23, 2026 14:50

cli: add changelog entry for user subscribe role removal

9cfc936

client: use spinner messages instead of printing lines during subscribe

8d29360

cli: return Ok(false) from check_accesspass when access pass is missing

fc65dd3

juan-malbeclabs force-pushed the cli/user-subscribe-role-removal branch from 549b994 to fc65dd3 Compare June 23, 2026 14:54

ben-dz approved these changes Jun 23, 2026

View reviewed changes

Conversation

juan-malbeclabs commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Diff Breakdown

Testing Verification

Uh oh!

ben-dz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juan-malbeclabs commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

juan-malbeclabs commented Jun 19, 2026 •

edited

Loading