Reduce code duplication #17
Conversation
| bytes calldata settleData, | ||
| bytes calldata wrapperData, | ||
| bytes calldata remainingWrapperData, | ||
| ParamsLocation paramMemoryLocation, |
There was a problem hiding this comment.
i actually think creating type here makes it more confusing to tell that this is just a regular bytes32 under the hood. I'm guessing the reason you wanted this is to prevent accidentally mixing the params location with another bytes32 elsewhere?
There was a problem hiding this comment.
Yeah, that's the only reason. This is a function that already has a lot of parameters and so I just wanted to make sure the type system prevents issues here.
| } | ||
| } | ||
|
|
||
| if (timing == SettlementTiming.After) { |
There was a problem hiding this comment.
for the current set of wrapeprs it is indeed the case that settlement either entirely happens before or entirely happens after the specified batch operations. but in previous iterations we had it both before and after (for example, when we were clearing the no-longer in use collateral types; this was determined as not necessary by the euler team, however).
so it may be worth considering having both BatchItem beforeItems and BatchItem afterItems instead of doing an enum
There was a problem hiding this comment.
Yeah, it's nicer to have before/after. I didn't want to change too much, but it would have been a cleaner implementation to do so.
|
i merged this because its a significant improvementi n the direction of where we want to take the codebase, and much faster to just merge this and I can clean up the merge stack rather than to do it manually |
Description
A proposal to simplify the current contracts by moving repeating code across different wrappers to the same contract.
This is achieved by creating a base abstract contract
CowEvcBaseWrapperwhich is supposed to be inherited by all Euler wrappers.It aggregates code related to three main features:
The purpose is making it easier to spot what is different and what is the same between wrappers when reading the code.
Ideally, the remaining single-instance wrappers should have little remaining occurrences of remaining code. (One exception is
_evcInternalSettle. I'm sure it can be simplified but I don't understand well enough the balance checks to make sure things are working after my changes, so I'm skipping it.)The result is kind of nice imho: it makes clear(er) that what matters in the various instances is not how the batch is executed, but rather what's the content of the batches (i.e., the implementation of
_encodeSignedBatchItems). It also makes it more visible that the settlement is executed before other operations when closing a position and after in the other two wrappers. I considered using a "itemsBefore/itemsAfter" model instead but I decided we can just switch to it if we ever find the need and just went for a flag.The bad part of this PR is that Solidity doesn't have generics. This means that all external accessors like
getSignedCalldatastill need to be repeated on each individual wrapper instance, causing annoying code repetition. Because of that, I also had to play with raw memory pointers, which may be dangerous to do if we abuse assembly to generate structures in memory.Note that I tried to make the minimal changes needed to the existing code base to show the difference, so some design choices I made may be questionable. I don't expect this PR to be merged, if anything because it would make a mess in the current PR stack.
Out of Scope
There should be no changes in the implementation, only in the code structure.
Testing Instructions
To be fair, I didn't go in depth when building the code, I'm trusting CI to tell me if something is off.
You may also want to compare the gas cost between the two runs. Here is the diff between PR base e8e9e6c and 0c98278.
forge snapshot --diff
The gas impact is visible (especially for opening a position, since it adds a function parameter to an external function) but overall limited (max ~3k gas), which I consider a reasonable tradeoff.