feat: implement generic wrapper #247

kaze-cow · 2025-09-23T09:49:23Z

Description

Implementation of generalized wrappers on the smart contract side. For more info see the notion document
https://www.notion.so/cownation/Generalized-Wrapper-2798da5f04ca8095a2d4c56b9d17134e

Test Plan

Once process is confirmed, the authentication flow will be verified to reach 100% coverage.

Related Issues

backend impl feat: generalized wrappers services#3700
euler (will be using this in short order) feat: working security euler-integration-contracts#2

github-actions · 2025-09-23T09:49:35Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

kaze-cow · 2025-09-26T13:55:04Z

I have read the CLA Document and I hereby sign the CLA

src/contracts/GPv2Wrapper.sol

anxolin · 2025-09-29T21:07:50Z

src/contracts/GPv2Wrapper.sol

+import "./interfaces/IERC20.sol";
+
+/**
+ * @dev Interface defining required methods for wrappers of the GPv2Settlement contract for CoW orders


Is it an interface or an abstract contract to provide a easy way to implement a wrapper?

I would htink we need an interface and this contract should implement it

the abstract contract makes it easier for us to ensure the critical functional requirements of every wrapper are met (ex. verifying the caller is a solver, pulling in the immutable wrapper contract). The interface basically only consists of the one wrappedSettle call, so I went ahead and created the interface as well so that in case we find out later that there is a contract that should be circumventing this functionality.

I expect the "abstract contract" just a convenience with a very specific implementation.

the abstract contract makes it easier for us to ensure the critical functional requirements of every wrapper are met (ex. verifying the caller is a solver, pulling in the immutable wrapper contract)

imo, verifying the caller is always a solver is the job of the whitelisting process, where we would check the implementation of what we are about to approve, and would try to see if its safe.

Imagine, I want the wrapper to work in a 2 step process. A solver pre-approves a solution, and that's where we check is a solver, and then the wrapSettle only verifies is pre-approved.

There's many potential implementations where we ensure security without comiting to any specific implementation.

In short, we should adhiere to SOLID principles

anxolin · 2025-09-29T21:09:37Z

src/contracts/GPv2Wrapper.sol

+ * A wrapper may also execute, or otherwise put the blockchain in a state that needs to be established prior to settlement.
+ * Additionally, it needs to be approved by the GPv2Authentication contract
+ */
+abstract contract GPv2Wrapper {


Similar comment as in the Euler PR, we are loosing composability of wrappers by not implementing an interface with the settle method.

This wrapper can contain only the settlement. Wouldn't it be more flexible to let the thing have a settlement or wrapper?

Ways to do this, could be using https://eips.ethereum.org/EIPS/eip-165 to chek if wrappedSettle is implemented?

Brainstorming on some other possibility, not sure if this is good, but if we make the wrapper have exactly the same signature as the settlement, then its a perfect proxy pattern.

But how can we have the same signature?
It would just delegate all methods except settle.

But the same settle won't work, we need some additional parameter for the wrapperData!
One possible solution, you tell me if this is crazy or is really feasable. I know in ethereum you can call a method and pass additional parameters that in principle are ignored. If we can read them, we can actually include this additional data at the end.

Wrappers capture this data

Settleemnt will ignore it (and also, no-one will call it with the additional data anyways)

so yes, we do not have composability with settle anymore. I thought long and hard about this, and ultimately determined that attempting to maintain composability actually has some severe drawbacks which run counter to the goals of what we are trying to build:

most wrappers require some additional data in order to function. Originally the idea was to attach this data before calling the wrapper in a separate call, but this has major issues:

a significant amount of additional gas is required to store the calldata in transient storage just to access it later in the settle function.

as martin identified, the current functionality of pre- and post- hooks does not allow for them to run prior to the actual settle call, so they do not satisfy the need without any significant code overhaul

The only places that will require the use of wrappers in the near future are effectively the driver, and any corresponding solvers that will be supporting it. I have already gone through the process of adding the new wrapper code to the driver, and based on my experience, implementing the new interface is not complicated and fairly. On the contrary, implementing the logic to be able to have multicalls with IS complicated and would be a major ask.

as far as "flexibility" goes, I haven't actually been able to identify the practical usecase of such flexibility. Almost anything that uses our settlement contract hardcodes the address anyway as well as the ABI, so considering changing the address already introduces significant changes, its low value to keep.

For all these reasons, I determined that changing the interface is justified and actually puts us on a better overall path.

Regarding your idea, its actually tottally possible to do what you are suggesting, including the extra data off the end of the function signature. The problem with this is its kind of complicated and makes it harder to debug or understand what is going on, and as mentioned before, the usecase of maintaining the interface and preserving the flexibility seems low.

so wrappedSettle could implement EIP165. I didnt think this would be super useful because in all usecases I have been able to envision so far, the contract function will always be called from off-chain and generated by UIs and similar. The list of available wrappers is also kind of well known since every wrapper has to be individually approved in the GPv2Authentication contract by our team. With all this in mind, I figured simple is best.

I would think implementing EIP165 could allow to add some sort of composability even if we define a new wrappedSettle.

You can have a wrapper whose logic is:

Extract all data, takes the bytes that are for the wrapper. Save the reminder bytes

If the settlement address implement wrappedSettle call that passing the reminder bytes, if not just call settle

Alternatively, we could make the wrapper that by convention assumes that having non-zero bytes in the reminder means it needs to call wrappedSettle otherwise, it calls settle.

as far as "flexibility" goes, I haven't actually been able to identify the practical usecase of such flexibility. Almost anything that uses our settlement contract hardcodes the address anyway as well as the ABI, so considering changing the address already introduces significant changes, its low value to keep.

One example is. You want to mix a huge liquidation with a huge limit order that requites a flash-loan of a user into a beautiful CoW.
Imagine the liquidation requires EnforceableHooksWrapper and the limit order requires FlashloanWrapper.

You could: EnforceableHooksWrapper.wrappedSettle() --> FlashloanWrapper.wrappedSettle() --> settlement.settle() . This would allow you to match those huge trades without using any AMM.

Would sth like this make sense?

src/contracts/GPv2Wrapper.sol

socket-security · 2025-09-30T04:34:21Z

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

fedgiac

I think this code should not be part of this repo as it isn't part of the core protocol, it should be part of a dedicated repo for generic wrappers (or, right now, on cowprotocol/euler-integration-contracts).

I'm still adding my review here, but it's better to move this discussion to the appropriate place sooner than later.

In the context of that repo, the code overall makes sense to me.

fedgiac · 2025-10-01T09:47:40Z

src/contracts/GPv2Wrapper.sol

+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.


Nit: the license preamble is unneeded, contracts in this repo usually don't specify it.

Suggested change

// This program is free software: you can redistribute it and/or modify

// it under the terms of the GNU General Public License as published by

// the Free Software Foundation, either version 3 of the License, or

// (at your option) any later version.

// This program is distributed in the hope that it will be useful,

// but WITHOUT ANY WARRANTY; without even the implied warranty of

// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License

// along with this program. If not, see <http://www.gnu.org/licenses/>.

// SPDX-License-Identifier: GPL-3.0-or-later

fedgiac · 2025-10-01T09:51:00Z

src/contracts/GPv2Wrapper.sol

+/**
+ * @title A minimalist base that can be extended to safely implement a wrapper. It ensures the most important basic functions of a wrapper are fulfilled
+ */
+abstract contract GPv2Wrapper is IGPv2Wrapper {


The name GPv2 is historical baggage, this shouldn't appear in new projects and if we could we'd drop it from the core contracts as well. Maybe we could use something like:

Suggested change

abstract contract GPv2Wrapper is IGPv2Wrapper {

abstract contract CowSettlementWrapper is ICowSettlementWrapper {

(This comment applies to every instance of GPv2 in this code.)

fedgiac · 2025-10-01T09:59:17Z

src/contracts/interfaces/IGPv2Settlement.sol

+
+/// @title Gnosis Protocol v2 Settlement Interface
+/// @author Gnosis Developers
+interface IGPv2Settlement {


If this code isn't move to another repo, I'd suggest using the concrete instance GPv2Settlement and drop the interface code from here. Otherwise, we could make this interface much more minimal with only the two functions that this contract needs (settle and authenticator) to reduce the amount of code.

fedgiac · 2025-10-01T10:00:22Z

src/contracts/interfaces/IGPv2Wrapper.sol

+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.


Nit: as before, not necessary.

Suggested change

// This program is free software: you can redistribute it and/or modify

// it under the terms of the GNU General Public License as published by

// the Free Software Foundation, either version 3 of the License, or

// (at your option) any later version.

// This program is distributed in the hope that it will be useful,

// but WITHOUT ANY WARRANTY; without even the implied warranty of

// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License

// along with this program. If not, see <http://www.gnu.org/licenses/>.

fedgiac · 2025-10-01T10:05:15Z

src/contracts/interfaces/IGPv2Wrapper.sol

+
+/**
+ * @dev Interface for wrappers of the GPv2Settlement contract for CoW orders. It serves as a way for the functionality
+ * of the settlement contract to be expanded without needing to affect the underlying security.


Well, it does affect the underlying security of the system. 😛

Suggested change

* of the settlement contract to be expanded without needing to affect the underlying security.

* of the settlement contract to be expanded while still settling through the usual settlement contract.

fedgiac · 2025-10-01T10:08:55Z

src/contracts/interfaces/IGPv2Wrapper.sol

+ * of the settlement contract to be expanded without needing to affect the underlying security.
+ * A wrapper should:
+ * * call the equivalent `settle` on the GPv2Settlement contract (0x9008D19f58AAbD9eD0D60971565AA8510560ab41)
+ * * verify that the caller is authorized via the GPv2Authentication contract registered on the settlement contract.


Nit, caller authentication may differ in specific implementations.

Suggested change

* * verify that the caller is authorized via the GPv2Authentication contract registered on the settlement contract.

* * verify that the caller is authorized (for example, via the GPv2Authentication contract registered on the settlement contract).

fedgiac · 2025-10-01T10:11:16Z

src/contracts/interfaces/IGPv2Wrapper.sol

+ */
+interface IGPv2Wrapper {
+    function UPSTREAM_SETTLEMENT() external view returns (IGPv2Settlement);
+    function AUTHENTICATOR() external view returns (GPv2Authentication);


The authentication mechanism doesn't have to be the settlement. This means that this call may be meaningless. Also, the authenticator is always available as UPSTREAM_SETTLEMENT().authenticator().

Suggested change

function AUTHENTICATOR() external view returns (GPv2Authentication);

fedgiac · 2025-10-01T10:15:57Z

src/contracts/interfaces/IGPv2Wrapper.sol

+        IERC20[] calldata tokens,
+        uint256[] calldata clearingPrices,
+        GPv2Trade.Data[] calldata trades,
+        GPv2Interaction.Data[][3] calldata interactions,


Would you be able to test what's the gas savings when using a pre-encoded settlement:

function wrappedSettle( bytes memory settlementCallData, bytes calldata wrapperData )

I'd like to know how much of an overhead it is to encode the call parameters in the wrapper rather than providing the call data ready to be CALLDATACOPYed, ideally also considering the case with multiple tokens, trades, and interactions.

fedgiac · 2025-10-01T10:18:57Z

test/src/EmptyWrapper.sol

+// solhint-disable-next-line compiler-version
+pragma solidity >=0.7.6 <0.9.0;
+pragma abicoder v2;


You can use Solidity v0.8 for the tests to get rid of the solhint disable, we already do that foll all Foundry tests.

fedgiac · 2025-10-01T10:20:07Z

test/src/EmptyWrapper.sol

+pragma solidity >=0.7.6 <0.9.0;
+pragma abicoder v2;
+
+import "src/contracts/GPv2Wrapper.sol";


Nit: explicit imports are nicer for debugging since it lets you understand more easily where some component comes from at a glance.

Suggested change

import "src/contracts/GPv2Wrapper.sol";

import {GPv2Wrapper} from "src/contracts/GPv2Wrapper.sol";

anxolin · 2025-10-01T10:25:16Z

src/contracts/interfaces/IGPv2Wrapper.sol

+ * Prior to usage, the wrapper itself needs to be approved by the GPv2Authentication contract.
+ */
+interface IGPv2Wrapper {
+    function UPSTREAM_SETTLEMENT() external view returns (IGPv2Settlement);


Why do we want this in the interface? I would think there's implementation that not necessarilly have this

The wrapper contract doesn't have a way to specify which settlement contract to call, so to me it makes sense to have a general way to expose this information somehow. (Not important though, it's not really necessary.)

anxolin · 2025-10-01T10:28:05Z

src/contracts/GPv2Wrapper.sol

+/**
+ * @title A minimalist base that can be extended to safely implement a wrapper. It ensures the most important basic functions of a wrapper are fulfilled
+ */
+abstract contract GPv2Wrapper is IGPv2Wrapper {


I think its better now that implements the interface.

I feel a bit unsure lal this code should live in the contracts project. Maybe deserves its own project (technically, its an extension to the protocol so no need to go in the core contracts)

kaze-cow · 2025-10-02T11:30:27Z

thanks everyone for reviews

this will be closed in favor of putting hte code initially in the euler repo and later it can be copied where it is needed. I will incorporate all feedbacks there and link here.

anxolin · 2025-10-06T19:03:31Z

thanks everyone for reviews

this will be closed in favor of putting hte code initially in the euler repo and later it can be copied where it is needed. I will incorporate all feedbacks there and link here.

👍

feat: implement generic wrapper

8f1648d

kaze-cow self-assigned this Sep 23, 2025

update wrapper in accordance with backend changes suggestions

1e8127f

kaze-cow marked this pull request as ready for review September 26, 2025 13:54

kaze-cow requested a review from a team as a code owner September 26, 2025 13:54

carlos-cow reviewed Sep 29, 2025

View reviewed changes

src/contracts/GPv2Wrapper.sol Show resolved Hide resolved

anxolin reviewed Sep 29, 2025

View reviewed changes

update the wrapper to have a base interface that it inherits from

9bfad39

github-actions bot added a commit that referenced this pull request Sep 30, 2025

@kaze-cow has signed the CLA in #247

e9221eb

kaze-cow added 2 commits September 30, 2025 13:29

fix override properties

14cc843

lock foundry version

9f119d6

Merge branch 'main' into feat/wrapper

277be44

kaze-cow requested review from a team and anxolin October 1, 2025 08:14

fedgiac reviewed Oct 1, 2025

View reviewed changes

anxolin reviewed Oct 1, 2025

View reviewed changes

kaze-cow closed this Oct 2, 2025

github-actions bot locked and limited conversation to collaborators Oct 2, 2025

	abstract contract GPv2Wrapper is IGPv2Wrapper {
	abstract contract CowSettlementWrapper is ICowSettlementWrapper {

	* of the settlement contract to be expanded without needing to affect the underlying security.
	* of the settlement contract to be expanded while still settling through the usual settlement contract.

	* * verify that the caller is authorized via the GPv2Authentication contract registered on the settlement contract.
	* * verify that the caller is authorized (for example, via the GPv2Authentication contract registered on the settlement contract).

	import "src/contracts/GPv2Wrapper.sol";
	import {GPv2Wrapper} from "src/contracts/GPv2Wrapper.sol";

feat: implement generic wrapper #247

feat: implement generic wrapper #247

Uh oh!

Conversation

kaze-cow commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test Plan

Related Issues

Uh oh!

github-actions bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaze-cow commented Sep 26, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

socket-security bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fedgiac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedgiac Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaze-cow commented Oct 2, 2025

Uh oh!

anxolin commented Oct 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kaze-cow commented Sep 23, 2025 •

edited

Loading

github-actions bot commented Sep 23, 2025 •

edited

Loading

socket-security bot commented Sep 30, 2025 •

edited

Loading

fedgiac Oct 1, 2025 •

edited

Loading