Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions src/contracts/GPv2Wrapper.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// SPDX-License-Identifier: GPL-3.0-or-later
// 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/>.
Comment on lines +2 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pragma solidity >=0.7.6 <0.9.0;
pragma abicoder v2;

import "./GPv2Settlement.sol";
import "./interfaces/IERC20.sol";

/**
* @dev Interface defining required methods for wrappers of the GPv2Settlement contract for CoW orders
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In short, we should adhiere to SOLID principles

* A wrapper should:
* * call the equivalent `settle` on the GPv2Settlement contract (0x9008D19f58AAbD9eD0D60971565AA8510560ab41)
* * verify that the caller is authorized via the GPv2Authentication contract.
* 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

GPv2Settlement public immutable UPSTREAM_SETTLEMENT;
GPv2Authentication public immutable AUTHENTICATOR;

constructor(address payable upstreamSettlement_) {
UPSTREAM_SETTLEMENT = GPv2Settlement(upstreamSettlement_);

// retrieve the authentication we are supposed to use from the settlement contract
AUTHENTICATOR = GPv2Settlement(upstreamSettlement_).authenticator();
}

/**
* @dev Called to initiate a wrapped call against the settlement function. See GPv2Settlement.settle() for more information.
*/
function wrappedSettle(
IERC20[] calldata tokens,
uint256[] calldata clearingPrices,
GPv2Trade.Data[] calldata trades,
GPv2Interaction.Data[][3] calldata interactions,
bytes calldata wrapperData
) external {
// Revert if not a valid solver
if (!AUTHENTICATOR.isSolver(msg.sender)) {
revert("GPv2Wrapper: not a solver");
}

_wrap(tokens, clearingPrices, trades, interactions, wrapperData);
}

/**
* @dev The logic for the wrapper. During this function, `_internalSettle` should be called
*/
function _wrap(
IERC20[] calldata tokens,
uint256[] calldata clearingPrices,
GPv2Trade.Data[] calldata trades,
GPv2Interaction.Data[][3] calldata interactions,
bytes calldata wrapperData
) internal virtual;

function _internalSettle(
IERC20[] calldata tokens,
uint256[] calldata clearingPrices,
GPv2Trade.Data[] calldata trades,
GPv2Interaction.Data[][3] calldata interactions
) internal {
UPSTREAM_SETTLEMENT.settle(tokens, clearingPrices, trades, interactions);

Check failure on line 75 in src/contracts/GPv2Wrapper.sol

View workflow job for this annotation

GitHub Actions / test (18.x, ubuntu-latest)

Replace tokens,·clearingPrices,·trades,·interactions with ⏎············tokens,⏎············clearingPrices,⏎············trades,⏎············interactions⏎········
}
}
20 changes: 20 additions & 0 deletions test/src/EmptyWrapper.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// solhint-disable-next-line compiler-version
pragma solidity >=0.7.6 <0.9.0;
pragma abicoder v2;
Comment on lines +2 to +4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


import "src/contracts/GPv2Wrapper.sol";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";


contract EmptyWrapper is GPv2Wrapper {
constructor(address payable upstreamSettlement_) GPv2Wrapper(upstreamSettlement_) {}

function _wrap(
IERC20[] calldata tokens,
uint256[] calldata clearingPrices,
GPv2Trade.Data[] calldata trades,
GPv2Interaction.Data[][3] calldata interactions,
bytes calldata /*wrappedData*/
) internal override {
_internalSettle(tokens, clearingPrices, trades, interactions);
}
}
Loading