Skip to content

refactor(b20-asset, activation): remove unreachable announcement guard and redundant AlreadyDeactivated error#3253

Open
rayyan224 wants to merge 3 commits into
mainfrom
rayyanalam/bop-292-5-unused-unreachable-code-in-the-b20-asset-and-activation
Open

refactor(b20-asset, activation): remove unreachable announcement guard and redundant AlreadyDeactivated error#3253
rayyan224 wants to merge 3 commits into
mainfrom
rayyanalam/bop-292-5-unused-unreachable-code-in-the-b20-asset-and-activation

Conversation

@rayyan224
Copy link
Copy Markdown
Contributor

@rayyan224 rayyan224 commented Jun 5, 2026

Summary

B20 Asset — announcement guard (BOP-292 finding #1)

  • Removes the in_announcement: bool field, is_announcement_active(), and begin_announcement() from B20AssetToken
  • Removes the dead is_announcement_active() guard and begin_announcement() call from announce() in dispatch.rs
  • Corrects the announce doc comment to name the selector check as the sole recursion defense

Activation Registry — redundant error (BOP-292 finding #2)

  • Removes AlreadyDeactivated from activation/abi.rs — it was declared but never returned
  • Clarifies set_activated variable names to make the activate/deactivate branch intent explicit

Context

Finding #1 — unreachable is_announcement_active() guard

The is_announcement_active() check in announce() was structurally unreachable:

  1. begin_announcement() was called after the check, so in_announcement was always false when the guard ran.
  2. The only re-entry vector — the inner call loop — already rejects the announce selector before dispatch, so a recursive call could never reach the outer function body where the guard lives.
  3. There was no end_announcement() or reset, leaving the token permanently stuck with in_announcement = true after announce() completed — a one-way latch with no observable effect only because the token is constructed fresh per invocation.

Cross-checked against MockB20Asset.sol: the reference implementation carries no flag at all. Its two actual defenses are the selector check (_checkSelector) and early ID marking before inner calls run. The Rust now matches this exactly.

Finding #2 — redundant AlreadyDeactivated error

AlreadyDeactivated and FeatureNotActivated both represent the same underlying state — features[feature] == false. The intended distinction (mutation context vs query context) was never implemented: MockActivationRegistry.sol collapses both into FeatureNotActivated. AlreadyDeactivated was not just unused — it was redundant with an existing error. Removed from the Rust ABI to match the updated IActivationRegistry spec.

Test plan

  • cargo test --manifest-path crates/common/precompiles/Cargo.toml — all 344 tests pass
  • No remaining references to is_announcement_active, begin_announcement, in_announcement, or AlreadyDeactivated in the crate tree

The is_announcement_active() check in announce() could never fire:
begin_announcement() is called after the check (so the flag is always
false when the guard runs), and the only re-entry vector — the inner
call loop — already rejects the announce selector before dispatch.

The flag also had no reset path, leaving the token permanently stuck
with in_announcement=true after announce() completed. The selector
check in the inner loop is the sole recursion defense, matching the
MockB20Asset reference implementation which carries no flag at all.

Remove in_announcement, is_announcement_active(), and begin_announcement()
from B20AssetToken, and drop the dead guard and call-site from dispatch.

Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
@linear
Copy link
Copy Markdown

linear Bot commented Jun 5, 2026

BOP-292

@cb-heimdall
Copy link
Copy Markdown
Collaborator

cb-heimdall commented Jun 5, 2026

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@rayyan224 rayyan224 marked this pull request as ready for review June 5, 2026 15:01
@rayyan224 rayyan224 requested review from eric-ships and refcell and removed request for eric-ships June 5, 2026 15:02
AlreadyDeactivated and FeatureNotActivated both represent the same
underlying state (features[feature] == false). The intended distinction
between mutation context (deactivate) and query context (checkActivated)
was never implemented: the mock collapses both into FeatureNotActivated.

Remove AlreadyDeactivated from the ABI to eliminate the dead declaration
and bring the Rust interface in line with the updated IActivationRegistry
spec and MockActivationRegistry behavior. Also clarifies set_activated
variable names to make the activate/deactivate branch intent explicit.

Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
@rayyan224 rayyan224 changed the title refactor(b20-asset): remove unreachable in_announcement guard refactor(b20-asset, activation): remove unreachable announcement guard and redundant AlreadyDeactivated error Jun 5, 2026
Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
feature,
}));

let current_activated_state = self.features.at(&feature).read()?;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not logic change, making it more readable with clean code principles

Comment on lines +137 to +140
let is_activating_and_already_activated =
to_activated_state == true && current_activated_state == true;
let is_deactivating_and_already_deactivated =
to_activated_state == false && current_activated_state == false;
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.

== true / == false comparisons on bool values will trigger clippy::bool_comparison (enabled via clippy::all in the workspace). Idiomatic Rust:

Suggested change
let is_activating_and_already_activated =
to_activated_state == true && current_activated_state == true;
let is_deactivating_and_already_deactivated =
to_activated_state == false && current_activated_state == false;
let is_activating_and_already_activated =
to_activated_state && current_activated_state;
let is_deactivating_and_already_deactivated =
!to_activated_state && !current_activated_state;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping it verbose is a stylstic descion

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

Review Summary

Clean removal of dead code — the analysis in the PR description is thorough and the changes are correct. No remaining references to the removed items (is_announcement_active, begin_announcement, in_announcement, AlreadyDeactivated) exist anywhere in the crate tree.

Finding

activation/storage.rs:137-140clippy::bool_comparison: The new == true / == false comparisons on bool values are non-idiomatic and will be flagged by clippy::bool_comparison (enabled via clippy::all in the workspace lints). See inline comment for suggested fix.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

✅ base-std fork tests: all 602 passed

base/base is fully in sync with the base-std spec.

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