Layr-Labs / eigenlayer-contracts

Other
627 stars 331 forks source link

Request: rename "latestServeUntilBlock" in Slasher to something else #149

Closed samlaf closed 4 months ago

samlaf commented 1 year ago

The word "latest" has two separate meanings:

The Slasher contract uses latest for both of those meanings in two different variables, which makes it very confusing:

I find changing latestServeUntilBlock to furthestServeUntilBlock or some other synonym would rid of this confusion and make the contract clearer.

ChaoticWalrus commented 1 year ago

The word "latest" has two separate meanings:

* of most recent date (referring to something in the **past**)

* far on in time; toward the end of a period (referring to something in the **future**)

The Slasher contract uses latest for both of those meanings in two different variables, which makes it very confusing:

Although one is in the past and one is in the future, IMO the usage here is consistent. In both cases it is being used to mean the greatest numerical value.

I'll point out as well at latestServeUntilBlock doesn't have to be in the future, and there are definitely cases in which it isn't in the future.

I'm not personally a fan of furthestServeUntilBlock / not seeing how it's better, but I'd be open to changing it.

The hard part is typically determining an actually good name; if you have other suggestions please add them

samlaf commented 1 year ago

Ah this actually clarifies things for me. Agreed latestServeUntilBlock can sometimes be in the past. In fact, it actually needs to be in the past in order to complete a withdrawal.

I see that it's more subtle than I was thinking. Kind of annoying that latest has somehow come to mean "most recent". I'm fine with closing this now, but do think that it remains very confusing for first time contract readers. Probably adding a comment that latestServeUntilBlock can either be in the past or future would help and be enough.

Will leave here antonyms of "earliest" in case these sound good to anyone else

image
ChaoticWalrus commented 1 year ago

I think 'max' is the best replacement here if you want to insist on one.

ChaoticWalrus commented 4 months ago

closing as outdated