feat: add wind down contracts #1

0xRaccoon · 2024-10-03T19:08:20Z

The purpose is to allow users to claim their balances of eth and ERC20 tokens with a prior snapshot of their balances.

Currently, the assets to be claimed by the users are located in 2 contracts in mainnet:

The eth is located in the PGN’s Optimism Portal in L1 contract
The ERC20 tokens DAI, USDC, USDT, and GTC are located in the L1StandardBridge

A snapshot of the users' eth balance of OptimismPortal and snapshot of the users' ERC20 balances of L1StandardBridge will be taken and the Merkle tree, root, and proofs for users will be generated off-chain.

The approach consists of a contract BalanceClaimer that contains the bytes32 merkle root value, all the Merkle verification logic, and claims methods. To achieve this approach, it is be necessary to update L1StandardBridge and the OptimisimPotal contracts and add the following methods:

L1StandardBridge: withdrawErc20Balance only called by BalanceClaimer contract after claim verification to send the user token balances.
OptimismPortal: withdrawEthBalance only called by BalanceClaimer contract after claim verification to send the user eth balance.

The claim method is permissionless and can be called by any address.

Signed-off-by: 0xRaccoon <[email protected]>

…ridge Signed-off-by: 0xRaccoon <[email protected]>

Signed-off-by: 0xRaccoon <[email protected]>

agusduha · 2024-10-10T13:48:50Z

packages/contracts-bedrock/src/L1/OptimismPortal.sol

+        (bool success,) = _user.call{ value: _ethBalance }("");
+        if (!success) {
+            revert IEthBalanceWithdrawer.EthTransferFailed();
+        }


They have a SafeSend lib that they use for this

agusduha · 2024-10-10T13:52:54Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

+        address indexed user, uint256 ethBalance, IBalanceWithdrawer.Erc20BalanceClaim[] erc20TokenBalances
+    );
+
+    string public constant version = "1.0.0";


Add natspec here:

/// @notice Semantic version. /// @custom:semver 1.0.0-beta.1

agusduha · 2024-10-10T14:00:16Z

packages/contracts-bedrock/src/L1/OptimismPortal.sol

    function initialize(
        IL2OutputOracle _l2Oracle,
        ISystemConfig _systemConfig,
-        ISuperchainConfig _superchainConfig
+        ISuperchainConfig _superchainConfig,
+        address _balanceClaimer


What about using IBalanceClaimer directly?

agusduha · 2024-10-10T14:01:06Z

packages/contracts-bedrock/src/L1/OptimismPortal.sol

@@ -24,6 +24,8 @@ import { ISystemConfig } from "src/L1/interfaces/ISystemConfig.sol";
 import { IResourceMetering } from "src/L1/interfaces/IResourceMetering.sol";
 import { ISuperchainConfig } from "src/L1/interfaces/ISuperchainConfig.sol";
 import { IL1Block } from "src/L2/interfaces/IL1Block.sol";
+import { IEthBalanceWithdrawer } from "src/L1/interfaces/winddown/IEthBalanceWithdrawer.sol";


This interface can be inherited here, the only thing to take into account is that it also has to be added to its interface also.

agusduha · 2024-10-10T14:05:01Z

packages/contracts-bedrock/src/L1/L1StandardBridge.sol

@@ -24,6 +28,8 @@ import { ISystemConfig } from "src/L1/interfaces/ISystemConfig.sol";
 ///         of some token types that may not be properly supported by this contract include, but are
 ///         not limited to: tokens with transfer fees, rebasing tokens, and tokens with blocklists.
 contract L1StandardBridge is StandardBridge, ISemver {


IBalanceWithdrawer should be inherit here and in IL1StandardBridge

agusduha · 2024-10-10T14:13:10Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

+
+/// @title BalanceClaimer
+/// @notice Contract that allows users to claim and withdraw their balances
+contract BalanceClaimer is Initializable, ISemver {


Is it going to be proxied like the others contracts? Missing the natspec

/// @custom:proxied true

0xDiscotech

Looking good so far sir! Well done

I put the focus mainly on the contracts logic, left suggestions mostly related to styling and some NITs.

The tests need to follow their tests naming standard, you can find it here https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/STYLE_GUIDE.md#tests :)

0xDiscotech · 2024-10-10T15:24:13Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

+        uint256 _ethBalance,
+        IBalanceWithdrawer.Erc20BalanceClaim[] calldata _erc20TokenBalances
+    )
+        internal


Why not just doing it public and removing the external one?

Suggested change

internal

public

second this

0xDiscotech · 2024-10-10T15:25:15Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.15;


New contracts are being created under 0.8.25, is there any specific requirement for it to be 0.8.15?

we are going to update this from a very old commit, I preferred to use the same version than the L1StandardBridge and OptmismPortal contracts for consistency.

0xDiscotech · 2024-10-10T15:25:57Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

+    IErc20BalanceWithdrawer public erc20BalanceWithdrawer;
+
+    /// @notice The mapping of users who have claimed their balances
+    mapping(address => bool) public claimed;


If updating the compiler version, the mapping could have named args

Suggested change

mapping(address => bool) public claimed;

mapping(address _user => bool _claimed) public claimed;

since we prefer to deploy using solidity 0.8.15 this naming is not supported in this version

0xDiscotech · 2024-10-10T15:26:36Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

+        if (!_canClaim(_proof, _user, _ethBalance, _erc20TokenBalances)) {
+            revert IBalanceClaimer.NoBalanceToClaim();


Just a NIT, but I'm very much in favor of inlined if's, wdyt?

0xDiscotech · 2024-10-10T15:49:35Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

+contract BalanceClaimer is Initializable, ISemver {
+    /// @notice Emitted when a user claims their balance
+    event BalanceClaimed(
+        address indexed user, uint256 ethBalance, IBalanceWithdrawer.Erc20BalanceClaim[] erc20TokenBalances


Missng args natspec

0xDiscotech · 2024-10-10T16:03:28Z

packages/contracts-bedrock/test/L1/winddown/integration/BalanceClaimer.t.sol

+    }
+
+    /// @dev Test that the claim function reverts when the user is invalid
+    function test_claim_invalid_user_reverts() external {


Suggested change

function test_claim_invalid_user_reverts() external {

function test_claim_reverts_invalidUser() external {

0xDiscotech · 2024-10-10T16:03:43Z

packages/contracts-bedrock/test/L1/winddown/integration/BalanceClaimer.t.sol

+    }
+
+    /// @dev Test that the claim function reverts when the proof is invalid
+    function test_claim_invalid_proof_reverts() external {


Suggested change

function test_claim_invalid_proof_reverts() external {

function test_claim_reverts_invalidProof() external {

0xDiscotech · 2024-10-10T16:04:02Z

packages/contracts-bedrock/test/L1/winddown/integration/BalanceClaimer.t.sol

+    }
+
+    /// @dev Test that the claim function reverts when the eth balance is invalid
+    function test_claim_invalid_eth_balance_reverts() external {


Suggested change

function test_claim_invalid_eth_balance_reverts() external {

function test_claim_reverts_invalidEthBalance() external {

0xDiscotech · 2024-10-10T16:04:21Z

packages/contracts-bedrock/test/L1/winddown/integration/BalanceClaimer.t.sol

+    }
+
+    /// @dev Test that the claim function reverts when the erc20 balance is invalid
+    function test_claim_invalid_erc20_balance_reverts() external {


Suggested change

function test_claim_invalid_erc20_balance_reverts() external {

function test_claim_reverts_erc20BalanceReverts() external {

0xDiscotech · 2024-10-10T16:07:17Z

packages/contracts-bedrock/src/L1/L1StandardBridge.sol


    /// @notice Address of the SuperchainConfig contract.
    ISuperchainConfig public superchainConfig;

    /// @notice Address of the SystemConfig contract.
    ISystemConfig public systemConfig;

+    address public balanceClaimer;


Missing natspec

also storing it casted to IBalanceWithdrawer for neatness :)

+1 to IBalanceWithdrawer

0xmoebius · 2024-10-15T15:22:59Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

+        bytes32[] calldata _proof,
+        address _user,
+        uint256 _ethBalance,
+        IBalanceWithdrawer.Erc20BalanceClaim[] calldata _erc20TokenBalances


as mentioned by @0xDiscotech on L1StandardBridge, i'd rename this argument to _erc20TokenClaim

0xmoebius · 2024-10-15T15:40:56Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

+        if (claimed[_user]) return false;
+        bytes32 _leaf = keccak256(bytes.concat(keccak256(abi.encode(_user, _ethBalance, _erc20TokenBalances))));
+        _canClaimTokens = MerkleProof.verify(_proof, root, _leaf);
+    }


is there a chance someone may need two claims to fully withdraw their balances? if so, the claimed check wouldn't allow a second claim.

no, the claim should be just one per account, the merkle tree should include all the tokens and eth information for the user to claim

0xmoebius · 2024-10-15T15:46:06Z

packages/contracts-bedrock/src/L1/interfaces/winddown/IBalanceWithdrawer.sol

+
+    function balanceClaimer() external view returns (address);


missing natspec

will add the natspec in the contract since it seems to be the practice used in this repo

0xmoebius · 2024-10-15T15:46:49Z

packages/contracts-bedrock/src/L1/OPContractsManager.sol

@@ -33,6 +33,7 @@ import { IL1CrossDomainMessenger } from "src/L1/interfaces/IL1CrossDomainMesseng
 import { IL1ERC721Bridge } from "src/L1/interfaces/IL1ERC721Bridge.sol";
 import { IL1StandardBridge } from "src/L1/interfaces/IL1StandardBridge.sol";
 import { IOptimismMintableERC20Factory } from "src/universal/interfaces/IOptimismMintableERC20Factory.sol";
+import { IBalanceClaimer } from "src/L1/interfaces/winddown/IBalanceClaimer.sol";


the BalanceClaimer and BalanceWithdrawer interfaces are really confusing

0xmoebius · 2024-10-15T15:49:03Z

packages/contracts-bedrock/src/L1/OptimismPortal.sol

+    address public balanceClaimer;
+


missing natspec

i'd store the address casted to an interface like the rest of contracts for neatness

turtlemoji · 2024-10-16T17:46:48Z

packages/contracts-bedrock/src/L1/winddown/BalanceClaimer.sol

+        returns (bool _canClaimTokens)
+    {
+        if (claimed[_user]) return false;
+        bytes32 _leaf = keccak256(bytes.concat(keccak256(abi.encode(_user, _ethBalance, _erc20TokenBalances))));


do we have an example merkle tree / data created via the offchain script to test?

yes we tested with an example, the plan is to test all the leaves with a script

0xRaccoon · 2024-10-21T16:59:41Z

PR moved to: #3

feat: add wind down contracts

a66a549

Signed-off-by: 0xRaccoon <[email protected]>

0xRaccoon requested review from 0xGorilla, nemuiikiryou, skeletor-spaceman and 0xnigir1 October 3, 2024 19:08

0xRaccoon added 3 commits October 3, 2024 16:33

fix: add version TODOs

e393772

Signed-off-by: 0xRaccoon <[email protected]>

fix: add balanceClaimer storage var to OptimismPortal and L1StandardB…

e6ab65f

…ridge Signed-off-by: 0xRaccoon <[email protected]>

fix: format

c69ad8a

Signed-off-by: 0xRaccoon <[email protected]>

0xRaccoon changed the base branch from develop to feat/wind-down October 4, 2024 11:48

0xRaccoon added 10 commits October 4, 2024 15:08

chore: update deploy scripts

748a942

Signed-off-by: 0xRaccoon <[email protected]>

feat: made balance claimer and withdrawer interfaces token agnostic

040ce16

Signed-off-by: 0xRaccoon <[email protected]>

test: add L1StandardBridge withdrawErc20Balance unit tests

0c71242

Signed-off-by: 0xRaccoon <[email protected]>

fix: canClaim function parameters order

a4c278f

Signed-off-by: 0xRaccoon <[email protected]>

test: add OptimismPortal withdrawEthBalance unit tests

3397063

Signed-off-by: 0xRaccoon <[email protected]>

fix: removed unused imports

e9c3653

Signed-off-by: 0xRaccoon <[email protected]>

test: add BalanceClaimer unit tests

f8e39b9

Signed-off-by: 0xRaccoon <[email protected]>

test: add BalanceClaimer unit tests and assertions

b200a82

Signed-off-by: 0xRaccoon <[email protected]>

test: add BalanceClaimer integration tests

54dfd4e

Signed-off-by: 0xRaccoon <[email protected]>

chore: updated scripts

06a8d10

Signed-off-by: 0xRaccoon <[email protected]>

0xRaccoon requested a review from agusduha October 9, 2024 18:21

0xRaccoon added 4 commits October 9, 2024 22:27

chore: update scripts

9162827

Signed-off-by: 0xRaccoon <[email protected]>

fix: updated tests

1f3b30d

Signed-off-by: 0xRaccoon <[email protected]>

fix: update snapshots

eee263b

Signed-off-by: 0xRaccoon <[email protected]>

fix: remove comments

fb65c1c

Signed-off-by: 0xRaccoon <[email protected]>

agusduha reviewed Oct 10, 2024

View reviewed changes

0xDiscotech reviewed Oct 10, 2024

View reviewed changes

turtlemoji self-requested a review October 14, 2024 16:28

0xRaccoon requested a review from 0xmoebius October 14, 2024 16:35

0xmoebius reviewed Oct 15, 2024

View reviewed changes

turtlemoji reviewed Oct 16, 2024

View reviewed changes

0xRaccoon closed this Oct 21, 2024

0xRaccoon deleted the feat/add-wind-down-contracts branch October 22, 2024 11:05

0xRaccoon mentioned this pull request Oct 28, 2024

feat: add BalanceClaimer and contracts update #3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add wind down contracts #1

feat: add wind down contracts #1

0xRaccoon commented Oct 3, 2024 •

edited

Loading

agusduha Oct 10, 2024

agusduha Oct 10, 2024

agusduha Oct 10, 2024

turtlemoji Oct 16, 2024

agusduha Oct 10, 2024

agusduha Oct 10, 2024 •

edited

Loading

agusduha Oct 10, 2024

0xDiscotech left a comment •

edited

Loading

0xDiscotech Oct 10, 2024

0xmoebius Oct 15, 2024

0xDiscotech Oct 10, 2024

0xRaccoon Oct 16, 2024

0xDiscotech Oct 10, 2024

0xRaccoon Oct 16, 2024

0xDiscotech Oct 10, 2024

0xDiscotech Oct 10, 2024

0xDiscotech Oct 10, 2024

0xDiscotech Oct 10, 2024

0xDiscotech Oct 10, 2024

0xDiscotech Oct 10, 2024

0xDiscotech Oct 10, 2024 •

edited

Loading

0xmoebius Oct 15, 2024 •

edited

Loading

turtlemoji Oct 16, 2024

0xmoebius Oct 15, 2024

0xmoebius Oct 15, 2024

0xRaccoon Oct 16, 2024

0xmoebius Oct 15, 2024

0xRaccoon Oct 16, 2024

0xmoebius Oct 15, 2024

0xmoebius Oct 15, 2024

turtlemoji Oct 16, 2024

turtlemoji Oct 16, 2024

0xRaccoon Oct 16, 2024

0xRaccoon commented Oct 21, 2024

		@@ -0,0 +1,118 @@
		// SPDX-License-Identifier: MIT
		pragma solidity 0.8.15;

	mapping(address => bool) public claimed;
	mapping(address _user => bool _claimed) public claimed;

		if (!_canClaim(_proof, _user, _ethBalance, _erc20TokenBalances)) {
		revert IBalanceClaimer.NoBalanceToClaim();

	function test_claim_invalid_user_reverts() external {
	function test_claim_reverts_invalidUser() external {

	function test_claim_invalid_proof_reverts() external {
	function test_claim_reverts_invalidProof() external {

	function test_claim_invalid_eth_balance_reverts() external {
	function test_claim_reverts_invalidEthBalance() external {

	function test_claim_invalid_erc20_balance_reverts() external {
	function test_claim_reverts_erc20BalanceReverts() external {

feat: add wind down contracts #1

feat: add wind down contracts #1

Conversation

0xRaccoon commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agusduha Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xDiscotech left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xDiscotech Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

0xmoebius Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xRaccoon commented Oct 21, 2024

0xRaccoon commented Oct 3, 2024 •

edited

Loading

agusduha Oct 10, 2024 •

edited

Loading

0xDiscotech left a comment •

edited

Loading

0xDiscotech Oct 10, 2024 •

edited

Loading

0xmoebius Oct 15, 2024 •

edited

Loading