feat: add bridges #623
feat: add bridges #623
Conversation
| error Paused(); | ||
|
|
||
| /// @notice Thrown when caller is not the authorized L2 CGT Bridge. | ||
| error OnlyL2CGTBridge(); |
There was a problem hiding this comment.
this errors should also be L1CGTBridge_{error}():
| error Paused(); | |
| /// @notice Thrown when caller is not the authorized L2 CGT Bridge. | |
| error OnlyL2CGTBridge(); | |
| error L1CGTBridge_Paused(); | |
| /// @notice Thrown when caller is not the authorized L2 CGT Bridge. | |
| error L1CGTBridge_OnlyL2CGTBridge(); |
| /// @param _amount Amount of CGT tokens to bridge. | ||
| /// @param _minGasLimit Minimum gas limit for the bridge. | ||
| function bridgeCGT(address _to, uint256 _amount, uint32 _minGasLimit) external virtual { | ||
| if (!depositsEnabledL1toL2) revert L1CGTBridge_DepositsDisabled(); |
There was a problem hiding this comment.
Naming here can be confused with Portal's deposits. wdyt?
There was a problem hiding this comment.
Also, why do we need both the paused flag in superchainConfig and this boolean to block the bridge?
If there are already two booleans to control bridging, is it because paused can disable both directions at once?
I think we should choose one option:
- two independent booleans.
paused
| if (!depositsEnabledL1toL2) revert L1CGTBridge_DepositsDisabled(); | ||
| if (superchainConfig.paused(address(this))) revert Paused(); | ||
|
|
||
| cgtToken.safeTransferFrom(msg.sender, address(this), _amount); |
There was a problem hiding this comment.
Just an experimental idea: Could tokens be locked in another contract instead of being transferred to the bridge? Would this bring modularity?
| revert OnlyL1CGTBridge(); | ||
| } | ||
|
|
||
| if (_to == address(this) || _to == address(messenger)) { |
There was a problem hiding this comment.
Think this check can be removed: _to == address(this), except we decide to add a receive function.
|
|
||
| /// @notice Address of the LiquidityController contract. | ||
| /// @custom:network-specific | ||
| ILiquidityController public liquidityController; |
There was a problem hiding this comment.
This can be replaced to use Predeploys lib. e.g: Predeploys.LIQUIDITY_CONTROLLER. As done here.
| /// @notice Tests for the `bridgeCGT` function of the `L1CGTBridge` contract. | ||
| contract L1CGTBridge_BridgeCGT_Test is L1CGTBridge_TestInit { | ||
| /// @notice Tests that bridgeCGT succeeds when called properly. | ||
| function test_bridgeCGT_withRecipient_succeeds(uint256 _amount, uint32 _minGasLimit) external { |
There was a problem hiding this comment.
NITs:
- shouldn't test name be
testFuzz_...? - why using
aliceandbobfixed addresses? Couldn't we fuzz them? - Consider fuzzing the recipient address too.
|
|
||
| // Approve the bridge to spend tokens | ||
| vm.prank(alice); | ||
| cgtToken.approve(address(l1CGTBridge), _amount); |
There was a problem hiding this comment.
MEGA nit: allowance could be set manipulating the storage.
Leave it like this if you want.
| function test_bridgeCGT_whenDepositsDisabled_reverts() external { | ||
| // Disable deposits | ||
| vm.prank(alice); | ||
| l1CGTBridge.setDepositsEnabledL1toL2(false); |
There was a problem hiding this comment.
Could be set by manipulating the storage instead of a call.
| // First, simulate a deposit to set up the contract state | ||
| vm.prank(alice); | ||
| cgtToken.approve(address(l1CGTBridge), BRIDGE_AMOUNT); | ||
|
|
||
| vm.mockCall(address(messenger), abi.encodeWithSelector(ICrossDomainMessenger.sendMessage.selector), ""); | ||
|
|
||
| vm.startPrank(alice, alice); | ||
| l1CGTBridge.bridgeCGT(alice, BRIDGE_AMOUNT, MIN_GAS_LIMIT); | ||
| vm.stopPrank(); |
There was a problem hiding this comment.
same as above, consider setting the storage manually instead of executing calls to isolate the function you're testing.
|
|
||
| /// @notice Tests that setDepositsEnabledL1toL2 reverts when called by unauthorized account. | ||
| function test_setDepositsEnabledL1toL2_whenUnauthorized_reverts() external { | ||
| vm.expectRevert(); |
|
|
||
| /// @notice Tests that setFinalizeEnabledL2toL1 reverts when called by unauthorized account. | ||
| function test_setFinalizeEnabledL2toL1_whenUnauthorized_reverts() external { | ||
| vm.expectRevert(); |
CLOSES OPT-1189