Two rotation functions accept a "new" value equal to the current one, silently succeed, and emit a misleading event:
update_admin(admin, new_admin) at lib.rs:1172 — passing new_admin == admin writes the same admin back and emits UpdateAdminEvent { old_admin, new_admin } with identical fields.update_orchestrator(user, new_orchestrator, name) at lib.rs:514 — passing the currently registered orchestrator re-emits UpdateOrchEvent with old == new.
Neither corrupts balances, but both pollute the on-chain event stream with phantom "rotations" that indexers and audit tooling will treat as real changes — and a no-op call almost always signals a client bug (e.g. a form submitted without an actual change). Failing fast surfaces that bug instead of hiding it.
Goal
Both functions reject a rotation whose target equals the current value, before any state write or event emission.
Design decisions left to the implementer
- Error variant. A dedicated
VaultError::NoChange reads clearest; reusing an existing variant is acceptable if the maintainers prefer not to grow the enum — justify the choice in the PR.
- Check ordering. The no-op check must come after the existing security checks (admin auth;
active_tasks_count == 0; not-owned-by-another-user) so the most security-relevant failure always takes precedence.
Edge cases to consider
update_orchestrator where the user has no orchestrator registered at all — the existing OrchestratorNotRegistered path must still win.- A legitimate rotation to a genuinely different address must still succeed and emit its event exactly once.
Acceptance criteria
Notes for contributors
If you'd like to work on this, comment below so we can assign it to you.
Questions welcome — see CONTRIBUTING.md for setup
and workflow.
Two rotation functions accept a "new" value equal to the current one, silently succeed, and emit a misleading event:
update_admin(admin, new_admin)atlib.rs:1172— passingnew_admin == adminwrites the same admin back and emitsUpdateAdminEvent { old_admin, new_admin }with identical fields.update_orchestrator(user, new_orchestrator, name)atlib.rs:514— passing the currently registered orchestrator re-emitsUpdateOrchEventwithold == new.Neither corrupts balances, but both pollute the on-chain event stream with phantom "rotations" that indexers and audit tooling will treat as real changes — and a no-op call almost always signals a client bug (e.g. a form submitted without an actual change). Failing fast surfaces that bug instead of hiding it.
Goal
Both functions reject a rotation whose target equals the current value, before any state write or event emission.
Design decisions left to the implementer
VaultError::NoChangereads clearest; reusing an existing variant is acceptable if the maintainers prefer not to grow the enum — justify the choice in the PR.active_tasks_count == 0; not-owned-by-another-user) so the most security-relevant failure always takes precedence.Edge cases to consider
update_orchestratorwhere the user has no orchestrator registered at all — the existingOrchestratorNotRegisteredpath must still win.Acceptance criteria
update_adminwithnew_admin == current adminerrors and emits no event or state writeupdate_orchestratorwith an unchanged orchestrator errors and emits no event or state writecargo testandcargo clippy --all-targets -- -D warningspassNotes for contributors
If you'd like to work on this, comment below so we can assign it to you.
Questions welcome — see CONTRIBUTING.md for setup
and workflow.