-
Notifications
You must be signed in to change notification settings - Fork 2
Demo/wrapper #50
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
base: carlos/helper
Are you sure you want to change the base?
Demo/wrapper #50
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
anxolin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking on PR format as we are working on that. From the PR description, I totally didn't have the expectation to see 3.5K lines deleted.
Its fine as its a DEMO, but maybe you can explain (if its the case) that this PR removes all the previous implementation deleting most files, and to review we should focus on checking justsrc/vendored/CowWrapper.sol and test/e2e/Aave.t.sol.
As i said, is nitpicky, mentioned because if someone has 20min to dedicate to this, makes things easier if they are not put off initially by seeing 4K changes and they jump to the gist
| /// The implementation should consume its wrapper-specific data and return the remainder. | ||
| /// @param wrapperData The full wrapper data to parse | ||
| /// @return remainingWrapperData The portion of wrapper data not consumed by this wrapper | ||
| function parseWrapperData(bytes calldata wrapperData) external virtual view returns (bytes calldata remainingWrapperData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You expect this one also to throw, if so, should we specify? returning the remainingWrapperData is returned on successful parsing.
Also, maybe we should specify what is expected there. Its just about consuming the bytes, or actually making verifications that the consumed bytes are not only correct in size, but also make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so during validation we actually expect that remainingWrapperData should be 0x (empty data) if the user has supplied the wrapper data correctly and it parses. The idea is that this funciton can have a dual use when calling _wrap() as it would then return the next wrapper contract address
| /// - Eventually call settle() on the approved CowSettlement contract | ||
| /// - Implement _wrap() for custom logic | ||
| /// - Implement parseWrapperData() for validation of implementation-specific wrapperData | ||
| abstract contract CowWrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For PoC is good, but I still think we should have both the abstract contract and interface. We don't want to limit the way people make wrappers and we prefer working wirth interfaces to apply SOLID principles in our contract designs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the interface should be bundled on line 58 of this file and should be includable with import {ICowWrapper} from './CowWrapper.sol'. But it looks like I need to have the abstract contract actually extend the interface
|
@anxolin thank you for your early look review! regarding the test failures, my apologies, I forgot to push some files originally. If you pull and check the latest and run the test command in Please note that the |

Description
Experiment with using the
CowWrapperwith the Aave Flashloans--specifically the collateral swap usecase.Context
In order to validate the usefulness of Generalized Wrappers as it continues to develop, we wanted to spot check that it could have worked well as a replacement for the flashloan router.
Originally I tried replacing the borrower, and this actually worked fine, but it leaves some potential security gaps (ie, if the user has enough collateral to just do a swap without actually taking out the flash loan, the solver may have opporitunity to steal funds)
Secondly, I tried replacing
FlashLoanRouter. The challenge here ended up being that the flash loan router expects to be able to call the settlement contract directly after it finishes executing, and changing this behavior would have been quite difficult. Since the wrapper standard effectively requires a different signature now, it wouldn't have worked well to include the FlashloanRouter in the pattern. (Additionally, Aave will not be using the flashloanrouter so there is little benefit in trying to see its practicality in general...)Ultimately, I ended up making a flow similra to what ew have with Euler which is very rigid by design and focused on solving a specific simple user flow, the collateral swap.
Implementation steps:
AaveCollateralSwapWrapper.sol, an implementation of aCowWrapperwhich takes out a flash loan on aave and then deposits into the user's account (so that collateral can be removed from their account without needing to repay)Aave.t.solto act as an E2E test to vlaidate that the concept works as expected.Of particular note:
Permit2pattern considered and ultimately used ot get this to work. The benefit is, with a single signature, all the necessary validations for a secure order can be done and the wrapper effectively becomes the validation, while still preserving COW and not adding any additional work for solversOut of Scope
This is just for POC/feasibility investigatoin. Should not be deeply considered.
The only files that are important to look at are
src/AaveCollateralSwapWrapper.sol, and its corresponding E2E test,test/AaveCollateralSwapWrapper.t.sol.The
CowWrapper.sol(and relatedCowWrapperHelpers.sol) is copied over from theeuler-integration-contracts, and should be reviewed there if you have any comments https://github.com/cowprotocol/euler-integration-contracts/pull/6/files#diff-68933a405a1519b868a8dbdcd57df9bf04b41dab0e6c09f6640bfe5532ec9213There are a lot of files deleted. I originally intended to use this repo as a starting point, but ended up tearing most of it apart and stripping down to bare bones. my apologies.
How to Test
Only the E2E test is guarenteed to work, as that was just use to establish the pattern and get a code baseline. To run, checkout the repo and run: