Add LM Optimism L1 to L2 bridge interface #840

ogtownsend · 2024-05-10T20:28:35Z

This PR adds the Optimism bridge interface for L1 to L2 transfers. Some important things to note:

The Optimism L1 bridge adapter increments a nonce with each transfer and each time sendERC20 is called. This nonce is included in multiple events throughout the transfer's lifecycle and is how we "match" and link events together to confirm they've reached the various states.
The GetTransfers() function makes use of this nonce and implements the matching logic to link these 3 events together and partition transfers into categories:
- L1's LiquidityManager LiquidityTransferred.bridgeReturnData, which should equal
- OP's StandardBridge event ERC20BridgeFinalized.extraData which should equal
- L2's LiquidityManager LiquidityTransferred.bridgeSpecificData
See the TODO: Applying the 1.2x gas... comment on how we should think about adjusting the gas limit appropriately to account for how OP's native bridge dynamically burns gas

github-actions · 2024-05-17T19:50:13Z

LCOV of commit `8e0e3d2` during Solidity Foundry #4572

Summary coverage rate:
  lines......: 98.8% (1251 of 1266 lines)
  functions..: 96.4% (266 of 276 functions)
  branches...: 92.2% (507 of 550 branches)

Files changed coverage rate: n/a

ogtownsend · 2024-05-17T22:53:36Z

core/services/ocr2/plugins/liquiditymanager/bridge/arb/l1_to_l2.go

+		// TODO: what if a rebalance is triggered at T=0, the sent log is emitted at t=1
+		// and the DepositFinalized log is emitted at T=2, and our query goes back to T=2. There will be
+		// 1 DepositFinalized log and 0 sent logs. Maybe drop this condition and just do the matching


cc @makramkd , we chatted about this in one of our meetings, I'll probably tackle this when I work on the arbitrum side of things

ogtownsend · 2024-05-17T22:54:23Z

core/services/ocr2/plugins/liquiditymanager/bridge/common/common.go

+	// TODO: these index values might need to be updated when Ryan makes changes to the LM contract and event fields
+	// LiquidityTransferredToChainSelectorTopicIndex is the index of the topic in the LiquidityTransferred event
+	// that contains the "to" chain selector.


FYI @RyanRHall , these are the indices that might change once we update the contract events and may impact the log poller queries

ogtownsend · 2024-05-17T22:55:20Z

core/services/ocr2/plugins/liquiditymanager/bridge/opstack/common.go

+	ERC20BridgeFinalizedTopic = optimism_standard_bridge.OptimismStandardBridgeERC20BridgeFinalized{}.Topic()
+
+	// Optimism contract addresses: https://docs.optimism.io/chain/addresses
+	// TODO: confirm that we're able to use these L1 proxy addresses


@makramkd or @RyanRHall do you know how often these proxy addresses change? Can we use these hardcoded proxy addresses in log poller queries reliably, or do they need to be fetched programmatically?

ogtownsend · 2024-05-17T22:56:09Z

core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l1_to_l2.go

+	l1FilterName := fmt.Sprintf("OptimismL1ToL2Bridge-L1-LiquidityManager:%s-Local:%s-Remote:%s",
+		l1LiquidityManagerAddress.String(), localChain.Name, remoteChain.Name)
+
+	// TODO: FIXME pass valid context


@amirylm what's the correct way to pass a context down into these bridge interfaces? I see this TODO FIXME inside the arbitrum bridges as well

We should accept a context arg in this function, that should be passed all the way from NewBridge() which currently doesn't accept a context but it should.

You can also create a ticket for fixing contexts cross the liquidity manager instead of fixing it in this PR

Created LM-116

ogtownsend · 2024-05-17T22:56:28Z

core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l1_to_l2.go

+	ctx context.Context,
+	transfer models.Transfer,
+) ([]byte, *big.Int, error) {
+	// TODO: maybe add check if this is a native transfer or ERC20 transfer


we're only transferring ERC20s (WETH) for now right?

ogtownsend · 2024-05-17T22:56:51Z

core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l1_to_l2.go

+	// TODO: Applying the 1.2x gas limit bump here to the fee won't really have an effect on the actual gas burned by
+	//   the OP bridge since the gas units are hardcoded to 1e6 in services/relay/evm/liquidity_manager.go. We should
+	//   instead consider a better way to dynamically bump the gas used by the transmitter, or just hardcode an even
+	//   higher gas limit in the transmitter. The OP team has confirmed that only gas up to the "market rate" will be
+	//   burned, not all gas remaining in the limit.


cc @amirylm and @RyanRHall

ogtownsend · 2024-05-17T22:57:49Z

core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l1_to_l2.go

+		return nil, errors.New("no transfer nonces found in bridge payloads")
+	}
+
+	// TODO: reconfirm that all nonces should be the same across all payloads in a given round for OP


cc @makramkd you said this should hold true, but I wanted to reconfirm, otherwise this QuorumizedBridgePayload() function gets a little wonky since the bridgeReturnData/nonce should be unique for a given transfer

cc @amirylm

ogtownsend · 2024-05-21T01:51:55Z

core/services/ocr2/plugins/liquiditymanager/plugin.go

-	for _, pendingTransfer := range pendingTransfers {
-		unexecuted = append(unexecuted, pendingTransfer)
-	}
+	unexecuted := make([]liquidityrebalancer.UnexecutedTransfer, 0, len(resolvedTransfersQuorum)+len(inflightTransfers)+len(pendingTransfers))


this was just a nit, felt like resolved --> inflight --> pending better represented the chronological ordering/stages that the transfers go through

ogtownsend · 2024-06-03T17:13:42Z

core/services/ocr2/plugins/liquiditymanager/bridge/opstack/l1_to_l2.go

+		log.Fatalf("Failed to pack depositETHTo function call: %v", err)
+	}
+
+	// Estimate gas needed for the depositETHTo call issued from the L1 Bridge Adapter


update comment

ogtownsend force-pushed the LM-35-optimism-l-1-l-2-bridge-offchain-support branch 4 times, most recently from 100cd3b to 510e7af Compare May 17, 2024 19:43

ogtownsend had a problem deploying to sdlc May 17, 2024 19:43 — with GitHub Actions Failure

ogtownsend force-pushed the LM-35-optimism-l-1-l-2-bridge-offchain-support branch from 510e7af to d14abd3 Compare May 17, 2024 22:48

ogtownsend had a problem deploying to sdlc May 17, 2024 22:48 — with GitHub Actions Failure

ogtownsend force-pushed the LM-35-optimism-l-1-l-2-bridge-offchain-support branch from d14abd3 to 4ee8b91 Compare May 17, 2024 22:50

ogtownsend had a problem deploying to sdlc May 17, 2024 22:50 — with GitHub Actions Failure

ogtownsend changed the title ~~[do not merge] wip~~ [do not merge] Add LM Optimism L1 to L2 bridge interface May 17, 2024

ogtownsend commented May 17, 2024

View reviewed changes

ogtownsend force-pushed the LM-35-optimism-l-1-l-2-bridge-offchain-support branch from 4ee8b91 to d8a8145 Compare May 17, 2024 23:03

ogtownsend temporarily deployed to sdlc May 17, 2024 23:03 — with GitHub Actions Inactive

ogtownsend force-pushed the LM-35-optimism-l-1-l-2-bridge-offchain-support branch from d8a8145 to 17e53fe Compare May 20, 2024 23:33

ogtownsend temporarily deployed to sdlc May 20, 2024 23:33 — with GitHub Actions Inactive

ogtownsend commented May 21, 2024

View reviewed changes

ogtownsend changed the title ~~[do not merge] Add LM Optimism L1 to L2 bridge interface~~ Add LM Optimism L1 to L2 bridge interface May 21, 2024

ogtownsend temporarily deployed to sdlc May 21, 2024 21:03 — with GitHub Actions Inactive

ogtownsend marked this pull request as ready for review May 21, 2024 21:03

ogtownsend requested review from RyanRHall, infiloop2, makramkd, RensR, matYang and a team as code owners May 21, 2024 21:03

ogtownsend added 4 commits May 21, 2024 14:13

Add optimism L1 to L2 bridge interface

a2132cf

Add some more tests

3e6529a

Adding changeset

fad3f06

Add contracts changeset

bc5ea8a

ogtownsend added 2 commits May 21, 2024 14:13

Runny prettier on sol changes

a78ac35

Run go linter

8e0e3d2

ogtownsend force-pushed the LM-35-optimism-l-1-l-2-bridge-offchain-support branch from dd16a1b to 8e0e3d2 Compare May 21, 2024 21:23

ogtownsend temporarily deployed to sdlc May 21, 2024 21:23 — with GitHub Actions Inactive

ogtownsend commented Jun 3, 2024

View reviewed changes

ogtownsend mentioned this pull request Jun 4, 2024

Add Optimism L2 to L1 bridge interface offchain support #959

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LM Optimism L1 to L2 bridge interface #840

Add LM Optimism L1 to L2 bridge interface #840

ogtownsend commented May 10, 2024 •

edited

github-actions bot commented May 17, 2024 •

edited

ogtownsend May 17, 2024

ogtownsend May 17, 2024

ogtownsend May 17, 2024

ogtownsend May 17, 2024

amirylm May 20, 2024 •

edited

ogtownsend May 20, 2024

ogtownsend May 17, 2024

ogtownsend May 17, 2024

ogtownsend May 17, 2024

ogtownsend Jun 3, 2024

ogtownsend May 21, 2024

ogtownsend Jun 3, 2024

Add LM Optimism L1 to L2 bridge interface #840

Are you sure you want to change the base?

Add LM Optimism L1 to L2 bridge interface #840

Conversation

ogtownsend commented May 10, 2024 • edited

github-actions bot commented May 17, 2024 • edited

LCOV of commit 8e0e3d2 during Solidity Foundry #4572

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

amirylm May 20, 2024 • edited

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

ogtownsend commented May 10, 2024 •

edited

github-actions bot commented May 17, 2024 •

edited

LCOV of commit `8e0e3d2` during Solidity Foundry #4572

amirylm May 20, 2024 •

edited