From deea1235f1107a336feb9f8d9291ab627759c580 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Fri, 16 Feb 2024 16:29:56 +0100 Subject: [PATCH 1/6] perf: sorted withdrawals --- src/PublicAllocator.sol | 15 ++++++--------- src/libraries/ErrorsLib.sol | 7 +++++-- test/PublicAllocatorTest.sol | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/PublicAllocator.sol b/src/PublicAllocator.sol index 404983a..08b6814 100644 --- a/src/PublicAllocator.sol +++ b/src/PublicAllocator.sol @@ -89,16 +89,13 @@ contract PublicAllocator is IPublicAllocatorStaticTyping { Id depositMarketId = depositMarketParams.id(); uint128 totalWithdrawn; + Id id; + Id oldId; for (uint256 i = 0; i < withdrawals.length; i++) { - Id id = withdrawals[i].marketParams.id(); - - // Revert if the market is elsewhere in the list, or is the deposit market. - for (uint256 j = i + 1; j < withdrawals.length; j++) { - if (Id.unwrap(id) == Id.unwrap(withdrawals[j].marketParams.id())) { - revert ErrorsLib.InconsistentWithdrawTo(); - } - } - if (Id.unwrap(id) == Id.unwrap(depositMarketId)) revert ErrorsLib.InconsistentWithdrawTo(); + oldId = id; + id = withdrawals[i].marketParams.id(); + if (Id.unwrap(id) <= Id.unwrap(oldId)) revert ErrorsLib.InconsistentWithdrawals(); + if (Id.unwrap(id) == Id.unwrap(depositMarketId)) revert ErrorsLib.DepositMarketInWithdrawals(); uint128 withdrawnAssets = withdrawals[i].amount; totalWithdrawn += withdrawnAssets; diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index f8a3b9c..e6462b1 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -39,8 +39,11 @@ library ErrorsLib { /// @notice Thrown when the value is already set. error AlreadySet(); - /// @notice Thrown when there are duplicates with nonzero assets in `withdrawTo` arguments. - error InconsistentWithdrawTo(); + /// @notice Thrown when `withdrawals` contains a duplicate or is not sorted. + error InconsistentWithdrawals(); + + /// @notice Thrown when the deposit market is in `withdrawals`. + error DepositMarketInWithdrawals(); /// @notice Thrown when attempting to set max inflow/outflow above the MAX_SETTABLE_FLOW_CAP. error MaxSettableFlowCapExceeded(); diff --git a/test/PublicAllocatorTest.sol b/test/PublicAllocatorTest.sol index a9a957d..2e6282b 100644 --- a/test/PublicAllocatorTest.sol +++ b/test/PublicAllocatorTest.sol @@ -448,7 +448,7 @@ contract PublicAllocatorTest is IntegrationTest { // _setCap(allMarkets[1], CAP2); withdrawals.push(Withdrawal(idleParams, 1e18)); withdrawals.push(Withdrawal(idleParams, 1e18)); - vm.expectRevert(ErrorsLib.InconsistentWithdrawTo.selector); + vm.expectRevert(ErrorsLib.InconsistentWithdrawals.selector); publicAllocator.withdrawTo(withdrawals, allMarkets[0]); } @@ -460,7 +460,7 @@ contract PublicAllocatorTest is IntegrationTest { publicAllocator.setFlowCaps(flowCaps); withdrawals.push(Withdrawal(idleParams, 1e18)); - vm.expectRevert(ErrorsLib.InconsistentWithdrawTo.selector); + vm.expectRevert(ErrorsLib.DepositMarketInWithdrawals.selector); publicAllocator.withdrawTo(withdrawals, idleParams); } From 969db2ec4d2cd2b609b6a97a2edd49e3a60771ff Mon Sep 17 00:00:00 2001 From: Adrien Husson Date: Fri, 16 Feb 2024 20:33:26 +0100 Subject: [PATCH 2/6] misc: tests and doc --- src/PublicAllocator.sol | 6 ++-- src/interfaces/IPublicAllocator.sol | 1 + test/PublicAllocatorTest.sol | 55 +++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/PublicAllocator.sol b/src/PublicAllocator.sol index 1238f29..4bed5c7 100644 --- a/src/PublicAllocator.sol +++ b/src/PublicAllocator.sol @@ -87,11 +87,11 @@ contract PublicAllocator is IPublicAllocatorStaticTyping { uint128 totalWithdrawn; Id id; - Id oldId; + Id prevId; for (uint256 i = 0; i < withdrawals.length; i++) { - oldId = id; + prevId = id; id = withdrawals[i].marketParams.id(); - if (Id.unwrap(id) <= Id.unwrap(oldId)) revert ErrorsLib.InconsistentWithdrawals(); + if (Id.unwrap(id) <= Id.unwrap(prevId)) revert ErrorsLib.InconsistentWithdrawals(); if (Id.unwrap(id) == Id.unwrap(supplyMarketId)) revert ErrorsLib.DepositMarketInWithdrawals(); MORPHO.accrueInterest(withdrawals[i].marketParams); diff --git a/src/interfaces/IPublicAllocator.sol b/src/interfaces/IPublicAllocator.sol index 3fb36ae..bd10552 100644 --- a/src/interfaces/IPublicAllocator.sol +++ b/src/interfaces/IPublicAllocator.sol @@ -64,6 +64,7 @@ interface IPublicAllocatorBase { /// @param supplyMarketParams The market receiving total withdrawn to. /// @dev Will call MetaMorpho's `reallocate`. /// @dev Checks that the public allocator constraints (flows, caps) are respected. + /// @dev Will revert when `withdrawals` contains a duplicate or is not sorted. /// @dev Will revert if a withdrawal amount is larger than available liquidity. function reallocateTo(Withdrawal[] calldata withdrawals, MarketParams calldata supplyMarketParams) external diff --git a/test/PublicAllocatorTest.sol b/test/PublicAllocatorTest.sol index 1e965bd..ca34086 100644 --- a/test/PublicAllocatorTest.sol +++ b/test/PublicAllocatorTest.sol @@ -28,12 +28,34 @@ contract CantReceive { } } +// Withdrawal sorting snippet +library SortWithdrawals { + using MarketParamsLib for MarketParams; + // Sorts withdrawals in-place using gnome sort. + // Does not detect duplicates. + // The sort will not be in-place if you pass a storage array. + function sort(Withdrawal[] memory ws) pure internal returns (Withdrawal[] memory) { + uint256 i; + while (i < ws.length) { + if (i == 0 || Id.unwrap(ws[i].marketParams.id()) >= Id.unwrap(ws[i-1].marketParams.id())) { + i++; + } else { + (ws[i], ws[i - 1]) = (ws[i - 1], ws[i]); + i--; + } + } + return ws; + } +} + + contract PublicAllocatorTest is IntegrationTest { IPublicAllocator public publicAllocator; Withdrawal[] internal withdrawals; FlowConfig[] internal flowCaps; SupplyConfig[] internal supplyCaps; + using SortWithdrawals for Withdrawal[]; using MarketParamsLib for MarketParams; using MorphoBalancesLib for IMorpho; @@ -206,6 +228,10 @@ contract PublicAllocatorTest is IntegrationTest { withdrawals.push(Withdrawal(idleParams, flow)); withdrawals.push(Withdrawal(allMarkets[1], flow)); + Withdrawal[] memory sortedWithdrawals = withdrawals.sort(); + withdrawals[0] = sortedWithdrawals[0]; + withdrawals[1] = sortedWithdrawals[1]; + vm.expectEmit(address(publicAllocator)); emit EventsLib.PublicWithdrawal(idleParams.id(), flow); emit EventsLib.PublicWithdrawal(allMarkets[1].id(), flow); @@ -484,4 +510,33 @@ contract PublicAllocatorTest is IntegrationTest { vm.prank(OWNER); publicAllocator.setFlowCaps(flowCaps); } + + function testReallocateToNotSorted() public { + // Prepare public reallocation from 2 markets to 1 + _setCap(allMarkets[1], CAP2); + + MarketAllocation[] memory allocations = new MarketAllocation[](3); + allocations[0] = MarketAllocation(idleParams, INITIAL_DEPOSIT - 2e18); + allocations[1] = MarketAllocation(allMarkets[0], 1e18); + allocations[2] = MarketAllocation(allMarkets[1], 1e18); + vm.prank(OWNER); + vault.reallocate(allocations); + + flowCaps.push(FlowConfig(idleParams.id(), FlowCap(MAX_SETTABLE_FLOW_CAP, MAX_SETTABLE_FLOW_CAP))); + flowCaps.push(FlowConfig(allMarkets[0].id(), FlowCap(MAX_SETTABLE_FLOW_CAP, MAX_SETTABLE_FLOW_CAP))); + flowCaps.push(FlowConfig(allMarkets[1].id(), FlowCap(MAX_SETTABLE_FLOW_CAP, MAX_SETTABLE_FLOW_CAP))); + vm.prank(OWNER); + publicAllocator.setFlowCaps(flowCaps); + + withdrawals.push(Withdrawal(allMarkets[0],1e18)); + withdrawals.push(Withdrawal(allMarkets[1],1e18)); + Withdrawal[] memory sortedWithdrawals = withdrawals.sort(); + // Created non-sorted withdrawals list + withdrawals[0] = sortedWithdrawals[1]; + withdrawals[1] = sortedWithdrawals[0]; + + publicAllocator.reallocateTo(withdrawals, idleParams); + + } + } From 30ee257235ce88235427b1b871771ec86a398889 Mon Sep 17 00:00:00 2001 From: Adrien Husson Date: Fri, 16 Feb 2024 20:34:21 +0100 Subject: [PATCH 3/6] fix: add expectRevert in testReallocateToNotSorted --- test/PublicAllocatorTest.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/test/PublicAllocatorTest.sol b/test/PublicAllocatorTest.sol index ca34086..77d8874 100644 --- a/test/PublicAllocatorTest.sol +++ b/test/PublicAllocatorTest.sol @@ -535,6 +535,7 @@ contract PublicAllocatorTest is IntegrationTest { withdrawals[0] = sortedWithdrawals[1]; withdrawals[1] = sortedWithdrawals[0]; + vm.expectRevert(ErrorsLib.InconsistentWithdrawals.selector); publicAllocator.reallocateTo(withdrawals, idleParams); } From 8d8b1c2cfe4169e3cfc85d2175f6e6bdeeb79d15 Mon Sep 17 00:00:00 2001 From: MathisGD <74971347+MathisGD@users.noreply.github.com> Date: Sat, 17 Feb 2024 11:07:54 +0100 Subject: [PATCH 4/6] docs: minor improvement Signed-off-by: MathisGD <74971347+MathisGD@users.noreply.github.com> --- src/interfaces/IPublicAllocator.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/src/interfaces/IPublicAllocator.sol b/src/interfaces/IPublicAllocator.sol index bd10552..5b731c8 100644 --- a/src/interfaces/IPublicAllocator.sol +++ b/src/interfaces/IPublicAllocator.sol @@ -65,6 +65,7 @@ interface IPublicAllocatorBase { /// @dev Will call MetaMorpho's `reallocate`. /// @dev Checks that the public allocator constraints (flows, caps) are respected. /// @dev Will revert when `withdrawals` contains a duplicate or is not sorted. + /// @dev Will revert if `withdrawals` contains the supply market. /// @dev Will revert if a withdrawal amount is larger than available liquidity. function reallocateTo(Withdrawal[] calldata withdrawals, MarketParams calldata supplyMarketParams) external From 4bfa39d2f9effe0a444865f8acf235e766f44afe Mon Sep 17 00:00:00 2001 From: MathisGD Date: Sat, 17 Feb 2024 11:19:38 +0100 Subject: [PATCH 5/6] test: minor refactor --- test/PublicAllocatorTest.sol | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/PublicAllocatorTest.sol b/test/PublicAllocatorTest.sol index 77d8874..f355713 100644 --- a/test/PublicAllocatorTest.sol +++ b/test/PublicAllocatorTest.sol @@ -228,17 +228,13 @@ contract PublicAllocatorTest is IntegrationTest { withdrawals.push(Withdrawal(idleParams, flow)); withdrawals.push(Withdrawal(allMarkets[1], flow)); - Withdrawal[] memory sortedWithdrawals = withdrawals.sort(); - withdrawals[0] = sortedWithdrawals[0]; - withdrawals[1] = sortedWithdrawals[1]; - vm.expectEmit(address(publicAllocator)); emit EventsLib.PublicWithdrawal(idleParams.id(), flow); emit EventsLib.PublicWithdrawal(allMarkets[1].id(), flow); emit EventsLib.PublicReallocateTo(sender, allMarkets[0].id(), 2 * flow); vm.prank(sender); - publicAllocator.reallocateTo(withdrawals, allMarkets[0]); + publicAllocator.reallocateTo(withdrawals.sort(), allMarkets[0]); } function testReallocateNetting(uint128 flow) public { From 6cdebfdb801189c73e9921d48208fbb15c2119da Mon Sep 17 00:00:00 2001 From: MathisGD Date: Sat, 17 Feb 2024 11:23:20 +0100 Subject: [PATCH 6/6] chore: fmt --- src/libraries/ErrorsLib.sol | 2 +- test/PublicAllocatorFactoryTest.sol | 8 ++------ test/PublicAllocatorTest.sol | 16 +++++++--------- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index 04ead2f..563bcac 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -25,7 +25,7 @@ library ErrorsLib { /// @notice Thrown when `withdrawals` contains a duplicate or is not sorted. error InconsistentWithdrawals(); - + /// @notice Thrown when the deposit market is in `withdrawals`. error DepositMarketInWithdrawals(); diff --git a/test/PublicAllocatorFactoryTest.sol b/test/PublicAllocatorFactoryTest.sol index 41c6148..3a9d67e 100644 --- a/test/PublicAllocatorFactoryTest.sol +++ b/test/PublicAllocatorFactoryTest.sol @@ -25,9 +25,7 @@ contract PublicAllocatorFactoryTest is IntegrationTest { new PublicAllocatorFactory(address(0)); } - function testCreatePublicAllocator(address initialOwner, address vault, bytes32 salt) - public - { + function testCreatePublicAllocator(address initialOwner, address vault, bytes32 salt) public { vm.assume(address(initialOwner) != address(0)); vm.assume(address(vault) != address(0)); @@ -35,9 +33,7 @@ contract PublicAllocatorFactoryTest is IntegrationTest { address expectedAddress = computeCreate2Address(salt, initCodeHash, address(factory)); vm.mockCall( - metaMorphoFactory, - abi.encodeWithSelector(IMetaMorphoFactory.isMetaMorpho.selector, vault), - abi.encode(true) + metaMorphoFactory, abi.encodeWithSelector(IMetaMorphoFactory.isMetaMorpho.selector, vault), abi.encode(true) ); vm.mockCall(vault, abi.encodeWithSelector(IMetaMorphoBase.MORPHO.selector), abi.encode(morpho)); diff --git a/test/PublicAllocatorTest.sol b/test/PublicAllocatorTest.sol index f355713..08fcaa5 100644 --- a/test/PublicAllocatorTest.sol +++ b/test/PublicAllocatorTest.sol @@ -31,13 +31,14 @@ contract CantReceive { // Withdrawal sorting snippet library SortWithdrawals { using MarketParamsLib for MarketParams; - // Sorts withdrawals in-place using gnome sort. + // Sorts withdrawals in-place using gnome sort. // Does not detect duplicates. // The sort will not be in-place if you pass a storage array. - function sort(Withdrawal[] memory ws) pure internal returns (Withdrawal[] memory) { - uint256 i; + + function sort(Withdrawal[] memory ws) internal pure returns (Withdrawal[] memory) { + uint256 i; while (i < ws.length) { - if (i == 0 || Id.unwrap(ws[i].marketParams.id()) >= Id.unwrap(ws[i-1].marketParams.id())) { + if (i == 0 || Id.unwrap(ws[i].marketParams.id()) >= Id.unwrap(ws[i - 1].marketParams.id())) { i++; } else { (ws[i], ws[i - 1]) = (ws[i - 1], ws[i]); @@ -48,7 +49,6 @@ library SortWithdrawals { } } - contract PublicAllocatorTest is IntegrationTest { IPublicAllocator public publicAllocator; Withdrawal[] internal withdrawals; @@ -524,8 +524,8 @@ contract PublicAllocatorTest is IntegrationTest { vm.prank(OWNER); publicAllocator.setFlowCaps(flowCaps); - withdrawals.push(Withdrawal(allMarkets[0],1e18)); - withdrawals.push(Withdrawal(allMarkets[1],1e18)); + withdrawals.push(Withdrawal(allMarkets[0], 1e18)); + withdrawals.push(Withdrawal(allMarkets[1], 1e18)); Withdrawal[] memory sortedWithdrawals = withdrawals.sort(); // Created non-sorted withdrawals list withdrawals[0] = sortedWithdrawals[1]; @@ -533,7 +533,5 @@ contract PublicAllocatorTest is IntegrationTest { vm.expectRevert(ErrorsLib.InconsistentWithdrawals.selector); publicAllocator.reallocateTo(withdrawals, idleParams); - } - }