makerdao / dss-exec-lib

DSS Executive Spellcrafting Library Contracts
GNU Affero General Public License v3.0
36 stars 21 forks source link

Refactor nextCastTime to DssExecLib #74

Closed brianmcmichael closed 3 years ago

brianmcmichael commented 3 years ago

There's a lot of logic that is going in to the DssExec every week for the officehours functionality.

This creates two pure functions with optimized calldata that can be called directly from DssAction.

This should reduce the bytecode size of a weekly deployment with minimal impact on cast cost.

gbalabasquer commented 3 years ago

Did we check how close to the bytecode max size we got in DssExecLib with this change?

hexonaut commented 3 years ago

I don't think we should be pulling these out into a separate library. This is going to require us to add a new library reference on every spell which takes up time and additional bytecode making reference to an additional library. I think as a practical matter I would prefer to keep everything in dss-exec-lib until we start bumping up against the bytecode limit.

gbalabasquer commented 3 years ago

I agree with Sam. I would remove nextCastTime from the execlib to move it to somewhere else which the spell itself has nothing to do. Like a support contract. I know that would change things for UI people, but shouldn't be a big deal. If that doesn't happen, then I prefer to keep one library until we reach the limit.

hexonaut commented 3 years ago

Yeah that's a good point. It isn't used by the spell directly. It should probably be just some utility contract that can be pulled in when needed.

brianmcmichael commented 3 years ago

I moved everything back to the library for now. I've always thought that the time stuff would be a good candidate for an external reference contract, but that's going to be a more involved deployment process. We've still got space here for now.