Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use custom storage for lockData #353

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 1 token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
96005
95788
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 2 tokens.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
153358
152900
2 changes: 1 addition & 1 deletion .forge-snapshots/gas overhead of no-op lock.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
58562
58632
2 changes: 1 addition & 1 deletion .forge-snapshots/initialize.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
38009
37964
2 changes: 1 addition & 1 deletion .forge-snapshots/mint with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
307152
306957
2 changes: 1 addition & 1 deletion .forge-snapshots/mint with native token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
275102
274907
2 changes: 1 addition & 1 deletion .forge-snapshots/mint.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
293760
293565
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
49720
49766
Original file line number Diff line number Diff line change
@@ -1 +1 @@
124982
124524
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
109692
109234
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with dynamic fee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
91502
91548
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
49691
49737
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with native.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
49720
49766
26 changes: 16 additions & 10 deletions contracts/PoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {ERC1155} from "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";
import {IERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";
import {PoolId, PoolIdLibrary} from "./types/PoolId.sol";
import {BalanceDelta} from "./types/BalanceDelta.sol";
import {LockData} from "./types/LockData.sol";

/// @notice Holds the state for all pools
contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC1155, IERC1155Receiver {
Expand All @@ -30,7 +31,6 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC1155, IERC1155Rec
using Hooks for IHooks;
using Position for mapping(bytes32 => Position.Info);
using CurrencyLibrary for Currency;
using LockDataLibrary for IPoolManager.LockData;
using FeeLibrary for uint24;

/// @inheritdoc IPoolManager
Expand All @@ -39,9 +39,6 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC1155, IERC1155Rec
/// @inheritdoc IPoolManager
int24 public constant override MIN_TICK_SPACING = 1;

/// @inheritdoc IPoolManager
IPoolManager.LockData public override lockData;

/// @dev Represents the currencies due/owed to each locker.
/// Must all net to zero when the last lock is released.
mapping(address locker => mapping(Currency currency => int256 currencyDelta)) public currencyDelta;
Expand Down Expand Up @@ -138,14 +135,16 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC1155, IERC1155Rec

/// @inheritdoc IPoolManager
function lock(bytes calldata data) external override returns (bytes memory result) {
LockData lockData = LockDataLibrary.getLockData();
Copy link
Member

@ewilz ewilz Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something definitely feels a bit weird to me about using pop/push (write functionality) on a type memory uint256 that actually does storage writes. I wish there was a nice way to return a storage reference/pointer here instead of recaching below. Feels a bit off but can't think of any great solutions...this + update() feel a big "loosey goosey" if you will lol

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it is a bit wonky..let me know if you can think of another way, but I had to move away from a library for storage pointers because we don't yet have the transient keyword.

We can rewrite the lib to use the transient keyword when there is solidity support.

lockData.push(msg.sender);

// the caller does everything in this callback, including paying what they owe via calls to settle
result = ILockCallback(msg.sender).lockAcquired(data);

if (lockData.length == 1) {
if (lockData.nonzeroDeltaCount != 0) revert CurrencyNotSettled();
delete lockData;
lockData = LockDataLibrary.getLockData();
if (lockData.length() == 1) {
if (lockData.nonzeroDeltaCount() != 0) revert CurrencyNotSettled();
lockData.update(0, 0);
} else {
lockData.pop();
}
Expand All @@ -154,15 +153,18 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC1155, IERC1155Rec
function _accountDelta(Currency currency, int128 delta) internal {
if (delta == 0) return;

LockData lockData = LockDataLibrary.getLockData();
address locker = lockData.getActiveLock();
int256 current = currencyDelta[locker][currency];
int256 next = current + delta;

uint128 nonzeroDeltaCount = lockData.nonzeroDeltaCount();

unchecked {
if (next == 0) {
lockData.nonzeroDeltaCount--;
lockData.update(lockData.length(), nonzeroDeltaCount - 1);
} else if (current == 0) {
lockData.nonzeroDeltaCount++;
lockData.update(lockData.length(), nonzeroDeltaCount + 1);
}
}

Expand All @@ -176,7 +178,7 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC1155, IERC1155Rec
}

modifier onlyByLocker() {
address locker = lockData.getActiveLock();
address locker = LockDataLibrary.getLockData().getActiveLock();
if (msg.sender != locker) revert LockedBy(locker);
_;
}
Expand Down Expand Up @@ -411,6 +413,10 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC1155, IERC1155Rec
return value;
}

function getLockData() external view returns (LockData lockData) {
return LockDataLibrary.getLockData();
}

/// @notice receive native tokens for native pools
receive() external payable {}
}
13 changes: 3 additions & 10 deletions contracts/interfaces/IPoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {IFees} from "./IFees.sol";
import {BalanceDelta} from "../types/BalanceDelta.sol";
import {PoolId} from "../types/PoolId.sol";
import {Position} from "../libraries/Position.sol";
import {LockData} from "../types/LockData.sol";

interface IPoolManager is IFees, IERC1155 {
/// @notice Thrown when currencies touched has exceeded max of 256
Expand Down Expand Up @@ -112,19 +113,11 @@ interface IPoolManager is IFees, IERC1155 {
/// @notice Returns the reserves for a given ERC20 currency
function reservesOf(Currency currency) external view returns (uint256);

/// @notice Contains data about pool lockers.
struct LockData {
/// @notice The current number of active lockers
uint128 length;
/// @notice The total number of nonzero deltas over all active + completed lockers
uint128 nonzeroDeltaCount;
}

/// @notice Returns the locker in the ith position of the locker queue.
function getLock(uint256 i) external view returns (address locker);

/// @notice Returns lock data
function lockData() external view returns (uint128 length, uint128 nonzeroDeltaCount);
/// @notice Returns the lock data (length and nonzeroDeltaCount) for the lock data structure.
function getLockData() external view returns (LockData lockData);

/// @notice Initialize the state for a given pool ID
function initialize(PoolKey memory key, uint160 sqrtPriceX96, bytes calldata hookData)
Expand Down
76 changes: 59 additions & 17 deletions contracts/libraries/LockDataLibrary.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,49 +2,91 @@
pragma solidity ^0.8.20;

import {IPoolManager} from "../interfaces/IPoolManager.sol";
import {LockData} from "../types/LockData.sol";

/// @dev This library manages a custom storage implementation for a queue
/// that tracks current lockers. The "sentinel" storage slot for this data structure,
/// always passed in as IPoolManager.LockData storage self, stores not just the current
/// that tracks current lockers. The LOCK_DATA storage slot for this data structure,
/// passed in as a LockData custom type that is a uint256, stores not just the current
/// length of the queue but also the global count of non-zero deltas across all lockers.
/// The values of the data structure start at OFFSET, and each value is a locker address.
/// The values of the data structure start at LOCKERS, and each value is a locker address.
library LockDataLibrary {
uint256 private constant OFFSET = uint256(keccak256("LockData"));
using LockDataLibrary for LockData;

/// @dev Pushes a locker onto the end of the queue, and updates the sentinel storage slot.
function push(IPoolManager.LockData storage self, address locker) internal {
// read current value from the sentinel storage slot
uint128 length = self.length;
uint256 private constant LOCK_DATA = uint256(keccak256("LockData"));
uint256 private constant LOCKERS = uint256(keccak256("Lockers"));

/// @dev Pushes a locker onto the end of the queue at the LOCKERS + length storage slot, and updates the length at the LOCK_DATA storage slot.
function push(LockData lockData, address locker) internal {
// read current length from the LOCK_DATA storage slot
uint128 _length = lockData.length();
unchecked {
uint256 indexToWrite = OFFSET + length; // not in assembly because OFFSET is in the library scope
uint256 lockerSlot = LOCKERS + _length; // not in assembly because LOCKERS is in the library scope

/// @solidity memory-safe-assembly
assembly {
// in the next storage slot, write the locker
sstore(indexToWrite, locker)
sstore(lockerSlot, locker)
}
// update the sentinel storage slot
self.length = length + 1;
lockData.update(_length + 1, lockData.nonzeroDeltaCount());
}
}

/// @dev Pops a locker off the end of the queue. Note that no storage gets cleared.
function pop(IPoolManager.LockData storage self) internal {
function pop(LockData lockData) internal {
unchecked {
self.length--;
lockData.update(lockData.length() - 1, lockData.nonzeroDeltaCount());
}
}

function length(LockData lockData) internal pure returns (uint128 _length) {
/// @solidity memory-safe-assembly
assembly {
_length := shr(128, lockData)
}
}

function nonzeroDeltaCount(LockData lockData) internal pure returns (uint128 _nonzeroDeltaCount) {
/// @solidity memory-safe-assembly
assembly {
_nonzeroDeltaCount := lockData
}
}

function update(LockData lockData, uint128 _length, uint128 _nonzeroDeltaCount) internal {
Copy link
Member

@ewilz ewilz Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for readability/less footguns (there's certain parameter combinations here that are unacceptable), wonder if we can break this into 3 functions:
lockData.incrementNonzeroDeltaCount()
lockData.decrementNonzeroDeltaCount()
lockData.delete()

probably at the expense of some bytecode savings, but I think it would make a lot of the code nicer to look at, update(length, count) feels vague and bug prone

Copy link
Member Author

@snreynolds snreynolds Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i added these functions but they all use similar code paths.. which I separated into toLockData and _update which does the final write

uint256 lockDataSlot = LOCK_DATA;
assembly {
lockData :=
or(
shl(128, _length),
and(0x00000000000000000000000000000000ffffffffffffffffffffffffffffffff, _nonzeroDeltaCount)
)

sstore(lockDataSlot, lockData)
}
}

function getLock(uint256 i) internal view returns (address locker) {
unchecked {
uint256 position = OFFSET + i; // not in assembly because OFFSET is in the library scope
uint256 position = LOCKERS + i; // not in assembly because LOCKERS is in the library scope
/// @solidity memory-safe-assembly
assembly {
locker := sload(position)
}
}
}

function getActiveLock(IPoolManager.LockData storage self) internal view returns (address locker) {
return getLock(self.length - 1);
function getActiveLock(LockData lockData) internal view returns (address) {
uint128 _length;
unchecked {
_length = lockData.length() - 1;
}
return getLock(_length);
}

function getLockData() internal view returns (LockData lockData) {
uint256 lockDataSlot = LOCK_DATA;
assembly {
lockData := sload(lockDataSlot)
}
}
}
12 changes: 12 additions & 0 deletions contracts/types/LockData.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.20;

import {LockDataLibrary} from "../libraries/LockDataLibrary.sol";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keeping this in a separate file is breaking convention with types Currency + BalanceDelta. Wonder if it's worth migrating the library here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved it to types/LockData.sol to match


/// @notice The LockData holds global information for the lock data structure: the length and nonzeroDeltaCount.
/// @dev The left most 128 bits is the length, or total number of active lockers.
/// @dev The right most 128 bits is the nonzeroDeltaCount, or total number of nonzero deltas over all active + completed lockers.
/// @dev Located in transient storage.
type LockData is uint256;

using LockDataLibrary for LockData global;
Loading