feat: migrating usdc to native flow #24

excaliborr · 2024-05-27T12:30:19Z

🤖 Linear

Closes OPT-73

linear · 2024-05-27T12:39:00Z

OPT-73 USDC Native Migration Flow

L1 Adapter:

receiveSetBurnAmount(uint256 _amount*) external;: Will verify that the calling adapter is the adapter from L2* . Will set the burn amount for that specific adapter, should also set messages as deprecated, this will mean there is no way to restart the adapters. This is the last call in the migration flow so we deprecate here
migrateToNative: Will send a transfer ownership call to the L2 adapter for the bridged token, must be called by the UpgradeManager , will set a circle address who will be the only one allowed to call burnLockedUSDC. Will also stop new bridge messages from being sent from this adapter.
function burnLockedUSDC() external; must exist in the interface for circle to be able to upgrade the OpUSDC contract of the chain it lives on to native USDC should they want to, this function must do the following:
1. Be only callable by an address that Circle specifies closer to the time of the upgrade.
2. Burn the amount of USDC held by the bridge that corresponds precisely to the circulating total supply of bridged USDC established by the supply lock.
3. Cannot use a parameter to pass in the amount, we will use a storage variable
4. Concurrent upgrades cannot happen, circle will be reset to address(0) after this is called

L2 Adapter:

receiveMigrateToNative : Will receive the transfer ownership call from L1, must be called by the l1Adapter and sent from the messenger , transfers ownership of the Bridged USDC on this chain to newOwner. Will also stop new messages from being sent from this adapter. Should set messaging as deprecated

0xDiscotech · 2024-05-28T15:22:38Z

src/contracts/L1OpUSDCBridgeAdapter.sol

- // TODO: Implement this in future PR
+ function migrateToNative(
+ address _messenger,
+ address _circle,


Missing a check over _circle != address(0) that could break the logic

But maybe this check would fit better inside the prepareNativeMigration function

There is a check for this

opUSDC/src/contracts/UpgradeManager.sol

Line 127 in a0eb0dd

if (_migration.circle == address(0) || _migration.executor == address(0)) {

0xDiscotech · 2024-05-28T15:23:11Z

src/contracts/L2OpUSDCBridgeAdapter.sol

+ * @dev This only starts the process, full migration cant finish until L1 receives the message for setting the burn amount
+ * @param _newOwner The address to transfer ownerships to
+ * @param _setBurnAmountMinGasLimit Minimum gas limit that the setBurnAmount message we are sending can be executed on L1


style: linter warning

0xDiscotech · 2024-05-28T15:27:34Z

src/contracts/L1OpUSDCBridgeAdapter.sol

+ if (circle != address(0)) revert IOpUSDCBridgeAdapter_MigrationInProgress();
+
+ circle = _circle;
+ messengerStatus[_messenger] = Status.Deprecated;


Is it worth to create a transitioned state for the 'in upgrade' state?
Because, what happens if the function args are bad and it fails on L2, or even the prepraed migration is wrong?
I think we should allow to re-send the native message while it's paused on L1.

Yea i like this we can set it as Upgrading, and deptrcate it when we receive the setBurnAmount

hexshire · 2024-05-28T18:22:54Z

src/contracts/L1OpUSDCBridgeAdapter.sol

+ );
+
+ emit MigratingToNative(_messenger, _circle);
+ emit CircleSet(_circle);


Why is this event needed if it only used in this function and also present in the event above?

src/contracts/L2OpUSDCBridgeAdapter.sol

hexshire · 2024-05-28T18:32:46Z

src/contracts/L1OpUSDCBridgeAdapter.sol

@@ -78,25 +78,21 @@ contract L1OpUSDCBridgeAdapter is OpUSDCBridgeAdapter, UUPSUpgradeable, IL1OpUSD
 /**
 * @notice Sets the amount of USDC tokens that will be burned when the burnLockedUSDC function is called
 * @param _amount The amount of USDC tokens that will be burned
- * @dev Only callable by the UpgradeManager
+ * @dev Only callable by a whitelisted messenger during its migration process


This change is not applied on the interface.

0xDiscotech · 2024-05-28T22:26:25Z

src/contracts/L2OpUSDCFactory.sol

 // Execute the L2 Adapter initialization transactions
- if (_l2AdapterInitTxs.length > 1) {
+ if (_l2AdapterInitTxs.length > 0) {
 // Initialize L2 adapter
- for (uint256 i = 1; i < _l2AdapterInitTxs.length; i++) {
+ for (uint256 i; i < _l2AdapterInitTxs.length; i++) {


It started from 1 because the item 0 of the array is already used on the initialize tx done when l2 adapter proxy is created

0xDiscotech · 2024-05-28T22:38:06Z

test/unit/L1OpUSDCBridgeAdapter.t.sol

@@ -71,9 +70,9 @@ contract L1OpUSDCBridgeAdapter_Unit_Constructor is Base {

 contract L1OpUSDCBridgeAdapter_Unit_SetBurnAmount is Base {
 /**
- * @notice Check that the function reverts if the sender is not in a deprecated state
+ * @notice Check that the function reverts if the sender is not in a upgrading state


Suggested change

* @notice Check that the function reverts if the sender is not in a upgrading state

* @notice Check that the function reverts if the messenger is not in an upgrading state

0xDiscotech · 2024-05-28T22:41:02Z

test/unit/L1OpUSDCBridgeAdapter.t.sol

@@ -99,7 +98,7 @@ contract L1OpUSDCBridgeAdapter_Unit_SetBurnAmount is Base {
 * @notice Check that the burn amount is set as expected
 */
 function test_setAmount(uint256 _burnAmount) external {
- adapter.forTest_setMessagerStatus(_messenger, IL1OpUSDCBridgeAdapter.Status.Deprecated);
+ adapter.forTest_setMessagerStatus(_messenger, IL1OpUSDCBridgeAdapter.Status.Upgrading);


Would be nice to add the exact same test, but setting Active as status so we also cover the first part of the conditional

this is the burn amount which can only be called when its upgrading, do you mean the other scenario? I believe we have both branches covered

Ah true, I confused 🙏

0xDiscotech · 2024-05-28T22:55:01Z

src/contracts/L1OpUSDCBridgeAdapter.sol

+ if (messengerStatus[_messenger] != Status.Active && messengerStatus[_messenger] != Status.Upgrading) {
+ revert IOpUSDCBridgeAdapter_MessagingDisabled();
+ }
+ if (circle != address(0)) revert IOpUSDCBridgeAdapter_MigrationInProgress();


We need to remove this check right? Otherwise if it's in Upgrading state it will revert here

0xDiscotech

Fantastic job ser! 🚀

hexshire

Outstanding as always!

chore: checkpoint

d77ec69

excaliborr and others added 2 commits May 27, 2024 08:49

chore: finish tests

16cfdf0

Merge branch 'dev' into feat/migrating-to-native

eaafa82

excaliborr marked this pull request as ready for review May 28, 2024 14:50

0xDiscotech reviewed May 28, 2024

View reviewed changes

hexshire reviewed May 28, 2024

View reviewed changes

fix: comments

027af27

0xDiscotech reviewed May 28, 2024

View reviewed changes

fix: comments

1b00792

0xDiscotech reviewed May 28, 2024

View reviewed changes

fix: bug

ee9cc1f

0xDiscotech approved these changes May 28, 2024

View reviewed changes

hexshire approved these changes May 28, 2024

View reviewed changes

excaliborr merged commit 861e489 into dev May 28, 2024
4 checks passed

excaliborr deleted the feat/migrating-to-native branch May 28, 2024 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrating usdc to native flow #24

feat: migrating usdc to native flow #24

excaliborr commented May 27, 2024 •

edited

Loading

linear bot commented May 27, 2024

0xDiscotech May 28, 2024

0xDiscotech May 28, 2024

excaliborr May 28, 2024

0xDiscotech May 28, 2024

0xDiscotech May 28, 2024

0xDiscotech May 28, 2024

excaliborr May 28, 2024

0xDiscotech May 28, 2024

hexshire May 28, 2024 •

edited

Loading

hexshire May 28, 2024

0xDiscotech May 28, 2024

0xDiscotech May 28, 2024

0xDiscotech May 28, 2024

excaliborr May 28, 2024 •

edited

Loading

excaliborr May 28, 2024

0xDiscotech May 28, 2024

0xDiscotech May 28, 2024

0xDiscotech left a comment

hexshire left a comment

	* @notice Check that the function reverts if the sender is not in a upgrading state
	* @notice Check that the function reverts if the messenger is not in an upgrading state

feat: migrating usdc to native flow #24

feat: migrating usdc to native flow #24

Conversation

excaliborr commented May 27, 2024 • edited Loading

🤖 Linear

linear bot commented May 27, 2024

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

hexshire May 28, 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

excaliborr May 28, 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

0xDiscotech left a comment

Choose a reason for hiding this comment

hexshire left a comment

Choose a reason for hiding this comment

excaliborr commented May 27, 2024 •

edited

Loading

hexshire May 28, 2024 •

edited

Loading

excaliborr May 28, 2024 •

edited

Loading