-
Notifications
You must be signed in to change notification settings - Fork 152
feat: generalized wrappers #3700
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
Conversation
this introduces the ability for a `wrapper` contract to be included in the appdata and solutions. A `wrapper` is a contract which is called with the same parameters as settlement contract `settle`, but executes a series of operations either/both before or after executing the actual GPv2Settlement contract's `settle` function. This simplifies integration. Only approved `wrapper` contracts will be allowed to call the GPv2Settlement contract (via the current GPv2Allow), and it is expected that wrapper contracts will do the necessary validation of solvers on behalf of the settlement contract. Its expected that generalized wrappers can simplify the implementation of many initiatives, including: * flashloans integrations (ex. aave, euler) * enforcable pre- and post-hooks (users can ensure that post actions must at least be attempted, such as deposit to a vault) * ex: with our current CowShed implementation, if cross chain send were to fail due to ex. insufficient fee, funds can be sent back to the user rather than needing to be recovered from the CowShed contract * TWAP * currently we are implementing this for EOAs, but its very difficult to ensure funds are only pulled from the users wallet when needed because there is no auth. Since the CowShed could effectively become a wrapper contract, it can pull funds from user and immediately call the necessary `settle` function afterwards to ensure funds are traded. Implementation notes: * `wrapper` is added to the `appData` so that the frontend can "hint" the necessary flashloan or other required functionality if needed (similar to current `flashloans` functionality) * `wrapper` is added to solution so its easy for the driver to encode into a final solution, * additionally a solver can add their own wrapper if they determine its necessary/useful to get the best result from a trade (some already technically do this) Potentially further changes required/limitations: * when quoting, how can the use of a wrapper be effectively included in the calculation? * what happens if we more than one swap in a batch that requires a wrapper? * since its not possible to supply additional parameters to `settle` on the wrapper, in some cases it may be required to use a `preHook` to apply settings on a `transient` variable as part of the multicall for execution
unfortunately doesn't quite work because of flaws with cargo and the complexity of managing the dependencies. rust-lang/cargo#2644
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
|
The contracts team is taking a bit of extra time to review the design of the wrappers. Some changes will be coming next week but nothing too serious. Afterwards. We'll be requesting reviews and feedback from the back end team 👍 |
fleupold
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.
not a real review, just a comment on the structure of the new field
10f7352 to
0c5cbb6
Compare
Co-authored-by: Martin Magnus <[email protected]>
Co-authored-by: Martin Magnus <[email protected]>
Co-authored-by: Martin Magnus <[email protected]>
Co-authored-by: Martin Magnus <[email protected]>
Co-authored-by: Martin Magnus <[email protected]>
Co-authored-by: Martin Magnus <[email protected]>
Co-authored-by: Martin Magnus <[email protected]>
|
|
||
| pub type WrapperCalls = HashMap<order::Uid, Vec<(H160, Vec<u8>, bool)>>; | ||
|
|
||
| #[allow(clippy::too_many_arguments)] |
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.
@MartinquaXD I aws just peeking through the changes one more time and I just noticed this. For now is it ok to be overriding clippy here?
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.
Yeah, that's fine. Although for new overrides let's use #[expect(clippy::too_many_arguments)] (see here for more context).
squadgazzz
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.
LGTM, only nits
for right now its just a fork test. later will probably want to have something more sophisticated
MartinquaXD
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.
Thanks for adding the test. I think after addressing the last nits on the test this is ready to be merged. 🙌
crates/e2e/tests/e2e/wrapper.rs
Outdated
| web3.alloy | ||
| .anvil_send_impersonated_transaction_with_config( |
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 is no need to impersonate the trader for this call since we have the PK available. For example the approve call below works just fine without impersonation.
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.
fixed in 1f356b0
* dont need to impresonate trader to deposit ETH * auction seems to be having issues pulling the trace sometimes * actually check to make sure some expected wrapper calldata is coming through
Description
this introduces the ability for a
wrappercontract to be included in the appdata and solutions. Awrapperis a contract which is called with the same parameters as settlement contractsettle, but executes a series of operations either/both before or after executing the actual GPv2Settlement contract'ssettlefunction. This simplifies integration. see the full info here https://www.notion.so/cownation/Generalized-Wrapper-2798da5f04ca8095a2d4c56b9d17134eOnly approved
wrappercontracts will be allowed to call theGPv2Settlementcontract (via the current GPv2AllowlistAuthenticator), and it is expected that wrapper contracts will do the necessary validation of solvers on behalf of the settlement contract.Its expected that generalized wrappers can simplify the implementation of many initiatives, including:
settlefunction afterwards to ensure funds are traded.Implementation Notes
wrappersis added to theappDataso that the frontend can "hint" the necessary flashloan or other required functionality if needed (similar to currentflashloansfunctionality). More than one wrapper can be specified in a single order. A wrapper is combination ofaddress, the address of the wrapper contract to execute, anddata, which is any additional data required for the wrapper to executewrappersis added to solution so its easy for the driver to encode into a final solution. It is an array in order to accomodate different orders that require different wrappers.Potentially further changes required/limitations:
How to Test the Wrapper
I have deployed a testing
EmptyWrapperto0x54112E2F481AC239661914691082039d7B05A264. By using this contract as a simple wrapper, created a script to serve as a minimal E2E POC:This PR can be verified to be working E2E by taking the following steps:
docker compose -f playground/docker-compose.fork.yml up --build./playground/test_wrapper.shcast:cast tx <my transaction hash from logs>cast receipt <my transaction hash from logs>wrapperandwrapperDataby opening up https://localhost:8001 (the CoW Explorer) and putting in the printed order ID at the end of script execution.Related PRs and Documentation