Skip to content
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(bootstrap): do not fail mark bootstrap #90

Merged

Conversation

MaxMustermann2
Copy link
Collaborator

@MaxMustermann2 MaxMustermann2 commented Sep 2, 2024

  • As outlined in doc: add cross-chain-communication-model.md #83, a cross-chain message that can't/won't be retried must not fail. Hence, all calls to markBootstrapped on Bootstrap should not fail. Fixes Mark bootstrap failures should not halt the system #89
  • In light of the above, the initialize function of the Bootstrap contract must be provided with the address of the clientChainGatewayLogic and its initialization data.
  • Separately, sending multiple LZ messages in one transaction is convoluted and hence disabled. As a consequence, marking bootstrap on all chains is no longer supported; instead, the caller must provide the LZ chain ID. The calls can be retried, and hence, the caller must provide the native fee for this transaction.

Summary by CodeRabbit

  • New Features

    • Introduced a streamlined bootstrap process for client chains, enhancing error handling and initialization logic.
    • Added new events to communicate bootstrap states effectively.
    • Integrated a fee-based approach for marking bootstrap across chains.
    • Added a new function for initiating the bootstrap process for specific chain indices.
  • Bug Fixes

    • Improved error handling during bootstrap operations, allowing for retries and graceful failures.
  • Tests

    • Enhanced test coverage for bootstrap initialization and error scenarios, ensuring robust contract behavior.
    • Added tests to validate behavior with zero addresses and malformed data.

- As outlined in ExocoreNetwork#83, a cross-chain message that can't/won't be retried
  must not fail. Hence, all calls to `markBootstrapped` on `Bootstrap`
  should not fail. Fixes ExocoreNetwork#89
- Separately, sending multiple LZ messages in one transaction is
  convoluted and hence disabled. As a consequence, marking bootstrap on
  all chains is no longer supported; instead, the caller must provide
  the LZ chain ID. Along similar lines, the caller must provide the
  native fee for this transaction.
Copy link
Contributor

coderabbitai bot commented Sep 2, 2024

Walkthrough

The changes involve significant modifications to the contracts related to the bootstrap process across multiple chains. Key updates include the introduction of new functions for marking bootstraps, enhancements in error handling, and a shift towards a fee-based structure for client chain interactions. Additionally, the initialization processes have been refined, and several redundant elements have been removed to streamline the functionality.

Changes

Files Change Summary
script/11_SetPeers.s.sol, script/14_CorrectBootstrapErrors.s.sol, script/7_DeployBootstrap.s.sol Modifications to funding mechanisms, initialization logic, and the introduction of new state variables for enhanced client chain management.
src/core/Bootstrap.sol, src/core/ExocoreGateway.sol, src/interfaces/IExocoreGateway.sol Updates to constructors and functions to improve error handling and introduce new parameters for client chain gateway functionality.
src/libraries/Errors.sol, src/storage/BootstrapStorage.sol, src/storage/ExocoreGatewayStorage.sol Removal of obsolete error declarations and mappings, along with the addition of new events to improve communication of bootstrap states.
test/foundry/unit/Bootstrap.t.sol, test/foundry/unit/ExocoreGateway.t.sol Enhancements to test cases reflecting new initialization logic and error handling, including tests for edge cases with zero addresses and malformed data.
test/mocks/ExocoreGatewayMock.sol Simplification of bootstrap logic with new internal functions and improved error handling for client chain interactions.

Assessment against linked issues

Objective Addressed Explanation
Mark bootstrap failures should not halt the system (#89)

🐇 "In the meadow, we hop and play,
New changes come to brighten the day.
With fees and logic, all in line,
Bootstraps are smooth, oh how divine!
Errors now handled with grace,
A joyful leap in this code space!" 🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (2)
test/mocks/ExocoreGatewayMock.sol (2)

110-118: Looks good, but consider removing the payable modifier and handling the failure case of _markBootstrap call.

The code changes are approved with a few suggestions:

  • The function is marked as payable, which is not necessary as it doesn't receive any Ether. Consider removing the payable modifier.
  • The function doesn't handle the failure case of _markBootstrap call. Consider adding a check for the success of _markBootstrap call and reverting with a specific error if it fails.

120-122: Looks good, but consider removing the payable modifier and handling the failure case of _markBootstrap call.

The code changes are approved with a few suggestions:

  • The function is marked as payable, which is not necessary as it doesn't receive any Ether. Consider removing the payable modifier.
  • The function doesn't handle the failure case of _markBootstrap call. Consider adding a check for the success of _markBootstrap call and reverting with a specific error if it fails.
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 021ea9d and 662835d.

Files selected for processing (13)
  • script/11_SetPeers.s.sol (2 hunks)
  • script/14_CorrectBootstrapErrors.s.sol (3 hunks)
  • script/7_DeployBootstrap.s.sol (3 hunks)
  • script/integration/1_DeployBootstrap.s.sol (1 hunks)
  • src/core/Bootstrap.sol (4 hunks)
  • src/core/ExocoreGateway.sol (1 hunks)
  • src/interfaces/IExocoreGateway.sol (1 hunks)
  • src/libraries/Errors.sol (1 hunks)
  • src/storage/BootstrapStorage.sol (1 hunks)
  • src/storage/ExocoreGatewayStorage.sol (1 hunks)
  • test/foundry/unit/Bootstrap.t.sol (9 hunks)
  • test/foundry/unit/ExocoreGateway.t.sol (1 hunks)
  • test/mocks/ExocoreGatewayMock.sol (1 hunks)
Files skipped from review due to trivial changes (1)
  • src/libraries/Errors.sol
Additional context used
Learnings (6)
script/14_CorrectBootstrapErrors.s.sol (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#16
File: test/foundry/Bootstrap.t.sol:156-156
Timestamp: 2024-06-03T13:57:54.251Z
Learning: The `setClientChainGatewayLogic` method in `src/core/Bootstrap.sol` now includes checks to ensure the address is non-zero and the initialization data is at least 4 bytes long to include the function selector.
script/integration/1_DeployBootstrap.s.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#23
File: script/integration/1_DeployBootstrap.s.sol:19-19
Timestamp: 2024-06-18T08:22:01.846Z
Learning: The `1_DeployBootstrap.s.sol` script in the Exocore contracts repository is intended for testing purposes, and its import paths, such as for `MyToken`, are structured accordingly.
test/mocks/ExocoreGatewayMock.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#23
File: test/mocks/ExocoreGatewayMock.sol:0-0
Timestamp: 2024-06-17T07:43:43.986Z
Learning: User adu-web3 suggests that the `chainToBootstrapped[clientChainId]` marking in `ExocoreGatewayMock.sol` has no significant effects beyond setting a state and that synchronization issues are currently not a concern. They are open to implementing a response check in the future if necessary.
src/core/Bootstrap.sol (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#16
File: test/foundry/Bootstrap.t.sol:156-156
Timestamp: 2024-06-03T13:57:54.251Z
Learning: The `setClientChainGatewayLogic` method in `src/core/Bootstrap.sol` now includes checks to ensure the address is non-zero and the initialization data is at least 4 bytes long to include the function selector.
test/foundry/unit/ExocoreGateway.t.sol (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#23
File: test/mocks/ExocoreGatewayMock.sol:0-0
Timestamp: 2024-06-17T07:43:43.986Z
Learning: User adu-web3 suggests that the `chainToBootstrapped[clientChainId]` marking in `ExocoreGatewayMock.sol` has no significant effects beyond setting a state and that synchronization issues are currently not a concern. They are open to implementing a response check in the future if necessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#16
File: test/foundry/Bootstrap.t.sol:156-156
Timestamp: 2024-06-03T13:57:54.251Z
Learning: The `setClientChainGatewayLogic` method in `src/core/Bootstrap.sol` now includes checks to ensure the address is non-zero and the initialization data is at least 4 bytes long to include the function selector.
test/foundry/unit/Bootstrap.t.sol (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#16
File: test/foundry/Bootstrap.t.sol:156-156
Timestamp: 2024-06-03T13:57:54.251Z
Learning: The `setClientChainGatewayLogic` method in `src/core/Bootstrap.sol` now includes checks to ensure the address is non-zero and the initialization data is at least 4 bytes long to include the function selector.
Additional comments not posted (38)
script/11_SetPeers.s.sol (2)

5-5: LGTM!

The import statement for GatewayStorage is correctly added.


57-59: LGTM!

The code segment correctly calculates the nativeFee using the quote method from the exocoreGateway and passes it as a value to the markBootstrap function call. This change streamlines the process of marking the bootstrap across all chains by directly utilizing a fee structure instead of a conditional funding mechanism, aligning with the objectives mentioned in the PR summary and linked issues.

src/interfaces/IExocoreGateway.sol (1)

61-66: LGTM!

The new markBootstrap function declaration in the IExocoreGateway interface looks good:

  • The function is properly declared as external and payable, aligning with its purpose of marking the network as bootstrapped on the client chain and requiring a fee to be paid to the LayerZero infrastructure.
  • The function takes a single parameter clientChainId of type uint32, representing the LayerZero chain ID of the client chain.
  • The function is well-documented with NatSpec comments that clearly explain its purpose, prerequisites, and operational details.
  • The function declaration is consistent with the PR objectives and the AI-generated summary.
script/14_CorrectBootstrapErrors.s.sol (5)

32-32: LGTM!

The addition of the clientGatewayLogic state variable is approved. It will be useful for storing the address of the client gateway logic contract.


33-33: LGTM!

The addition of the initialization state variable is approved. It will be useful for storing the encoded initialization data for the client gateway.


67-68: LGTM!

The changes to read the clientGatewayLogic address from the configuration and include a requirement check for non-zero address are approved.

These changes align with the learning from PR #16 about including checks for non-zero address in the setClientChainGatewayLogic method. Good job on applying the learnings consistently!


69-69: LGTM!

Populating the initialization variable with the encoded call to the initialize function of the ClientChainGateway contract is approved. This ensures that the contract is correctly initialized during the upgrade process.


89-91: LGTM!

The modifications to the upgradeAndCall method invocation to include the proxyAdmin, clientGateway, and initialization parameters are approved.

These changes align with the PR objectives of introducing new functions for marking bootstraps and refining the initialization process. They enhance the upgrade mechanism by integrating additional logic for client gateway management and initialization, thereby improving the contract's operational robustness.

script/7_DeployBootstrap.s.sol (4)

63-73: LGTM!

The code changes are approved. The necessary contracts for the client chain gateway are being deployed correctly, and the constructor parameters for ClientChainGateway are set appropriately.


75-77: LGTM!

The code changes are approved. The initialization data for the ClientChainGateway contract is correctly prepared using abi.encodeWithSelector, passing the necessary parameters to the initialize function.


93-95: LGTM!

The code changes are approved. The proxyAdmin address, clientGatewayLogic address, and the initialization data are correctly passed as constructor parameters for the Bootstrap contract.


Line range hint 114-118: LGTM!

The code changes are approved. The addresses of the deployed contracts related to the client chain, including capsuleImplementation, capsuleBeacon, and clientGatewayLogic, are correctly serialized using vm.serializeAddress and stored in the clientChainContracts variable.

src/storage/ExocoreGatewayStorage.sol (1)

Line range hint 1-1: Verify the impact of removing the chainToBootstrapped mapping.

The removal of the chainToBootstrapped mapping is a significant change that can affect how the contract handles bootstrap requests and maintains a record of bootstrapped chains. Please ensure that:

  1. The contract's functionality is not adversely affected by the removal of this mapping.
  2. There is an alternative mechanism in place to track and manage bootstrap requests for client chains.
  3. The removal of this mapping does not introduce any unintended consequences or vulnerabilities.

To verify the impact of removing the mapping, consider running the following script:

Please review the output of the script and address any potential issues or concerns related to the removal of the chainToBootstrapped mapping.

Verification successful

Removal of chainToBootstrapped Mapping Verified

The removal of the chainToBootstrapped mapping does not adversely affect the contract's functionality. Alternative mechanisms, such as the markBootstrap function and related events and state variables, are in place to handle bootstrapping processes. Ensure that these mechanisms are thoroughly tested to prevent any unintended consequences or vulnerabilities.

  • Consider reviewing the markBootstrap function and related events to ensure they adequately replace the removed mapping's functionality.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash

# Description:
# This script searches for references to the removed `chainToBootstrapped` mapping
# and its usage in the codebase to identify potential issues and ensure that the
# contract's functionality is not adversely affected.

# Test 1: Search for references to the removed mapping
echo "Searching for references to the removed 'chainToBootstrapped' mapping..."
rg --type solidity 'chainToBootstrapped' 

# Test 2: Search for code patterns that may be affected by the removal
echo "Searching for code patterns that may be affected by the removal..."
rg --type solidity -A 5 $'ExocoreGatewayStorage' | rg $'function|event|error'

# Test 3: Search for alternative mechanisms to track and manage bootstrap requests
echo "Searching for alternative mechanisms to track and manage bootstrap requests..."
rg --type solidity -A 5 $'bootstrap'

Length of output: 722


Script:

#!/bin/bash

# Description:
# This script searches for references to the removed `chainToBootstrapped` mapping
# and its usage in the codebase to identify potential issues and ensure that the
# contract's functionality is not adversely affected.

# Test 1: Search for references to the removed mapping in Solidity files
echo "Searching for references to the removed 'chainToBootstrapped' mapping..."
rg 'chainToBootstrapped' --glob '*.sol'

# Test 2: Search for code patterns that may be affected by the removal in Solidity files
echo "Searching for code patterns that may be affected by the removal..."
rg -A 5 'ExocoreGatewayStorage' --glob '*.sol' | rg 'function|event|error'

# Test 3: Search for alternative mechanisms to track and manage bootstrap requests in Solidity files
echo "Searching for alternative mechanisms to track and manage bootstrap requests..."
rg -A 5 'bootstrap' --glob '*.sol'

Length of output: 101425

script/integration/1_DeployBootstrap.s.sol (1)

133-135: Intentional changes for testing context. Verify no unintended side effects.

The addition of address(0x1) and bytes("123456") as parameters to the bootstrap.initialize function call is noted to be irrelevant for the local network testing scenario, as per the comment.

However, it's important to ensure that these changes don't introduce any unintended side effects or impact the contract's behavior in unexpected ways, even in the testing environment.

To verify that the added parameters don't introduce unintended side effects, you can run the following script:

This script searches for occurrences of the bootstrap.initialize function in the codebase and provides additional context around each match. Review the results to ensure that the added parameters are only used in the intended testing context and don't introduce unintended side effects in other parts of the system.

Verification successful

Intentional changes confirmed for testing purposes. No unintended side effects found.

The addition of address(0x1) and bytes("123456") to the bootstrap.initialize function call is consistent across both the integration script and unit tests, indicating that these changes are intentional for specific test scenarios. The varied use of parameters in other test cases further supports that the testing framework is designed to handle different initialization scenarios, reducing the risk of unintended side effects.

  • Files Reviewed:
    • script/integration/1_DeployBootstrap.s.sol
    • test/foundry/unit/Bootstrap.t.sol
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the added parameters to `bootstrap.initialize` don't introduce unintended side effects.

# Test: Search for the `bootstrap.initialize` function usage in the codebase. 
# Expect: No occurrences outside the testing context where the added parameters might introduce unintended side effects.
rg --type solidity -A 5 $'bootstrap\.initialize'

Length of output: 82


Script:

#!/bin/bash
# Description: Verify that the added parameters to `bootstrap.initialize` don't introduce unintended side effects.

# Test: Search for the `bootstrap.initialize` function usage in the codebase.
# Expect: No occurrences outside the testing context where the added parameters might introduce unintended side effects.
rg -A 5 'bootstrap\.initialize'

Length of output: 4773

test/mocks/ExocoreGatewayMock.sol (1)

124-128: LGTM!

The code changes are approved.

src/storage/BootstrapStorage.sol (2)

230-232: LGTM!

The BootstrapNotTimeYet event aligns with the PR objective of preventing bootstrap failures from disrupting the system by providing a mechanism to signal that the bootstrap process cannot proceed yet without treating it as an error.


234-237: LGTM!

The BootstrapUpgradeFailed event aligns with the PR objective of ensuring bootstrap failures do not disrupt the overall system functionality by providing a mechanism to signal the failure of the bootstrap upgrade process without treating it as an error to prevent blocking the system.

src/core/ExocoreGateway.sol (2)

101-106: LGTM!

The code changes are approved.


108-114: LGTM!

The code changes are approved.

src/core/Bootstrap.sol (4)

88-88: LGTM!

The new parameters clientChainGatewayLogic_ and clientChainInitializationData_ are being properly utilized in the initialize function to set up the client chain gateway.


552-578: LGTM!

The changes to the markBootstrapped function introduce more resilient error handling and align with the PR objectives:

  • Emitting a BootstrapNotTimeYet event instead of reverting when the current block timestamp is less than the spawn time allows for more graceful handling of timing issues.
  • Using a try-catch block for the implementation upgrade and emitting a BootstrapUpgradeFailed event on failure enables retries without causing the entire operation to fail.
  • Emitting the Bootstrapped event regardless of the upgrade success or failure ensures consistent event emission.

These changes contribute to avoiding failures in the markBootstrapped process and maintaining the integrity of the system.


Line range hint 595-613: LGTM!

The addition of the _setClientChainGatewayLogic internal function is a good improvement:

  • It encapsulates the logic for setting the client chain gateway, promoting modularity and reusability.
  • The function includes validation checks for the provided parameters, ensuring that the gateway logic address is not zero and the initialization data is of sufficient length.

This modular approach enhances code organization and maintainability.


66-68: Verify the usage of the new constructor parameters in the initialize function.

Ensure that clientChainGatewayLogic_ and clientChainInitializationData_ are properly utilized in the initialize function to set up the client chain gateway.

Run the following script to verify the usage of the new constructor parameters:

Verification successful

Parameters are correctly utilized in the initialize function.

The parameters clientChainGatewayLogic_ and clientChainInitializationData_ are used in the initialize function, specifically in the _setClientChainGatewayLogic function call, confirming their intended usage.

  • clientChainGatewayLogic_ and clientChainInitializationData_ are passed to _setClientChainGatewayLogic.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `clientChainGatewayLogic_` and `clientChainInitializationData_` in the `initialize` function.

# Test: Search for the usage of `clientChainGatewayLogic_` and `clientChainInitializationData_`. Expect: Usage in the `initialize` function.
rg --type solidity -A 5 $'clientChainGatewayLogic_|clientChainInitializationData_'

Length of output: 116


Script:

#!/bin/bash
# Description: Verify the usage of `clientChainGatewayLogic_` and `clientChainInitializationData_` in the `initialize` function.

# Test: Search for the usage of `clientChainGatewayLogic_` and `clientChainInitializationData_`. Expect: Usage in the `initialize` function.
rg -A 5 'clientChainGatewayLogic_|clientChainInitializationData_'

Length of output: 985

test/foundry/unit/ExocoreGateway.t.sol (5)

889-889: New state variable nativeFee added.

The nativeFee variable stores the fee required for calling markBootstrap. This aligns with the PR objective of allowing retries by requiring the caller to provide the native fee for the transaction.


891-892: New custom error NoPeer added.

The NoPeer error is emitted when markBootstrap is called with an unregistered chain ID. This improves error handling and aligns with the PR objective of ensuring that failures in the markBootstrapped process do not disrupt the overall system functionality.


893-896: setUp function updated to initialize nativeFee.

The setUp function now initializes the nativeFee by quoting the fee required for calling markBootstrap with the clientChainId. This setup aligns with the changes made to the markBootstrap function.


903-903: test_Success function updated to pass nativeFee.

The test_Success function now passes the nativeFee when calling markBootstrap. This aligns with the updated markBootstrap function signature that requires the native fee to be provided.


906-909: test_Fail function updated to expect NoPeer error.

The test_Fail function now expects the NoPeer error to be emitted when calling markBootstrap with an unregistered chain ID (clientChainId + 1). This test case verifies the correct error handling for invalid chain IDs.

test/foundry/unit/Bootstrap.t.sol (10)

103-115: LGTM!

The code changes are approved.


129-137: LGTM!

The code changes are approved.


1012-1020: LGTM!

The code changes are approved.


1043-1051: LGTM!

The code changes are approved.


1072-1081: LGTM!

The code changes are approved.


1102-1105: LGTM!

The code changes are approved.


1125-1128: LGTM!

The code changes are approved.


1148-1163: LGTM!

The code changes are approved.


1165-1193: LGTM!

The code changes are approved.


1195-1217: LGTM!

The code changes are approved.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 662835d and 88d41a3.

Files ignored due to path filters (1)
  • docs/architecture.svg is excluded by !**/*.svg
Files selected for processing (1)
  • test/mocks/ExocoreGatewayMock.sol (1 hunks)
Additional context used
Learnings (1)
test/mocks/ExocoreGatewayMock.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#23
File: test/mocks/ExocoreGatewayMock.sol:0-0
Timestamp: 2024-06-17T07:43:43.986Z
Learning: User adu-web3 suggests that the `chainToBootstrapped[clientChainId]` marking in `ExocoreGatewayMock.sol` has no significant effects beyond setting a state and that synchronization issues are currently not a concern. They are open to implementing a response check in the future if necessary.
Additional comments not posted (2)
test/mocks/ExocoreGatewayMock.sol (2)

110-114: LGTM!

The code changes are approved. The function is correctly implemented and follows the best practices.


106-108: Verify the usage of the payable keyword.

The function is marked as payable, indicating that it can accept Ether. However, the received Ether is not used in the function.

Please verify if the payable keyword is intended and if the received Ether is handled correctly in the broader context of the contract.

To verify, run this script to search for the usage of msg.value or address(this).balance:

src/core/Bootstrap.sol Outdated Show resolved Hide resolved
src/core/Bootstrap.sol Show resolved Hide resolved
test/foundry/unit/Bootstrap.t.sol Show resolved Hide resolved
In the case where Bootstrap call first fails and then succeeds.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 88d41a3 and 31fc163.

Files selected for processing (3)
  • src/core/Bootstrap.sol (4 hunks)
  • src/storage/BootstrapStorage.sol (1 hunks)
  • test/foundry/unit/Bootstrap.t.sol (12 hunks)
Files skipped from review as they are similar to previous changes (2)
  • src/core/Bootstrap.sol
  • src/storage/BootstrapStorage.sol
Additional context used
Learnings (1)
test/foundry/unit/Bootstrap.t.sol (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#16
File: test/foundry/Bootstrap.t.sol:156-156
Timestamp: 2024-06-03T13:57:54.251Z
Learning: The `setClientChainGatewayLogic` method in `src/core/Bootstrap.sol` now includes checks to ensure the address is non-zero and the initialization data is at least 4 bytes long to include the function selector.
Additional comments not posted (14)
test/foundry/unit/Bootstrap.t.sol (14)

61-61: LGTM!

The code changes are approved.


893-896: LGTM!

The code changes are approved.


898-916: LGTM!

The code changes are approved.


920-921: LGTM!

The code changes are approved.


943-948: LGTM!

The code changes are approved.


951-956: LGTM!

The code changes are approved.


1020-1028: LGTM!

The code changes are approved.


1051-1059: LGTM!

The code changes are approved.


1080-1089: LGTM!

The code changes are approved.


1110-1113: LGTM!

The code changes are approved.


1133-1136: LGTM!

The code changes are approved.


1156-1170: LGTM!

The code changes are approved.


1173-1201: LGTM!

The code changes are approved.


1203-1225: LGTM!

The code changes are approved.

@adu-web3
Copy link
Collaborator

adu-web3 commented Sep 3, 2024

I have added it back but it does seem redundant. Once the contract is bootstrapped, the function markBootstrapped is not available. Even if it were available, it is removed as a whitelisted function selector.

in this case, it means if someone sent a bootstrap request after the contract has been bootstrapped and upgraded, this request would revert because the markAsBootstrapped function selector has been removed and block the succeeding messages from ExocoreGateway. I think we should revise the BootstrapLzReceiver._lzReceive implementation to emit an event instead of reverting the tx in this case:

function _lzReceive(Origin calldata _origin, bytes calldata payload) internal virtual override {
if (_origin.srcEid != EXOCORE_CHAIN_ID) {
revert UnexpectedSourceChain(_origin.srcEid);
}
_verifyAndUpdateNonce(_origin.srcEid, _origin.sender, _origin.nonce);
Action act = Action(uint8(payload[0]));
if (act == Action.RESPOND) {
revert Errors.BootstrapLzReceiverInvalidAction();
}
bytes4 selector_ = _whiteListFunctionSelectors[act];
if (selector_ == bytes4(0)) {
revert UnsupportedRequest(act);
}
(bool success, bytes memory reason) = address(this).call(abi.encodePacked(selector_, abi.encode(payload[1:])));
if (!success) {
revert RequestOrResponseExecuteFailed(act, _origin.nonce, reason);
}
}

@MaxMustermann2
Copy link
Collaborator Author

MaxMustermann2 commented Sep 3, 2024

in this case, it means if someone sent a bootstrap request after the contract has been bootstrapped and upgraded, this request would revert because the markAsBootstrapped function selector has been removed and block the succeeding messages from ExocoreGateway. I think we should revise the BootstrapLzReceiver._lzReceive implementation to emit an event instead of reverting the tx in this case:

Extremely good catch. Added to the ClientGatewayLzReceiver in 51ad701. This was chosen instead of the BootstrapLzReceiver since the upgrade causes the new implementation to start working.

@adu-web3
Copy link
Collaborator

adu-web3 commented Sep 3, 2024

in this case, it means if someone sent a bootstrap request after the contract has been bootstrapped and upgraded, this request would revert because the markAsBootstrapped function selector has been removed and block the succeeding messages from ExocoreGateway. I think we should revise the BootstrapLzReceiver._lzReceive implementation to emit an event instead of reverting the tx in this case:

Extremely good catch. Added to the ClientGatewayLzReceiver in 51ad701. This was chosen instead of the BootstrapLzReceiver since the upgrade causes the new implementation to start working.

yeah, you are right. ClientGatewayLzReceiver is the one that works after bootstrap.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 31fc163 and 51ad701.

Files selected for processing (3)
  • src/core/ClientChainGateway.sol (2 hunks)
  • src/core/ClientGatewayLzReceiver.sol (1 hunks)
  • test/foundry/unit/Bootstrap.t.sol (12 hunks)
Files skipped from review as they are similar to previous changes (1)
  • test/foundry/unit/Bootstrap.t.sol
Additional context used
Learnings (1)
src/core/ClientChainGateway.sol (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#16
File: test/foundry/Bootstrap.t.sol:156-156
Timestamp: 2024-06-03T13:57:54.251Z
Learning: The `setClientChainGatewayLogic` method in `src/core/Bootstrap.sol` now includes checks to ensure the address is non-zero and the initialization data is at least 4 bytes long to include the function selector.
Additional comments not posted (3)
src/core/ClientChainGateway.sol (2)

74-75: LGTM!

The code changes are approved. The _whiteListFunctionSelectors entry for Action.REQUEST_MARK_BOOTSTRAP is correctly updated to use the afterReceiveMarkBootstrapRequest function selector for processing bootstrap requests.


Line range hint 88-102: Verify the impact of not clearing the bootstrap-related function selector.

The _clearBootstrapData function no longer clears the _whiteListFunctionSelectors entry for Action.REQUEST_MARK_BOOTSTRAP. Ensure that this change does not have any unintended consequences on the bootstrap process.

Run the following script to verify the impact:

src/core/ClientGatewayLzReceiver.sol (1)

330-336: LGTM!

The new afterReceiveMarkBootstrapRequest function is implemented correctly to handle the scenario where a mark-bootstrap response is received after the contract is already bootstrapped. The function silently ignores the call and emits a BootstrappedAlready event to signal that the bootstrap process has already been completed. This implementation prevents nonce mismatches and system halts that could occur if the function were to fail.

The onlyCalledFromThis and whenNotPaused modifiers ensure that the function can only be called from within the contract and when the contract is not paused.

The comments provide clear and concise documentation for the function's purpose and behavior.

Copy link
Collaborator

@adu-web3 adu-web3 left a comment

Choose a reason for hiding this comment

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

LGTM, it would be perfect if we could add some tests for the newly added message handler

src/core/ClientGatewayLzReceiver.sol Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 51ad701 and f318b7e.

Files selected for processing (1)
  • test/foundry/unit/Bootstrap.t.sol (12 hunks)
Files skipped from review as they are similar to previous changes (1)
  • test/foundry/unit/Bootstrap.t.sol

@MaxMustermann2
Copy link
Collaborator Author

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Sep 4, 2024

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f318b7e and 65debfd.

Files selected for processing (1)
  • test/foundry/unit/Bootstrap.t.sol (11 hunks)
Files skipped from review as they are similar to previous changes (1)
  • test/foundry/unit/Bootstrap.t.sol

Copy link
Collaborator

@bwhour bwhour left a comment

Choose a reason for hiding this comment

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

it looks good.

@MaxMustermann2 MaxMustermann2 added this pull request to the merge queue Sep 5, 2024
Merged via the queue into ExocoreNetwork:main with commit c1667a1 Sep 5, 2024
7 checks passed
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.

Mark bootstrap failures should not halt the system
3 participants