Repository navigation
feat(lz-bridge-gw): add isReceiveEnabled flag for gateway replacements - #231
Conversation
Allow lzReceive() to continue accepting in-flight messages after disable(), preventing stuck messages during gateway replacements. The flag is gated to emergency/admin, reset automatically by disable(), and has no effect when the gateway is enabled.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded an independent receive-gating flag Changes
Sequence Diagram(s)sequenceDiagram
participant OCG as OCG Proposal
participant OldGW as Old Gateway
participant NewGW as New Gateway
participant DAO as DAO MS
participant Kernel as Kernel
OCG->>Kernel: Propose disable old gateway (stop sending)
Kernel->>OldGW: disable() -- enabler hook sets isReceiveEnabled = false
OCG->>Kernel: Propose enable receive on OldGW and enable NewGW
Kernel->>OldGW: setIsReceiveEnabled(true) / enable() -- receive allowed
Kernel->>NewGW: enable() -- new gateway active
DAO->>DAO: Reconfigure non-canonical chains (no delivery interruption to OldGW)
Kernel->>Kernel: Deactivate OldGW in Kernel (old gateway removed)
Note right of OldGW: After deactivation, explicit setIsReceiveEnabled(false) not required
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| vm.prank(caller_); | ||
| gateway.disable(bytes("")); | ||
| } | ||
|
|
There was a problem hiding this comment.
validate that isReceiveEnabled() is true if re-enabled:
- after disable, then enable
- after disable, then set receive enabled, then enable
| vm.prank(address(endpointSetup.endpointList[1])); | ||
| gateway2.lzReceive(origin, bytes32(0), bytes(""), address(0), bytes("")); | ||
| } | ||
|
|
There was a problem hiding this comment.
additional test
- disabled
- set receive enabled
- enable
receive still works
| ); | ||
| } | ||
|
|
||
| function test_isReceiveEnabled_defaultsFalse() external view { |
There was a problem hiding this comment.
IMO if enabled, isReceiveEnabled should also be true (otherwise it's confusing)
There was a problem hiding this comment.
For simplicity, the isReceivedEnabled flag was implemented like this: it’s ignored when isEnabled is true and only checked when !isEnabled.
Since that behavior isn’t intuitive, the flag now works like a regular flag. _enable and _disable are overridden to automatically turn isReceivedEnabled on or off, so lzReceive no longer checks isEnabled (because _enable/_disable always toggle isReceivedEnabled). admin/emergency can call isReceivedEnabled(true) after disable() for gateway replacements. setIsReceivedEnabled is forbidden if isEnabled.
enable()/disable() now manage isReceiveEnabled automatically via _enable/_disable overrides, so lzReceive checks only isReceiveEnabled without referencing isEnabled. Add ReceiveNotEnabled error for lzReceive distinct from the onlyEnabled modifier used by burnAndSend.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/policies/interfaces/ILZBridgeGateway.sol (1)
242-243: Clarify NatSpec forisReceiveEnabled().The comment "Whether receiving is allowed while the gateway is disabled" is accurate for the primary use case but could be clearer, since
isReceiveEnabledis alsotruewhen the gateway is fully enabled. Consider:- /// `@notice` Whether receiving is allowed while the gateway is disabled. + /// `@notice` Whether lzReceive() can process incoming messages. + /// `@dev` Automatically set to true by enable() and false by disable(). + /// Can be manually set via setIsReceiveEnabled() to allow receiving + /// while the gateway is otherwise disabled (e.g., during gateway replacements).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/policies/interfaces/ILZBridgeGateway.sol` around lines 242 - 243, Update the NatSpec for the isReceiveEnabled() function to explicitly state its two conditions: return true when the gateway is fully enabled and also return true when the gateway is disabled but receiving is still permitted; reference the function name isReceiveEnabled() in the comment and replace the current line with a clearer description like “Returns true if receiving is permitted — either because the gateway is fully enabled or because receiving is explicitly allowed while the gateway is disabled.”
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/policies/bridge/LZBridgeGateway.sol`:
- Around line 328-334: The setIsReceiveEnabled function currently allows
toggling isReceiveEnabled while the gateway is enabled, which can block inbound
messages; modify setIsReceiveEnabled (and/or _setIsReceiveEnabled) to prevent
changing isReceiveEnabled when isEnabled == true by adding a guard that reverts
if isEnabled is true (introduce a new revert like
LZBridgeGateway_CannotToggleReceiveWhileEnabled or reuse an existing appropriate
error) so the receive flag cannot affect the enabled gateway path.
---
Nitpick comments:
In `@src/policies/interfaces/ILZBridgeGateway.sol`:
- Around line 242-243: Update the NatSpec for the isReceiveEnabled() function to
explicitly state its two conditions: return true when the gateway is fully
enabled and also return true when the gateway is disabled but receiving is still
permitted; reference the function name isReceiveEnabled() in the comment and
replace the current line with a clearer description like “Returns true if
receiving is permitted — either because the gateway is fully enabled or because
receiving is explicitly allowed while the gateway is disabled.”
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85b5ddb3-f652-4af5-8d23-cd754c3f491e
📒 Files selected for processing (10)
documentation/lz-bridge/GATEWAY_UPGRADE_NOTES.mdsrc/policies/bridge/LZBridgeGateway.solsrc/policies/interfaces/ILZBridgeGateway.solsrc/proposals/LZBridgeSecurityUpgradeProposal.solsrc/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_BurnAndSend.t.solsrc/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_EnableDisable.t.solsrc/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_LzReceive.t.solsrc/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_RetryingFailedMessages.t.solsrc/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_SetIsReceiveEnabled.t.solsrc/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_View.t.sol
💤 Files with no reviewable changes (1)
- src/proposals/LZBridgeSecurityUpgradeProposal.sol
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_EnableDisable.t.sol (1)
31-39: Align new tests with repo test-structure conventions (given*+ branching-tree naming).The newly added test names and setup style don’t follow the repository’s required test pattern. Please migrate these to
test_given<Condition>_<Action>_<ExpectedResult>()and move reusable setup intogiven*modifiers/helpers.As per coding guidelines: "
src/test/**/*.t.sol: Usegiven*modifiers for state setup in tests" and "Follow branching tree naming for tests:test_given<Condition>_<Action>_<ExpectedResult>()".Also applies to: 41-58, 84-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_EnableDisable.t.sol` around lines 31 - 39, Rename and restructure the tests to follow the repo convention: convert test_enable_setsIsReceiveEnabledTrue to test_givenGatewayDisabled_enable_setsIsReceiveEnabledTrue and move the repeated setup into a reusable given modifier/helper (e.g., givenGatewayDisabled) that performs vm.startPrank(admin), gateway.disable(...), and leaves prank context active; update corresponding tests at the other ranges (lines noted) similarly (e.g., test_givenGatewayEnabled_disable_setsIsReceiveEnabledFalse) and replace inline setup with calls to the given* modifier/helper, ensuring vm.stopPrank() is called only in a shared teardown or at the end of each test if not handled by the helper.documentation/lz-bridge/GATEWAY_UPGRADE_NOTES.md (1)
10-14: Convert the “Expected usage” sequence into a checkable TODO runbook.This section is an execution plan, but it is not trackable in checklist form. Please switch to checkbox items so upgrade progress can be audited during rollout.
📝 Suggested doc update
-### Expected usage - -1. **OCG proposal** calls `oldGateway.disable("")` then `oldGateway.setIsReceiveEnabled(true)` (and enables the new gateway). The old gateway can no longer send but still delivers incoming messages. -2. **DAO MS** reconfigures non-canonical chains at its own pace; in-flight messages continue to arrive at the old gateway. -3. **Old gateway is deactivated in the Kernel** once operations are complete. Calling `setIsReceiveEnabled(false)` is not required — Kernel deactivation is sufficient. +### Upgrade TODO checklist + +- [ ] **OCG proposal** calls `oldGateway.disable("")`, then `oldGateway.setIsReceiveEnabled(true)`, and enables the new gateway. +- [ ] **DAO MS** reconfigures non-canonical chains while in-flight messages continue arriving at the old gateway. +- [ ] **Deactivate old gateway in the Kernel** once operations complete.As per coding guidelines: "
**/*.md: When planning a new feature, write the plan to disk in Markdown format and always include a TODO list that can be checked off".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@documentation/lz-bridge/GATEWAY_UPGRADE_NOTES.md` around lines 10 - 14, Convert the "Expected usage" numbered sequence into a checkable TODO runbook by replacing the three numbered steps with Markdown checkbox items that mirror each action and verification point: include checkboxes for "OCG proposal: call oldGateway.disable(\"\")", "OCG: call oldGateway.setIsReceiveEnabled(true) and enable new gateway", "DAO MS: reconfigure non-canonical chains and verify in-flight messages continue to arrive at old gateway", and "Kernel: deactivate old gateway (no need to call setIsReceiveEnabled(false))"; add brief acceptance criteria or verification notes for each checkbox (e.g., new gateway enabled, messages delivered, deactivation confirmed) so rollout progress can be audited.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@documentation/lz-bridge/GATEWAY_UPGRADE_NOTES.md`:
- Around line 10-14: Convert the "Expected usage" numbered sequence into a
checkable TODO runbook by replacing the three numbered steps with Markdown
checkbox items that mirror each action and verification point: include
checkboxes for "OCG proposal: call oldGateway.disable(\"\")", "OCG: call
oldGateway.setIsReceiveEnabled(true) and enable new gateway", "DAO MS:
reconfigure non-canonical chains and verify in-flight messages continue to
arrive at old gateway", and "Kernel: deactivate old gateway (no need to call
setIsReceiveEnabled(false))"; add brief acceptance criteria or verification
notes for each checkbox (e.g., new gateway enabled, messages delivered,
deactivation confirmed) so rollout progress can be audited.
In
`@src/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_EnableDisable.t.sol`:
- Around line 31-39: Rename and restructure the tests to follow the repo
convention: convert test_enable_setsIsReceiveEnabledTrue to
test_givenGatewayDisabled_enable_setsIsReceiveEnabledTrue and move the repeated
setup into a reusable given modifier/helper (e.g., givenGatewayDisabled) that
performs vm.startPrank(admin), gateway.disable(...), and leaves prank context
active; update corresponding tests at the other ranges (lines noted) similarly
(e.g., test_givenGatewayEnabled_disable_setsIsReceiveEnabledFalse) and replace
inline setup with calls to the given* modifier/helper, ensuring vm.stopPrank()
is called only in a shared teardown or at the end of each test if not handled by
the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 456b52b9-911a-4274-8681-e495783f0d0c
📒 Files selected for processing (5)
documentation/lz-bridge/GATEWAY_UPGRADE_NOTES.mdsrc/policies/interfaces/ILZBridgeGateway.solsrc/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_EnableDisable.t.solsrc/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_RetryingFailedMessages.t.solsrc/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_SetIsReceiveEnabled.t.sol
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_RetryingFailedMessages.t.sol
- src/policies/interfaces/ILZBridgeGateway.sol
- src/test/policies/bridge/LZBridgeGateway/LZBridgeGateway_SetIsReceiveEnabled.t.sol
3 participants
Allow lzReceive() to continue accepting in-flight messages after disable(), preventing stuck messages during gateway replacements. The flag is gated to emergency/admin, reset automatically by disable() (for an emergency shutdown), and has no effect when the gateway is enabled.
Summary by CodeRabbit
Documentation
New Features
Tests