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/native restaking withdraw #10

Merged
merged 100 commits into from
Jul 17, 2024

Conversation

wewecalibrate
Copy link
Contributor

@wewecalibrate wewecalibrate commented May 31, 2024

Native restaking withdraw flow implemented

Summary by CodeRabbit

  • New Features

    • Introduced new events and functions in ExoCapsule for withdrawal actions, restaking activation, and ETH handling outside the Beacon Chain.
  • Bug Fixes

    • Enhanced error handling in ExoCapsule to improve withdrawal process reliability.
  • Refactor

    • Updated NativeRestakingController to handle both partial and full withdrawals with new proof types.
    • Renamed and restructured contracts and functions in tests to better reflect their purposes and improve clarity.
  • Chores

    • Removed the unused GWEI_TO_WEI constant from ClientChainGatewayStorage to clean up the codebase.

adu-web3 and others added 30 commits May 13, 2024 14:31
Copy link

Linting failed. Please check the logs.

Copy link

The build has failed. Please check the logs.

Copy link

Linting failed. Please check the logs.

Copy link

The build has failed. Please check the logs.

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: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8e27ced and 260f6de.

Files selected for processing (1)
  • src/core/ExoCapsule.sol (7 hunks)
Additional context used
GitHub Check: Foundry project
src/core/ExoCapsule.sol

[failure] 282-282:
Variable "withdrawalTimestamp" is unused

Additional comments not posted (15)
src/core/ExoCapsule.sol (15)

25-28: Event Addition Approved: PartialWithdrawalRedeemed

The PartialWithdrawalRedeemed event is correctly added and follows Solidity event naming conventions.


30-32: Event Addition Approved: FullWithdrawalRedeemed

The FullWithdrawalRedeemed event is correctly added and follows Solidity event naming conventions.


34-34: Event Addition Approved: RestakingActivated

The RestakingActivated event is correctly added and follows Solidity event naming conventions.


36-36: Event Addition Approved: NonBeaconChainETHReceived

The NonBeaconChainETHReceived event is correctly added and follows Solidity event naming conventions.


38-38: Event Addition Approved: NonBeaconChainETHWithdrawn

The NonBeaconChainETHWithdrawn event is correctly added and follows Solidity event naming conventions.


42-42: Error Addition Approved: InvalidHistoricalSummaries

The InvalidHistoricalSummaries error is correctly added and follows Solidity error naming conventions.


45-45: Error Addition Approved: WithdrawalAlreadyProven

The WithdrawalAlreadyProven error is correctly added and follows Solidity error naming conventions.


68-71: Function Addition Approved: receive

The receive function is correctly added and handles receiving ETH properly.


Line range hint 72-82:
Function Changes Approved: initialize

The initialize function correctly sets up the contract. The emission of the RestakingActivated event is intentional and aligns with the design.


Line range hint 88-128:
Function Changes Approved: verifyDepositProof

The verifyDepositProof function includes necessary checks and updates to verify deposit proofs correctly.


181-186: Function Changes Approved: withdraw

The withdraw function includes necessary checks and updates to handle withdrawals correctly.


190-200: Function Addition Approved: withdrawNonBeaconChainETHBalance

The withdrawNonBeaconChainETHBalance function includes necessary checks and updates to handle non-Beacon Chain ETH withdrawals correctly.


251-256: Function Changes Approved: _sendETH

The _sendETH function includes necessary checks and updates to handle ETH transfers correctly.


296-298: Function Changes Approved: _isActivatedAtEpoch

The _isActivatedAtEpoch function includes necessary checks and updates to verify activation epochs correctly.


Line range hint 300-302:
Function Changes Approved: _isStaleProof

The _isStaleProof function includes necessary checks and updates to verify stale proofs correctly.

src/core/ExoCapsule.sol Outdated Show resolved Hide resolved
src/core/ExoCapsule.sol Outdated Show resolved Hide resolved
Copy link

Linting failed. Please check the logs.

Copy link

The build has failed. Please check the logs.

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 260f6de and cc41593.

Files selected for processing (5)
  • src/core/ExoCapsule.sol (7 hunks)
  • src/libraries/BeaconChainProofs.sol (6 hunks)
  • test/foundry/DepositWithdrawPrinciple.t.sol (8 hunks)
  • test/foundry/ExocoreDeployer.t.sol (6 hunks)
  • test/foundry/unit/ExoCapsule.t.sol (5 hunks)
Files skipped from review as they are similar to previous changes (5)
  • src/core/ExoCapsule.sol
  • src/libraries/BeaconChainProofs.sol
  • test/foundry/DepositWithdrawPrinciple.t.sol
  • test/foundry/ExocoreDeployer.t.sol
  • test/foundry/unit/ExoCapsule.t.sol

Copy link

The tests have failed. Please check the logs.

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 cc41593 and 806c749.

Files selected for processing (2)
  • src/core/NativeRestakingController.sol (3 hunks)
  • test/foundry/DepositWithdrawPrinciple.t.sol (9 hunks)
Files skipped from review as they are similar to previous changes (2)
  • src/core/NativeRestakingController.sol
  • test/foundry/DepositWithdrawPrinciple.t.sol

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)
src/storage/ClientChainGatewayStorage.sol (2)

Line range hint 59-75: Review of Constructor: Ensure robustness and validate parameters.

The constructor initializes several immutable state variables and performs checks to ensure that the addresses are not zero. This is a good practice as it prevents the contract from being deployed with invalid configuration.

However, it would be beneficial to add more detailed error messages to the require statements to aid debugging and provide clearer context in case of deployment failures.

-        require(
-            beaconOracleAddress_ != address(0),
-            "ClientChainGatewayStorage: beacon chain oracle address should not be empty"
-        );
-        require(
-            exoCapsuleBeacon_ != address(0),
-            "ClientChainGatewayStorage: the exoCapsuleBeacon address for beacon proxy should not be empty"
-        );
+        require(beaconOracleAddress_ != address(0), "ClientChainGatewayStorage: Invalid beaconOracleAddress");
+        require(exoCapsuleBeacon_ != address(0), "ClientChainGatewayStorage: Invalid exoCapsuleBeacon");

Line range hint 79-87: Review of _getCapsule Function: Optimize error handling.

The function _getCapsule retrieves a capsule based on the owner address and reverts if the capsule does not exist. This is a critical function as it directly interacts with potentially sensitive data.

Consider enhancing the error message in the revert statement to include the owner address. This would provide better traceability and debugging information.

-        if (address(capsule) == address(0)) {
-            revert CapsuleNotExist();
-        }
+        if (address(capsule) == address(0)) {
+            revert CapsuleNotExist({
+                owner: owner
+            });
+        }
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 806c749 and 9426325.

Files selected for processing (1)
  • src/storage/ClientChainGatewayStorage.sol (1 hunks)

src/core/ExoCapsule.sol Show resolved Hide resolved
src/core/ExoCapsule.sol Show resolved Hide resolved
src/core/ExoCapsule.sol Show resolved Hide resolved
src/core/ExoCapsule.sol Show resolved Hide resolved
@wewecalibrate
Copy link
Contributor Author

@MaxMustermann2 we don't need it here because, there are 4 _sendETH and we're only sending to capsuleOwner in the withdraw. So we don't need to check again because capsuleOwner is not address(0).
And regarding withdrawNonBeaconETH and withraw available ETH, we only need to check which is done in the external function.

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.

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.

Overall the withdrawal verification logics look good to me, but I think the underlying check is critical and should not be missing

src/core/ExoCapsule.sol Outdated Show resolved Hide resolved
src/core/ExoCapsule.sol Outdated Show resolved Hide resolved
src/core/ExoCapsule.sol Show resolved Hide resolved
src/libraries/BeaconChainProofs.sol Show resolved Hide resolved
src/libraries/BeaconChainProofs.sol Show resolved Hide resolved
@ExocoreNetwork ExocoreNetwork deleted a comment from coderabbitai bot Jul 17, 2024
@wewecalibrate wewecalibrate merged commit 51e4969 into main Jul 17, 2024
6 checks passed
@cloud8little cloud8little deleted the fix/native-restaking-withdraw-in-progress branch July 23, 2024 03:16
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.

4 participants