-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(protocol): fix singla service cannot be shared by multiple taiko L1/L2 contracts on the same chain bug #15807
Conversation
WalkthroughThe modifications across the protocol's contracts and tests introduce a new mechanism for authorizing chain data relayers in the Changes
Related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- packages/protocol/contracts/signal/SignalService.sol (2 hunks)
- packages/protocol/test/L1/TaikoL1TestBase.sol (1 hunks)
- packages/protocol/test/L2/TaikoL2.t.sol (2 hunks)
- packages/protocol/test/L2/TaikoL2NoFeeCheck.t.sol (2 hunks)
- packages/protocol/test/signal/SignalService.t.sol (2 hunks)
Additional comments: 11
packages/protocol/test/L2/TaikoL2.t.sol (2)
- 31-39: The deployment of
SignalService
usingdeployProxy
is correctly implemented, ensuring the contract is initialized with the necessaryaddressManager
.- 60-60: Authorization of the chain data relayer for
SignalService
is correctly performed by callingss.authorizeChainDataRelayer(address(L2), true);
. This aligns with the PR's objective to support multiple taiko contracts.packages/protocol/test/L2/TaikoL2NoFeeCheck.t.sol (2)
- 30-38: The deployment of
SignalService
usingdeployProxy
is correctly implemented, ensuring the contract is initialized with the necessaryaddressManager
.- 60-60: Authorization of the chain data relayer for
SignalService
is correctly performed by callingss.authorizeChainDataRelayer(address(L2), true);
. This aligns with the PR's objective to support multiple taiko contracts.packages/protocol/contracts/signal/SignalService.sol (4)
- 42-43: Introduction of
isChainDataRelayerAuthorized
mapping is correctly implemented to track the authorization status of chain data relayers.- 49-49: The
RelayerAuthorized
event is correctly defined to log changes in authorization status.- 66-73: The
authorizeChainDataRelayer
function is correctly implemented, including validation to prevent redundant authorization changes and emitting theRelayerAuthorized
event.- 84-84: Modification in the
relayChainData
function to include an authorization check usingisChainDataRelayerAuthorized
mapping is correctly implemented, ensuring only authorized relayers can call this function.packages/protocol/test/L1/TaikoL1TestBase.sol (1)
- 57-57: Authorization of the chain data relayer for
SignalService
is correctly performed by callingss.authorizeChainDataRelayer(address(L1), true);
. This aligns with the PR's objective to support multiple taiko contracts.packages/protocol/test/signal/SignalService.t.sol (2)
- 55-55: The call to
authorizeChainDataRelayer
correctly authorizes a relayer. Ensure that corresponding deauthorization logic is also tested for completeness.- 304-309: The deauthorization of a chain data relayer and the subsequent expected revert due to unauthorized access is correctly implemented and tested. This ensures that the authorization mechanism is functioning as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- packages/protocol/contracts/signal/SignalService.sol (1 hunks)
- packages/protocol/script/DeployOnL1.s.sol (2 hunks)
- packages/protocol/test/L1/TaikoL1TestBase.sol (1 hunks)
- packages/protocol/test/L2/TaikoL2.t.sol (2 hunks)
- packages/protocol/test/L2/TaikoL2NoFeeCheck.t.sol (2 hunks)
- packages/protocol/test/signal/SignalService.t.sol (2 hunks)
- packages/protocol/utils/generate_genesis/taikoL2.ts (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/protocol/test/L1/TaikoL1TestBase.sol
- packages/protocol/test/L2/TaikoL2.t.sol
- packages/protocol/test/L2/TaikoL2NoFeeCheck.t.sol
- packages/protocol/test/signal/SignalService.t.sol
Additional comments: 6
packages/protocol/contracts/signal/SignalService.sol (4)
- 42-42: Line 42 introduces a new mapping
isRelayerAuthorized
to track the authorization status of relayers. Ensure that this mapping is appropriately used throughout the contract to enforce relayer authorization checks.- 49-49: Line 49 introduces a new event
RelayerAuthorized
to log changes in relayer authorization status. Verify that this event is emitted correctly in theauthorizeRelayer
function whenever a relayer's authorization status changes.- 66-73: Lines 66-73 introduce the
authorizeRelayer
function, which allows the contract owner to authorize or deauthorize relayers. Ensure that the function correctly updates theisRelayerAuthorized
mapping and emits theRelayerAuthorized
event. Also, check for potential reentrancy issues, although it seems unlikely due to the restricted access and nature of operations.- 84-84: Line 84 modifies the
relayChainData
function to check if the caller is an authorized relayer before proceeding. Ensure that this check effectively prevents unauthorized addresses from calling this function.packages/protocol/script/DeployOnL1.s.sol (1)
- 94-96: Lines 94-96 introduce a conditional block to authorize a relayer in the SignalService contract if the
SHARED_ADDRESS_MANAGER
environment variable is not set. Ensure that this logic aligns with the intended deployment and authorization strategy, particularly in relation to the environment setup and the conditions under which a relayer should be authorized.packages/protocol/utils/generate_genesis/taikoL2.ts (1)
- 464-466: Lines 464-466 modify the
generateContractConfigs
function to includeisRelayerAuthorized
mapping withTaikoL2
set totrue
in the SignalService contract configuration. Ensure that this change correctly initializes the SignalService contract with the TaikoL2 address authorized as a relayer, aligning with the intended functionality and security model.
In Signal Service, we cannot assume there is only one "taiko" address on a chain so using
onlyFromNamed("taiko")
is incorrect.Summary by CodeRabbit
Summary by CodeRabbit