use custom storage for lockData

[snreynolds]

Related Issue

Which issue does this pull request resolve?
By changing the lockData structure to use custom storage, we are setting up the repository to leverage tstore/tload.

Description of changes

Changes the LockDataLibrary to account for the sentinel node in storage.

[ewilz]

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

[snreynolds]

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

[ewilz]

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

[snreynolds]

moved it to types/LockData.sol to match

[ewilz]

can rename PR to something like: use custom storage for LockData?

[ewilz]

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

[snreynolds]

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.

[ewilz]

Wondering if we can tighten the library up by changing update to more opinionated functions otherwise library integrators can just input incoherent data right into storage.

Porbably not good the library performs storage writes using any arbitrary uint256 even if it doesn't match what's currently in storage. For push and pop, can we just call like this:

DataLockLibrary.push()
DataLockLibrary.pop()

then inline LockData lockData = getLockData(); as the first line of those functions instead of getting it outside?

[snreynolds]

Wondering if we can tighten the library up by changing update to more opinionated functions otherwise library integrators can just input incoherent data right into storage.

Porbably not good the library performs storage writes using any arbitrary uint256 even if it doesn't match what's currently in storage. For push and pop, can we just call like this:

DataLockLibrary.push()
DataLockLibrary.pop()

then inline LockData lockData = getLockData(); as the first line of those functions instead of getting it outside?

The reason I didn't do that is because as its written right now it would be easy to replace the in-memory lockData variable with a transient storage variable lockData and then write the library over that. So I'm just prepping for a more minimal diff later on.

I can change the update function though so that we have more clear actions over the lockData var

[hensha256]

Left some thoughts about the setup of the library!

[hensha256]

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

[snreynolds]

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

[hensha256]

Also - given the design of this library is currently a bit weird because it is designed with the transient keyword in mind, I feel like it makes sense to leave comments that say what we want to change when transient arrives. Your current knowledge of this library is sharper than it will be when you come back to make transient changes. So if they all start with like
// TODO transient: xyz
then when we get the transient key word we can go through the whole codebase making sure we make all those changes and dont miss any/make the changes incorrectly

[snreynolds]

closing in favor of separating the libs diff shown here #388