From 33e066d3664bf4814b315eb2093fac618dd4bb25 Mon Sep 17 00:00:00 2001 From: Parv Date: Fri, 8 Apr 2022 17:34:27 +0530 Subject: [PATCH 1/2] perf: migration optimisations --- .../migrations/SameAssetTransferMigration.sol | 42 ++++++-------- .../UniswapSingleTransferMigration.sol | 55 +++++++++---------- .../migrations/UniswapSingleTransfer.ts | 6 +- 3 files changed, 44 insertions(+), 59 deletions(-) diff --git a/contracts/migrations/SameAssetTransferMigration.sol b/contracts/migrations/SameAssetTransferMigration.sol index 06a1c633..66d69e1f 100644 --- a/contracts/migrations/SameAssetTransferMigration.sol +++ b/contracts/migrations/SameAssetTransferMigration.sol @@ -29,25 +29,26 @@ contract SameAssetTransferMigration is ReentrancyGuard, Vault, IMigration { mapping(address => SameAssetMigration) private _sameAssetMigration; + modifier onlyDiamond() { + require(msg.sender == diamond, "!diamond"); + _; + } + constructor(address _dao, address _diamond) Vault(_dao, _diamond) {} /// @inheritdoc IMigration function initMigration( address meToken, bytes memory /* encodedArgs */ - ) external override { - require(msg.sender == diamond, "!diamond"); - + ) external override onlyDiamond { MeTokenInfo memory meTokenInfo = IMeTokenRegistryFacet(diamond) .getMeTokenInfo(meToken); - HubInfo memory hubInfo = IHubFacet(diamond).getHubInfo( - meTokenInfo.hubId - ); - HubInfo memory targetHubInfo = IHubFacet(diamond).getHubInfo( - meTokenInfo.targetHubId - ); - require(hubInfo.asset == targetHubInfo.asset, "asset different"); + require( + IHubFacet(diamond).getHubInfo(meTokenInfo.hubId).asset == + IHubFacet(diamond).getHubInfo(meTokenInfo.targetHubId).asset, + "same asset" + ); _sameAssetMigration[meToken].isMigrating = true; } @@ -75,24 +76,19 @@ contract SameAssetTransferMigration is ReentrancyGuard, Vault, IMigration { external override nonReentrant + onlyDiamond returns (uint256 amountOut) { - require(msg.sender == diamond, "!diamond"); - SameAssetMigration storage usts = _sameAssetMigration[meToken]; - require(usts.isMigrating, "!migrating"); - MeTokenInfo memory meTokenInfo = IMeTokenRegistryFacet(diamond) .getMeTokenInfo(meToken); - HubInfo memory hubInfo = IHubFacet(diamond).getHubInfo( - meTokenInfo.hubId - ); HubInfo memory targetHubInfo = IHubFacet(diamond).getHubInfo( meTokenInfo.targetHubId ); - if (!usts.started) { - ISingleAssetVault(hubInfo.vault).startMigration(meToken); - usts.started = true; + if (!_sameAssetMigration[meToken].started) { + ISingleAssetVault( + IHubFacet(diamond).getHubInfo(meTokenInfo.hubId).vault + ).startMigration(meToken); } amountOut = meTokenInfo.balancePooled + meTokenInfo.balanceLocked; @@ -114,16 +110,14 @@ contract SameAssetTransferMigration is ReentrancyGuard, Vault, IMigration { usts = _sameAssetMigration[meToken]; } - // Kicks off meToken warmup period /// @inheritdoc Vault function isValid( address meToken, bytes memory /* encodedArgs */ ) external view override returns (bool) { - MeTokenInfo memory meTokenInfo = IMeTokenRegistryFacet(diamond) - .getMeTokenInfo(meToken); // MeToken not subscribed to a hub - if (meTokenInfo.hubId == 0) return false; + if (IMeTokenRegistryFacet(diamond).getMeTokenInfo(meToken).hubId == 0) + return false; return true; } diff --git a/contracts/migrations/UniswapSingleTransferMigration.sol b/contracts/migrations/UniswapSingleTransferMigration.sol index 9d158a67..223dc017 100644 --- a/contracts/migrations/UniswapSingleTransferMigration.sol +++ b/contracts/migrations/UniswapSingleTransferMigration.sol @@ -27,8 +27,6 @@ contract UniswapSingleTransferMigration is ReentrancyGuard, Vault, IMigration { uint24 fee; // if migration is active and startMigration() has not been triggered bool started; - // meToken has executed the swap and can finish migrating - bool swapped; } mapping(address => UniswapSingleTransfer) private _uniswapSingleTransfers; @@ -43,46 +41,48 @@ contract UniswapSingleTransferMigration is ReentrancyGuard, Vault, IMigration { uint24 public constant MIDFEE = 3000; // 0.3% (Default fee) uint24 public constant MAXFEE = 1e4; // 1% + modifier onlyDiamond() { + require(msg.sender == diamond, "!diamond"); + _; + } + constructor(address _dao, address _diamond) Vault(_dao, _diamond) {} /// @inheritdoc IMigration function initMigration(address meToken, bytes memory encodedArgs) external override + onlyDiamond { - require(msg.sender == diamond, "!diamond"); - MeTokenInfo memory meTokenInfo = IMeTokenRegistryFacet(diamond) .getMeTokenInfo(meToken); - HubInfo memory hubInfo = IHubFacet(diamond).getHubInfo( - meTokenInfo.hubId - ); - HubInfo memory targetHubInfo = IHubFacet(diamond).getHubInfo( - meTokenInfo.targetHubId - ); - require(hubInfo.asset != targetHubInfo.asset, "same asset"); + require( + IHubFacet(diamond).getHubInfo(meTokenInfo.hubId).asset != + IHubFacet(diamond).getHubInfo(meTokenInfo.targetHubId).asset, + "same asset" + ); - uint24 fee = abi.decode(encodedArgs, (uint24)); - UniswapSingleTransfer storage usts = _uniswapSingleTransfers[meToken]; - usts.fee = fee; + _uniswapSingleTransfers[meToken].fee = abi.decode( + encodedArgs, + (uint24) + ); } /// @inheritdoc IMigration function poke(address meToken) external override nonReentrant { - // Make sure meToken is in a state of resubscription UniswapSingleTransfer storage usts = _uniswapSingleTransfers[meToken]; MeTokenInfo memory meTokenInfo = IMeTokenRegistryFacet(diamond) .getMeTokenInfo(meToken); - HubInfo memory hubInfo = IHubFacet(diamond).getHubInfo( - meTokenInfo.hubId - ); + if ( - usts.fee != 0 && // this is to ensure the meToken is resubscribing + usts.fee != 0 && // make sure meToken is in a state of resubscription block.timestamp > meTokenInfo.startTime && // swap can only happen after resubscribe !usts.started // should skip if already started ) { - ISingleAssetVault(hubInfo.vault).startMigration(meToken); + ISingleAssetVault( + IHubFacet(diamond).getHubInfo(meTokenInfo.hubId).vault + ).startMigration(meToken); usts.started = true; _swap(meToken); } @@ -93,22 +93,19 @@ contract UniswapSingleTransferMigration is ReentrancyGuard, Vault, IMigration { external override nonReentrant + onlyDiamond returns (uint256 amountOut) { - require(msg.sender == diamond, "!diamond"); - UniswapSingleTransfer storage usts = _uniswapSingleTransfers[meToken]; - MeTokenInfo memory meTokenInfo = IMeTokenRegistryFacet(diamond) .getMeTokenInfo(meToken); - HubInfo memory hubInfo = IHubFacet(diamond).getHubInfo( - meTokenInfo.hubId - ); HubInfo memory targetHubInfo = IHubFacet(diamond).getHubInfo( meTokenInfo.targetHubId ); - if (!usts.started) { - ISingleAssetVault(hubInfo.vault).startMigration(meToken); + if (!_uniswapSingleTransfers[meToken].started) { + ISingleAssetVault( + IHubFacet(diamond).getHubInfo(meTokenInfo.hubId).vault + ).startMigration(meToken); amountOut = _swap(meToken); } else { // No swap, amountOut = amountIn @@ -206,8 +203,6 @@ contract UniswapSingleTransferMigration is ReentrancyGuard, Vault, IMigration { sqrtPriceLimitX96: 0 }); - usts.swapped = true; - // The call to `exactInputSingle` executes the swap amountOut = _router.exactInputSingle(params); diff --git a/test/contracts/migrations/UniswapSingleTransfer.ts b/test/contracts/migrations/UniswapSingleTransfer.ts index 79e96461..e3afafbd 100644 --- a/test/contracts/migrations/UniswapSingleTransfer.ts +++ b/test/contracts/migrations/UniswapSingleTransfer.ts @@ -64,10 +64,9 @@ const setup = async () => { let encodedVaultDAIArgs: string; let encodedVaultWETHArgs: string; let block; - let migrationDetails: [number, boolean, boolean] & { + let migrationDetails: [number, boolean] & { fee: number; started: boolean; - swapped: boolean; }; before(async () => { @@ -337,7 +336,6 @@ const setup = async () => { .to.emit(meTokenRegistry, "UpdateBalances"); migrationDetails = await migration.getDetails(meToken.address); expect(migrationDetails.started).to.be.equal(true); - expect(migrationDetails.swapped).to.be.equal(true); }); it("should be able to call when migration already started, but wont run startMigration()", async () => { const tx = await migration.poke(meToken.address); @@ -380,7 +378,6 @@ const setup = async () => { migrationDetails = await migration.getDetails(meToken.address); expect(migrationDetails.fee).to.equal(0); expect(migrationDetails.started).to.equal(false); - expect(migrationDetails.swapped).to.equal(false); }); it("should revert before endTime", async () => { let meTokenRegistryDetails = await meTokenRegistry.getMeTokenInfo( @@ -455,7 +452,6 @@ const setup = async () => { migrationDetails = await migration.getDetails(meToken.address); expect(migrationDetails.fee).to.equal(0); expect(migrationDetails.started).to.equal(false); - expect(migrationDetails.swapped).to.equal(false); }); describe("During resubscribe", () => { From 32635d67e87a4f9ef89f1ebd4de8bb13f13b15c1 Mon Sep 17 00:00:00 2001 From: Parv Date: Fri, 8 Apr 2022 17:37:00 +0530 Subject: [PATCH 2/2] fix: remove return from `finishMigration` - as this was not used --- contracts/interfaces/IMigration.sol | 5 +---- contracts/migrations/SameAssetTransferMigration.sol | 4 ++-- contracts/migrations/UniswapSingleTransferMigration.sol | 2 +- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/contracts/interfaces/IMigration.sol b/contracts/interfaces/IMigration.sol index e23352af..123aad1b 100644 --- a/contracts/interfaces/IMigration.sol +++ b/contracts/interfaces/IMigration.sol @@ -17,10 +17,7 @@ interface IMigration { /// @notice Method to send assets from migration vault to the vault of the /// target hub /// @param meToken Address of meToken - /// @return amountOut Amount of token returned - function finishMigration(address meToken) - external - returns (uint256 amountOut); + function finishMigration(address meToken) external; /// @notice Method returns bool if migration started /// @param meToken Address of meToken diff --git a/contracts/migrations/SameAssetTransferMigration.sol b/contracts/migrations/SameAssetTransferMigration.sol index 66d69e1f..01c2c82f 100644 --- a/contracts/migrations/SameAssetTransferMigration.sol +++ b/contracts/migrations/SameAssetTransferMigration.sol @@ -77,7 +77,6 @@ contract SameAssetTransferMigration is ReentrancyGuard, Vault, IMigration { override nonReentrant onlyDiamond - returns (uint256 amountOut) { MeTokenInfo memory meTokenInfo = IMeTokenRegistryFacet(diamond) .getMeTokenInfo(meToken); @@ -90,7 +89,8 @@ contract SameAssetTransferMigration is ReentrancyGuard, Vault, IMigration { IHubFacet(diamond).getHubInfo(meTokenInfo.hubId).vault ).startMigration(meToken); } - amountOut = meTokenInfo.balancePooled + meTokenInfo.balanceLocked; + uint256 amountOut = meTokenInfo.balancePooled + + meTokenInfo.balanceLocked; // Send asset to new vault only if there's a migration vault IERC20(targetHubInfo.asset).safeTransfer( diff --git a/contracts/migrations/UniswapSingleTransferMigration.sol b/contracts/migrations/UniswapSingleTransferMigration.sol index 223dc017..cf9bf365 100644 --- a/contracts/migrations/UniswapSingleTransferMigration.sol +++ b/contracts/migrations/UniswapSingleTransferMigration.sol @@ -94,7 +94,6 @@ contract UniswapSingleTransferMigration is ReentrancyGuard, Vault, IMigration { override nonReentrant onlyDiamond - returns (uint256 amountOut) { MeTokenInfo memory meTokenInfo = IMeTokenRegistryFacet(diamond) .getMeTokenInfo(meToken); @@ -102,6 +101,7 @@ contract UniswapSingleTransferMigration is ReentrancyGuard, Vault, IMigration { meTokenInfo.targetHubId ); + uint256 amountOut; if (!_uniswapSingleTransfers[meToken].started) { ISingleAssetVault( IHubFacet(diamond).getHubInfo(meTokenInfo.hubId).vault