serviceability: enforce Activated status on suspend handlers #2428
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.
Title
Enforce Activated status before suspend + add negative tests
Summary
This PR makes the suspend logic in the serviceability program a bit stricter and more consistent.
Right now, some *_suspend handlers don’t check the current status of the entity before allowing a suspend. That means it’s possible to try to suspend something that is still Pending or already Suspended, while link/suspend.rs already enforces that only Activated entities can be suspended.
Here I align all suspend handlers with that same rule and add one negative test per entity type to make sure we don’t regress in the future.
This addresses #2221.
What changed
For the following handlers:
location/suspend.rsexchange/suspend.rscontributor/suspend.rsdevice/suspend.rsuser/suspend.rsmulticastgroup/suspend.rsI now:
Deserialize the entity
Check that
entity.status == EntityStatus::ActivatedReturn
DoubleZeroError::InvalidStatusotherwise (with a small debug log under#[cfg(test)])The check follows the same pattern already used in link/suspend.rs:
This doesn’t change behaviour for valid suspends (entities already Activated). It just tightens validation when the status is not valid for a suspend.
I added one negative test per entity type to make the new checks explicit and guarded by tests:
test_suspend_location_from_suspended_failstest_suspend_exchange_from_suspended_failstest_suspend_contributor_from_suspended_failstest_suspend_device_from_pending_failstest_suspend_user_from_suspended_failstest_suspend_multicastgroup_from_pending_failsEach test:
Puts the entity in an invalid state for suspend (Pending or Suspended depending on the case),
Calls the corresponding *_suspend processor,
Asserts that it fails with
DoubleZeroError::InvalidStatus.This way each handler’s status check is covered and future changes will be forced to keep this behaviour.
While working on the tests, I also fixed the account list passed to device activation.
device/activate expects:
deviceglobalstatepayersystem_programThe previous test setup was missing some of these, so it didn’t really mirror the on-chain invocation. I updated the test to provide the full, correct account list.
This doesn’t change behaviour, but it makes the test more realistic and closer to what actually happens at runtime.
Testing
From the smartcontract workspace:
Results:
All 6 new negative tests pass
All existing tests in doublezero-serviceability pass
cargo clippy clean (no new warnings)