[lock/unlock]: support multi-stage finalization in rebalancer contract #613
Conversation
tests encoding and if logic
…438/rebalancer-changes
…438/rebalancer-changes
LCOV of commit
|
## Motivation Optimism ETH transfers need to be rewrapped before we can inject them into the token pool. ## Solution Add a parameter to the `ReceiveLiquidityParams` that indicates whether the Rebalancer contract should re-wrap native after executing a finalization. It is expected that the caller knows what they are doing (i.e DON or owner).
|
I see you updated files related to |
|
I see you updated files related to |
| s_nonceUsed[nonce] = true; | ||
| i_token.safeTransfer(localReceiver, amount); | ||
| ) external override returns (bool) { | ||
| Payload memory payload = abi.decode(bridgeSpecificPayload, (Payload)); |
There was a problem hiding this comment.
nit: wow! ik it's a mock, but still let's use return early to reduce some nesting
| @@ -115,9 +144,10 @@ contract Rebalancer__report is RebalancerSetup { | |||
| } | |||
|
|
|||
| contract Rebalancer_rebalanceLiquidity is RebalancerSetup { | |||
| uint256 internal constant AMOUNT = 12345679; | |||
There was a problem hiding this comment.
static AMOUNT implies these are good fuzz candidates, let's add some fuzzing? =)
There was a problem hiding this comment.
There are potentially opportunities for fuzzing, but I don't think the current tests are useful to make fuzzes. They basically just deal this AMOUNT to the pool and run through the rebalancing process.
Adding fuzz tests is on the docket though, we have tickets for that.
| // approve and liquidity container should transferFrom | ||
| // if we reach this point, the finalization failed. | ||
| // since we don't have enough information to know why it failed, | ||
| // we assume that it failed because the withdrawal was already finalized, |
There was a problem hiding this comment.
instead of assuming, let's explain why this will always be safe to call injectLiquidity here
There was a problem hiding this comment.
This logic will change in upcoming PRs after recent reviews, so stay tuned.
| @@ -4,15 +4,15 @@ pragma solidity 0.8.19; | |||
| import {IBridgeAdapter} from "./interfaces/IBridge.sol"; | |||
| import {IRebalancer} from "./interfaces/IRebalancer.sol"; | |||
| import {ILiquidityContainer} from "./interfaces/ILiquidityContainer.sol"; | |||
|
|
|||
| import {IWrappedNative} from "../ccip/interfaces/IWrappedNative.sol"; | |||
There was a problem hiding this comment.
nit newline after interfaces
| @@ -258,6 +298,11 @@ contract Rebalancer is IRebalancer, OCR3Base { | |||
| ); | |||
| } | |||
|
|
|||
| function _wrapNative(uint256 amount) private { | |||
| IWrappedNative weth = IWrappedNative(address(i_localToken)); | |||
There was a problem hiding this comment.
nit: remove assignment and inline. Might as well remove the function and just inline the single line at that point?
| return _proveWithdrawal(payload); | ||
| } else if (payload.action == FinalizationAction.FinalizeWithdrawal) { | ||
| return _finalizeWithdrawal(payload, localReceiver); | ||
| } else { |
Motivation
PR #606 implements multi-stage finalization in the Optimism L1 bridge adapter, however this is not functional end-to-end because the Rebalancer assumes that finalization happens in a single step.
Solution
Change the bridge adapter interface to return a boolean from
finalizeWithdrawERC20that indicates whether the funds are available for use by the rebalancer.The rest of the changes are to get test coverage for this new interface change.