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 all 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
95828
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
152890
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
58722
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
306997
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
274947
2 changes: 1 addition & 1 deletion .forge-snapshots/mint.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
293760
293605
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
49720
49856
Original file line number Diff line number Diff line change
@@ -1 +1 @@
124982
124514
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
109224
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
91638
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
49827
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
49856
25 changes: 14 additions & 11 deletions contracts/PoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {Position} from "./libraries/Position.sol";
import {FeeLibrary} from "./libraries/FeeLibrary.sol";
import {Currency, CurrencyLibrary} from "./types/Currency.sol";
import {PoolKey} from "./types/PoolKey.sol";
import {LockDataLibrary} from "./libraries/LockDataLibrary.sol";
import {NoDelegateCall} from "./NoDelegateCall.sol";
import {Owned} from "./Owned.sol";
import {IHooks} from "./interfaces/IHooks.sol";
Expand All @@ -21,6 +20,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, LockDataLibrary} from "./types/LockData.sol";

/// @notice Holds the state for all pools
contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC1155, IERC1155Receiver {
Expand All @@ -30,7 +30,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 +38,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 +134,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();
LockDataLibrary.clear();
} else {
lockData.pop();
}
Expand All @@ -154,15 +152,16 @@ 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;

unchecked {
if (next == 0) {
lockData.nonzeroDeltaCount--;
lockData.decrementNonzeroDeltaCount();
} else if (current == 0) {
lockData.nonzeroDeltaCount++;
lockData.incrementNonzeroDeltaCount();
}
}

Expand All @@ -176,7 +175,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 +410,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
50 changes: 0 additions & 50 deletions contracts/libraries/LockDataLibrary.sol

This file was deleted.

136 changes: 136 additions & 0 deletions contracts/types/LockData.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.20;

/// @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.
/// TODO: With transient storage experiment writing length and nonzeroDeltaCount to two separate slots and compare gas.
type LockData is uint256;

using LockDataLibrary for LockData global;

function toLockData(uint128 _length, uint128 _nonzeroDeltaCount) pure returns (LockData lockData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given its going to be transient storage, should we consider just putting these in different slots? Every function that writes to this slot, only updates 1 of the 2 values - they never both get updated. It feels like it would be easier to just separate them?
push - only touches length
pull - only touches length
incrementNonzeroDeltaCount - only touches delta count
decrementNonzeroDeltaCount - only touches delta count

Imo it might make more sense to have

NON_ZERO_DELTA_COUNT_SLOT = x
LOCKERS_SLOT = y

where

sload(LOCKERS_SLOT) => length
sload(LOCKERS_SLOT+1) => first locker

this is more like how actual arrays are encoded in storage

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting, yeah we could do that and compare gas. tload/tstore still 100 gas so its not crazy to pack though

/// @solidity memory-safe-assembly
assembly {
lockData :=
or(
shl(128, _length),
and(0x00000000000000000000000000000000ffffffffffffffffffffffffffffffff, _nonzeroDeltaCount)
)
}
}

/// @dev This library manages a custom storage implementation for a queue
/// 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 LOCKERS, and each value is a locker address.
library LockDataLibrary {
snreynolds marked this conversation as resolved.
Show resolved Hide resolved
using LockDataLibrary for LockData;

uint256 private constant LOCK_DATA_SLOT = uint256(keccak256("LockData"));
uint256 private constant LOCKERS_SLOT = uint256(keccak256("Lockers"));

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
}
}

/// @dev Pushes a locker onto the end of the queue at the LOCKERS_SLOT + 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 nextLockerSlot = LOCKERS_SLOT + _length; // not in assembly because assembly cannot access constants

/// @solidity memory-safe-assembly
assembly {
// in the next storage slot, write the locker
sstore(nextLockerSlot, locker)
}
LockData newLockData = toLockData(_length + 1, lockData.nonzeroDeltaCount());
newLockData.update();
}
}

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

/// @dev Decreases the nonzero delta count by 1.
function decrementNonzeroDeltaCount(LockData lockData) internal {
uint256 newLockData;
unchecked {
newLockData = LockData.unwrap(lockData) - 1;
}
LockData.wrap(newLockData).update();
}

/// @dev Increases the nonzero delta count by 1.
function incrementNonzeroDeltaCount(LockData lockData) internal {
uint256 newLockData;
unchecked {
newLockData = LockData.unwrap(lockData) + 1;
}
LockData.wrap(newLockData).update();
}

/// @dev Clears the length and nonzero delta count from the lockData.
function clear() internal {
LockData lockData = LockData.wrap(uint256(0));
lockData.update();
}

/// @dev Writes the new length and nonzeroDeltaCount to storage.
/// TODO: Use transient storage.
function update(LockData lockData) internal {
uint256 lockDataSlot = LOCK_DATA_SLOT;
assembly {
sstore(lockDataSlot, lockData)
}
}

/// @dev Returns the locker at the ith index.
/// TODO: Use transient storage.
function getLock(uint256 i) internal view returns (address locker) {
unchecked {
uint256 position = LOCKERS_SLOT + i; // not in assembly because assembly can't read constants
/// @solidity memory-safe-assembly
assembly {
locker := sload(position)
}
}
}

/// @dev Returns the current locker.
function getActiveLock(LockData lockData) internal view returns (address) {
uint128 _length;
unchecked {
_length = lockData.length() - 1;
}
return getLock(_length);
}

/// @dev Returns the current length and nonzeroDeltaCount.
/// TODO: Use transient storage.
function getLockData() internal view returns (LockData lockData) {
uint256 lockDataSlot = LOCK_DATA_SLOT;
assembly {
lockData := sload(lockDataSlot)
}
}
}
Loading