feat: add /unregister_beacon route to deregister a beacon from the BeaconRegistry#83
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesAdds a beacon unregistration request model, service flow, Beacon unregistration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Rocket
participant BeaconService
participant Registry
participant SafeOrWallet
Client->>Rocket: POST /unregister_beacon
Rocket->>BeaconService: unregister_beacon_with_registry
BeaconService->>Registry: check_beacon_registered
alt Not registered
Registry-->>BeaconService: AlreadyUnregistered
else Registered
BeaconService->>SafeOrWallet: Propose or send unregisterBeacon
SafeOrWallet-->>BeaconService: Transaction hash
BeaconService->>Registry: Confirm transaction
Registry-->>BeaconService: Receipt status
end
BeaconService-->>Rocket: UnregistrationOutcome
Rocket-->>Client: JSON response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/integration_tests/unregister_beacon_integration_tests.rs (1)
29-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftProvide a seeded local fixture before claiming integration coverage. The tests are all ignored due to hanging real-RPC calls; two also convert fixture failure into a passing return. Seed beacon factories in the isolated Anvil setup, make setup failures assert, and remove
#[ignore].
tests/integration_tests/unregister_beacon_integration_tests.rs#L29-L37: execute the register/unregister flow against the seeded fixture and fail if beacon creation fails.tests/integration_tests/unregister_beacon_integration_tests.rs#L74-L76: execute the no-op flow in CI against the same hermetic fixture.tests/integration_tests/unregister_beacon_integration_tests.rs#L99-L107: execute idempotency coverage and replace the successful early return with a setup assertion.As per coding guidelines, “Mock providers with wallets in tests so setup is consistent and network calls fail gracefully in the test environment.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration_tests/unregister_beacon_integration_tests.rs` around lines 29 - 37, The unregister beacon integration tests currently remain ignored and may pass silently when fixture setup fails. In tests/integration_tests/unregister_beacon_integration_tests.rs#29-37, `#74-76`, and `#99-107`, seed beacon factories in the isolated Anvil setup, use hermetic wallet-backed mock providers, remove #[ignore], and assert beacon creation/setup succeeds instead of returning early; preserve the register/unregister, no-op, and idempotency flows respectively.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/beacon/core.rs`:
- Around line 500-508: Update the unregistration flow around
is_beacon_registered so RPC or contract-call failures are propagated instead of
being converted into false; use a strict lookup variant if available, and return
AlreadyUnregistered only when the registry successfully confirms the beacon is
absent.
- Around line 463-472: Update the transaction confirmation path around
is_transaction_confirmed so Ok(Ok(None)) performs the same progressive receipt
polling used by the registration flow before returning an error. Keep the
existing timeout and successful receipt handling, but allow pending transactions
to be observed before declaring the transaction missing and failing the request.
In `@tests/unit_tests/unregister_beacon_route_tests.rs`:
- Around line 109-145: Update test_unregister_beacon_unknown_status_is_noop_ok
and test_unregister_beacon_defaults_registry_when_absent so provider failures
are not converted into successful no-ops. Configure the mock provider to return
“not registered” for the relevant registry, especially
app_state.contracts.perpcity_registry when registry_address is None, or assert
that an unavailable provider produces InternalServerError instead.
---
Nitpick comments:
In `@tests/integration_tests/unregister_beacon_integration_tests.rs`:
- Around line 29-37: The unregister beacon integration tests currently remain
ignored and may pass silently when fixture setup fails. In
tests/integration_tests/unregister_beacon_integration_tests.rs#29-37, `#74-76`,
and `#99-107`, seed beacon factories in the isolated Anvil setup, use hermetic
wallet-backed mock providers, remove #[ignore], and assert beacon creation/setup
succeeds instead of returning early; preserve the register/unregister, no-op,
and idempotency flows respectively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69ed8249-f7f7-4682-bf72-ab561a2a7851
📒 Files selected for processing (9)
src/lib.rssrc/models/mod.rssrc/models/requests.rssrc/routes/beacon.rssrc/services/beacon/core.rstests/integration_tests/mod.rstests/integration_tests/unregister_beacon_integration_tests.rstests/unit_tests/mod.rstests/unit_tests/unregister_beacon_route_tests.rs
… (fix full-integration CI)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What
Adds
POST /unregister_beaconto the-beaconator — removes a beacon from the on-chainBeaconRegistry. The service previously hadregister_beaconbut no way to deregister.Why
We're decommissioning most mainnet censorship (OONI) markets and need to deregister the retired beacons from the registry (deleting the
market_metadatarow and stopping updates are separate, already-supported steps). No deregister route existed, so this adds one.How
Mirrors the existing
register_beacon_with_registrypath exactly, with an inverted pre-check:UnregistrationOutcomeenum (AlreadyUnregistered/SafeProposed/OnChainConfirmed) — kept separate fromRegistrationOutcomeso the register route's exhaustive match is untouched.unregister_beacon_with_registry()inservices/beacon/core.rs: if the beacon is not registered it's a no-op success; if a Safe is configured (both live networks) it proposes a Safe multisig transaction; otherwise (local/non-Safe) it executes directly with a pool wallet and confirms the receipt with an on-chain fallback.POST /unregister_beaconroute (ApiToken-guarded). Request{ beacon_address, registry_address? }—registry_addressis optional and defaults to the server-configured registry.IBeaconRegistry::unregisterBeacon(address)binding that already exists in the inlinesol!interface (src/routes/mod.rs) — no ABI refresh.Deployed-contract verification
the-beaconatorpinsbeacons=v0.0.1/perpcity-contracts=v0.1.0(.contracts-versions) — the tags actually deployed on Arbitrum One / Sepolia. ConfirmedBeaconRegistry.unregisterBeacon(IBeacon) external onlyOwnerexists atbeacons@v0.0.1(d61aca0), selectorunregisterBeacon(address)— matches the binding, so the call hits real deployed code.Drift note: at v0.0.1 the emit is
BeaconUnregistered(beacon, beacon.index())un-guarded; thetry/catcharoundindex()is a post-v0.0.1 addition onbeaconsmain and is NOT in the deployed contract. Practical impact: the deployedunregisterBeaconreverts only if the target'sindex()reverts — a non-issue for the live, healthy beacons we're removing.Live-network behavior
On mainnet the registry owner is a Safe multisig, so this route proposes the removal — it takes effect only once signers execute the Safe tx. This is the identical mechanism that registered all current beacons, so it's a proven path.
Tests
tests/unit_tests/unregister_beacon_route_tests.rs) — address validation, optional-registry defaulting, serde, and the no-op-on-unknown-status behavior. These run in theunit-testsCI job.tests/integration_tests/unregister_beacon_integration_tests.rs) mirror the register integration tests and are#[ignore]d for the same reason (the shared Anvil harness doesn't deploy the factories).make qualitygreen locally (fmt-check, clippy-D warnings, unit suite 200 passed, Redis tests).Notes
Summary by CodeRabbit
POST /unregister_beaconendpoint to deregister a beacon from its registry.500 InternalServerErrorinstead of behaving like a no-op.