Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
70bb676
add wrapper
kaze-cow Oct 7, 2025
4a1683a
fix test failure
kaze-cow Oct 7, 2025
a7c03fe
change interface back to fresh call interface, clean up unneeded things
kaze-cow Oct 9, 2025
511f7aa
add documentation
kaze-cow Oct 9, 2025
e82bc8a
more cleanups
kaze-cow Oct 9, 2025
edce0df
Update src/vendor/CowWrapper.sol
kaze-cow Oct 11, 2025
594613e
fix wrapper read next settlement doesn't need assembly
kaze-cow Oct 13, 2025
d9812ed
Merge branch 'feat/wrapper-base' of https://github.com/cowprotocol/eu…
kaze-cow Oct 13, 2025
8803d2a
switch to requires instead of if->reverts as they are supported now
kaze-cow Oct 13, 2025
b164062
fix test failures and clean up docs
kaze-cow Oct 13, 2025
19b8010
remove wrong comment
kaze-cow Oct 13, 2025
1e37149
actually implement the ICowWrapper interface
kaze-cow Oct 17, 2025
e583a6e
Update CowWrapper to use static settlement contract
kaze-cow Oct 17, 2025
323e7ce
add required name field
kaze-cow Oct 17, 2025
640529a
Merge remote-tracking branch 'origin/master' into feat/wrapper-base
kaze-cow Oct 20, 2025
00ff9ba
add memory safe annotation
kaze-cow Oct 20, 2025
cdf82fd
add CLAUDE.md
kaze-cow Oct 20, 2025
e5faddf
forge fmt
kaze-cow Oct 20, 2025
961e71a
update CLAUDE.md to reduce unnecessary messaging about reentrancy
kaze-cow Oct 20, 2025
7192697
Merge remote-tracking branch 'origin/master' into feat/wrapper-base
kaze-cow Oct 22, 2025
7fea7ba
updates from wrapper security review
kaze-cow Oct 22, 2025
61051cd
chore: `forge fmt`
kaze-cow Oct 27, 2025
6b12785
foundry fixes
kaze-cow Oct 27, 2025
b1d1127
add pull request template
kaze-cow Oct 27, 2025
9739ab1
chore: apply formatting
kaze-cow Oct 27, 2025
bcda0c2
Merge branch 'chore/fmt-master' into feat/wrapper-base
kaze-cow Oct 27, 2025
8529191
fixes from feedback
kaze-cow Nov 4, 2025
41ddd46
clarify the thing that is being tested and use expectCall more
kaze-cow Nov 4, 2025
63cc658
fix fmt
kaze-cow Nov 4, 2025
a464c98
remove more dead code
kaze-cow Nov 4, 2025
ca2c6b4
Update src/vendor/CowWrapper.sol
kaze-cow Nov 4, 2025
549d740
Update src/vendor/CowWrapper.sol
kaze-cow Nov 4, 2025
3a1654f
Update src/vendor/CowWrapper.sol
kaze-cow Nov 4, 2025
972f958
more warnings fixes and make it compile with previous suggestion accepts
kaze-cow Nov 4, 2025
b4c9b97
update claude md to quiet the ai
kaze-cow Nov 4, 2025
fe6446a
try to quiet claude
kaze-cow Nov 4, 2025
242e7c5
Merge remote-tracking branch 'origin/master' into feat/wrapper-base
kaze-cow Nov 4, 2025
13e09aa
move the cow wrapper file out of vendor, and update the docs a bit more
kaze-cow Nov 4, 2025
7c6be09
update documentation on the wrapper interface
kaze-cow Nov 4, 2025
0313ff2
remove parseWrapperData reference in comment
kaze-cow Nov 4, 2025
d358b09
add remaining wrapper data start variable
kaze-cow Nov 4, 2025
694e7b0
Update src/CowWrapper.sol
kaze-cow Nov 4, 2025
bccb8db
rename `_internalSettle` to `_next`
kaze-cow Nov 4, 2025
c4f4aea
refactor tests
kaze-cow Nov 5, 2025
9bac22f
forge fmt
kaze-cow Nov 5, 2025
5e3e4ee
pull in upstream requirements
kaze-cow Nov 6, 2025
d828cdb
fix comments from federico
kaze-cow Nov 6, 2025
daf68fc
impl comment suggestion
kaze-cow Nov 6, 2025
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
161 changes: 161 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
# CLAUDE.md
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this file was added here in this commit. it is actually the case that most of these PRs add/make changes to CLAUDE.md, as new information relevant to the code becomes needed. if you have a concern, I can remove this.

its probably not important to look closely at this file.

Choose a reason for hiding this comment

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

kill this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok I just want to confirm, a particular reason why? its common practice to commit this file to repo as it is used by all team members and can improve the quality of the agent (see: https://www.anthropic.com/engineering/claude-code-best-practices ). Especially since in this repo there are AI assisting in the reviews its helpful.

Copy link

Choose a reason for hiding this comment

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

From my point of view is fine to add. The only thing, is that this doesn't belong to this PR, but not a big deal.

Choose a reason for hiding this comment

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

I would kill it because what happens if then we add another ai? would you keep track inside the repo of a chatgpt, grok, etc?
As far as I understand it helps the ai gather contexts faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's nice to have. If we have many tools we add all of them, why not? It makes the configuration easy to read if needed. I agree this file by itself would have been a great standalone PR, would also move this discussion away from the wrapper feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

But reading the file content: this is just a readme, right? There's nothing special to Claude, this information would be helpful to a fellow human as well. It's actually a really good readme at that.
Can we simply move 75% of this file to readme.md? I'd leave out very little, like "coding style" or "Testing architecture." I'd be fine with just renaming this file to readme.md if it makes it simpler to maintain the actual readme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it is very much like a readme. although as you point out its probably got superfluous information that humans probably wouldn't want to look too deeply into. if you prefer we can have this be the README and then have CLAUDE.md literally just be Read README.md or so

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok to leave it like this in this PR and the following ones, but eventually it would be nice to extract useful information from here and move it to the readme.


This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.

## Project Overview

This repository contains **Euler-CoW Protocol integration contracts** that enable leveraged position management (opening/closing) through CoW Protocol settlements combined with Ethereum Vault Connector (EVC) operations. The contracts act as "wrappers" that coordinate complex multi-step DeFi operations atomically.

### Core Architecture

**Wrapper Pattern**: The codebase uses a chaining wrapper pattern where solvers can execute wrapped settlements that perform custom logic before/during/after CoW Protocol settlements.

- `CowWrapper.sol`: Base abstract contract providing the wrapper framework
- Validates callers are authenticated solvers
- Implements `wrappedSettle()` entry point
- Provides `_internalSettle()` for continuing the settlement chain
- Wrappers can be chained: Wrapper1 → Wrapper2 → Settlement

- `CowWrapperHelpers.sol`: Helper utilities for wrapper data parsing and validation
- The CowWrapper is designed to support reentrancy. Additionally, the CowWrapper is designed with gas efficiency in mind, so we only check if the previous contract was part of the trusted wrapper chain. Furthermore, any wrappers that will ever be approved exist will use `CowWrapper.sol` as a base, so its not possible to inject a unauthorized wrapper into the chain without it getting ultimately rejected by the time the settlement contract is reached.

**Specialized Wrappers**: Two production wrappers implement specific EVC + CoW Protocol workflows:

1. **`CowEvcOpenPositionWrapper.sol`**: Opens leveraged positions
- Enables collateral vault
- Enables controller (borrow vault)
- Deposits collateral
- Borrows assets
- Executes CoW settlement to swap borrowed assets → collateral
- All operations are atomic within EVC batch

2. **`CowEvcClosePositionWrapper.sol`**: Closes leveraged positions
- Executes CoW settlement to swap collateral → repayment assets
- Repays debt to borrow vault
- Returns excess assets to user
- Disables collateral if full repayment
- All operations are atomic within EVC batch

**Authorization Mechanisms**: Both wrappers support two authorization modes:
- **EVC Permit**: One-time permit signature for specific operation
- **Pre-Approved Hashes** (`PreApprovedHashes.sol`): Users pre-approve operation hashes on-chain (useful for EIP-7702 wallet interactions)

### Key Dependencies

- **Euler Vault Kit** (`lib/euler-vault-kit`): ERC4626 vault implementation with borrowing
- **Ethereum Vault Connector (EVC)** (`lib/evc`): Batch transaction coordinator with account checking
- **CoW Protocol** (`lib/cow`): DEX aggregator settlement contracts and order libraries

## Development Commands

### Build
```bash
forge build
```

### Test
```bash
# Run all tests (requires FORK_RPC_URL environment variable)
forge test

# Run specific test file
forge test --match-path test/CowEvcOpenPositionWrapper.t.sol

# Run specific test function
forge test --match-test test_OpenPosition

# Run with verbose output
forge test -vvv
```

**Important**: Tests require mainnet fork. Set `FORK_RPC_URL` environment variable to a mainnet RPC endpoint.

### Format
```bash
forge fmt
```

### Gas Snapshots
```bash
forge snapshot
```

## Testing Architecture

**Base Test Contract**: `test/helpers/CowBaseTest.sol`
- Sets up mainnet fork at block 22546006
- Configures CoW Protocol settlement and authenticator
- Deploys test solver contract
- Sets up test vaults (eSUSDS, eWETH) and tokens
- Provides helper functions for creating settlement data structures

**Test Helpers**:
- `MilkSwap.sol`: Simple test DEX for simulating swaps in settlements
- `GPv2OrderHelper.sol`: Utilities for constructing CoW Protocol orders
- `SignerECDSA.sol`: ECDSA signature utilities for tests
- `EmptyWrapper.sol`: Minimal wrapper for testing wrapper chaining

## Important Implementation Details

### Wrapper Data Format
Wrapper data is passed as a calldata slice with format:
```
[wrapper-specific-params][signature][next-wrapper-address (20 bytes)][remaining-wrapper-data]
```

The `parseWrapperData()` function must consume its portion and return the remainder.

### EVC Integration
Both wrappers execute operations within EVC batches to ensure atomicity and proper account health checks. The flow is:
1. Wrapper validates authorization (permit or pre-approved hash)
2. Build EVC.BatchItem[] array with all operations
3. Call `EVC.batch()` - EVC ensures account is healthy at end

### Settlement Execution Context
- Wrappers use `evcInternalSettle()` as internal callback from EVC batch
- This function can only be called by EVC during batch execution
- Uses transient storage (`depth`, `settleCalls`) to prevent reentrancy

### Authentication
- Only authenticated CoW Protocol solvers can call `wrappedSettle()`
- Authentication checked via `AUTHENTICATOR.isSolver(msg.sender)`
- Wrappers themselves can be added to solver allowlist for testing

## Foundry Configuration

- Compiler optimization: enabled
- IR compilation: enabled (`via_ir = true`)
- Source directory: `src/`
- Test directory: `test/`
- Library dependencies managed via git submodules

## Coding Style

### Error Handling
**Always use `require()` with custom errors instead of `if () { revert }`**. This pattern is used consistently throughout the codebase:

```solidity
// ✅ Preferred
require(msg.sender == address(EVC), Unauthorized(msg.sender));
require(depth > 0 && settleCalls == 0, Unauthorized(address(0)));

// ❌ Avoid
if (msg.sender != address(EVC)) {
revert Unauthorized(msg.sender);
}
```

This approach is more concise and maintains consistency with the existing codebase style.

## Remappings

Key import remappings:
- `cow/` → CoW Protocol contracts (`lib/cow/src/contracts`)
- `evc/` → Ethereum Vault Connector (`lib/euler-vault-kit/lib/ethereum-vault-connector/src/`)
- `euler-vault-kit/` → Euler vault implementation
- `openzeppelin/` → OpenZeppelin contracts (via EVC dependency)


## When Giving PR feedback
* do not re-suggest or address feedback after it has already been given, either by you or other contributors who have commented.
* be careful not to use too many inline comments. If there are already inline comments on the same line that you want to comment on, or if the inline comment is about something that has already been suggested, don't comment.
197 changes: 197 additions & 0 deletions src/CowWrapper.sol
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing of this is vendored so it should be moved to src.
I think each small interface could have its own file for ease of copypaste (ideally with a link to some URL where I can compare it with the full interface).

Copy link
Collaborator Author

@kaze-cow kaze-cow Oct 11, 2025

Choose a reason for hiding this comment

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

i actually deliberately moved all the interfaces/dependencies for CowWrapper into the same file. The idea is that its a lot easier to just copy one self-contained file with no dependencies from project to project than a bunch of misc files/whole folder. I rezlide this as I was starting to implement the wrapper in the flash-loan-router repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the reason I put CowWrapper into vendor is because its not directly related to the purposes/goal of this repository, though I understand you see it as an in-house product so it shouldn't be called "vendor".

How about moving it (and any related sources) to a new subdirectory src/wrapper instead?

Copy link

Choose a reason for hiding this comment

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

So, the issue is that the wrapper doesn't belong to this repo (Euler). I think we decided to have it here just to make it easy for you to iterate on the wrapper with this use case in mind. But I would think

  • Either; Eventually will go to its own project (@cowprotocol/settlement-wrapper-contracts)
  • Or, my prefered cause of simplicity. We rename this project to (@cowprotocol/settlement-wrapper-contracts) and put the main contracts in src and all wrappers in src/wrappers

Copy link

Choose a reason for hiding this comment

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

I also think we should break a bit more this file into smaller ones with the interfaces and contracts separately

Copy link
Contributor

Choose a reason for hiding this comment

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

though I understand you see it as an in-house product so it shouldn't be called "vendor".

Yeah, vendor sounds like this is just copied, but part of this code is new for this repo.

How about moving it (and any related sources) to a new subdirectory src/wrapper instead?

It makes sense to separate the wrapper code.

Overall, I'm ok with keeping ICoWSettlement and ICowAuthentication in the same file (and in vendored as well) but I'd still say that ICowWrapper should have its own file (in src/wrapper).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the motivation for this was addresses in the inline comment, I think the idea is nice. I consider this comment resolved with no change needed.

  • @notice This file is completely self-contained (ie no dependencies) and can be portably copied to whatever projects it is needed.

Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity >=0.8.0 <0.9.0;

/**
* CoW Wrapper all-in-one integration file
* CoW Protocol Developers
* This file is completely self-contained (ie no dependencies) and can be portably copied to whatever projects it is needed.
* It contains:
* * CowWrapper -- an abstract base contract which should be inherited by all wrappers
* * ICowWrapper -- the required interface for all wrappers
* * ICowSettlement -- A minimized interface and base structures for CoW Protocol settlement contract. From https://github.com/cowprotocol/contracts/blob/main/src/contracts/GPv2Settlement.sol
* * ICowAuthentication -- The authentication interface used by ICowSettlement. From https://github.com/cowprotocol/contracts/blob/main/src/contracts/interfaces/GPv2Authentication.sol
*/

/// @title CoW Protocol Authentication Interface
/// @author CoW DAO developers
interface ICowAuthentication {
/// @dev determines whether the provided address is an authenticated solver.
/// @param prospectiveSolver the address of prospective solver.
/// @return true when prospectiveSolver is an authenticated solver, otherwise false.
function isSolver(address prospectiveSolver) external view returns (bool);
}

/// @title CoW Protocol Settlement Interface
/// @notice Minimal interface for CoW Protocol's settlement contract
/// @dev Used for type-safe calls to the settlement contract's settle function
interface ICowSettlement {
/// @notice Trade data structure matching GPv2Settlement
struct Trade {
uint256 sellTokenIndex;
uint256 buyTokenIndex;
address receiver;
uint256 sellAmount;
uint256 buyAmount;
uint32 validTo;
bytes32 appData;
uint256 feeAmount;
uint256 flags;
uint256 executedAmount;
bytes signature;
}

/// @notice Interaction data structure for pre/intra/post-settlement actions which are supplied by the solver to complete the user request
struct Interaction {
address target;
uint256 value;
bytes callData;
}

/// @notice Returns the authentication contract used by the settlement contract.
function authenticator() external view returns (ICowAuthentication);

/// @notice Returns the address of the vaultRelayer, the target for approvals for funds entering the settlement contract.
function vaultRelayer() external view returns (address);

/// @notice Returns the domain separator for EIP-712 signing
function domainSeparator() external view returns (bytes32);

/// @notice Allows for approval of orders by submitting an authorized hash on-chain prior to order execution.
function setPreSignature(bytes calldata orderUid, bool signed) external;

/// @notice Settles a batch of trades atomically
/// @param tokens Array of token addresses involved in the settlement
/// @param clearingPrices Array of clearing prices for each token
/// @param trades Array of trades to execute
/// @param interactions Array of three interaction arrays (pre, intra, post-settlement)
function settle(
address[] calldata tokens,
uint256[] calldata clearingPrices,
Trade[] calldata trades,
Interaction[][3] calldata interactions
) external;
}
Copy link

Choose a reason for hiding this comment

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

this part needs to remain in vendor when you move the other stuff to src


/// @title CoW Protocol Wrapper Interface
/// @notice Interface for wrapper contracts that add custom logic around CoW settlements
/// @dev Wrappers can be chained together to compose multiple settlement operations
interface ICowWrapper {
/// @notice A human readable label for this wrapper. Used for display in explorer/analysis UIs
Copy link

Choose a reason for hiding this comment

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

nit: adding logs, as I think this is handy when debugging backend validations, etc

Suggested change
/// @notice A human readable label for this wrapper. Used for display in explorer/analysis UIs
/// @notice A human readable label for this wrapper. Used for display in explorers, UIs and logs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

which logs would it be used in? currently the wrapper does not emit a log (in order to try and keep gas down). Usage of a wrapper can be detected by tracing a settlement call for wrappedSettle calls. This seems to be something that the backend team has been doing already.

function name() external view returns (string memory);

/// @notice The settlement contract used by this wrapper
/// @return The CowSettlement contract address
function SETTLEMENT() external view returns (ICowSettlement);

/// @notice Initiates a wrapped settlement call
/// @dev This is the entry point for wrapped settlements. The wrapper will execute custom logic
/// before calling the next wrapper or settlement contract in the chain.
/// @param settleData ABI-encoded call to ICowSettlement.settle() containing trade data
/// @dev SECURITY: `settleData` is NOT guaranteed to remain unchanged through the wrapper chain.
/// Intermediate wrappers could modify it before passing it along. Do not rely on
/// `settleData` validation for security-critical checks.
/// @param wrapperData Encoded wrapper chain with the following format:
/// Structure: [uint16 len1][bytes data1][address wrapper2][uint16 len2][bytes data2][address wrapper3]...
///
/// Each wrapper in the chain consists of:
/// - 2 bytes: uint16 length of wrapper-specific data
/// - `length` bytes: wrapper-specific data for this wrapper
/// - 20 bytes: address of next wrapper (omitted for the final wrapper)
///
/// The final wrapper in the chain omits the next wrapper address and calls SETTLEMENT directly.
///
/// Example: [0x0005][0xAABBCCDDEE][0x1234...ABCD][0x0003][0x112233]
/// ↑len ↑data ↑next wrapper ↑len ↑data (final, no next address)
///
function wrappedSettle(bytes calldata settleData, bytes calldata wrapperData) external;
}

/// @title CoW Protocol Wrapper Base Contract
/// @notice Abstract base contract for creating wrapper contracts around CoW Protocol settlements
/// @dev A wrapper enables custom pre/post-settlement and context-setting logic and can be chained with other wrappers.
/// Wrappers must:
/// - Be approved by the ICowAuthentication contract
/// - Verify the caller is an authenticated solver
Copy link

Choose a reason for hiding this comment

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

this is not the same as the previous point?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, here it's about who calls it, before it's about who it's going to call. The call should be:

caller -> wrapper -> settlement
solver  | solver   |

the first says

caller -> wrapper
solver  |

the second says

caller -> wrapper -> settlement
        | solver

/// - Eventually call settle() on the approved ICowSettlement contract
/// - Implement _wrap() for custom logic
abstract contract CowWrapper is ICowWrapper {
/// @notice Thrown when the caller is not an authenticated solver
/// @param unauthorized The address that attempted to call wrappedSettle
error NotASolver(address unauthorized);

/// @notice Thrown when settle data doesn't contain the correct function selector
/// @param invalidSettleData The invalid settle data that was provided
error InvalidSettleData(bytes invalidSettleData);

/// @notice The settlement contract
ICowSettlement public immutable SETTLEMENT;

/// @notice The authentication contract used to verify solvers
/// @dev This is derived from `SETTLEMENT.authenticator()`.
ICowAuthentication public immutable AUTHENTICATOR;

/// @notice Constructs a new CowWrapper
/// @param settlement_ The ICowSettlement contract to use at the end of the wrapper chain. Also used for wrapper authentication.
constructor(ICowSettlement settlement_) {
SETTLEMENT = settlement_;
AUTHENTICATOR = settlement_.authenticator();
}

/// @inheritdoc ICowWrapper
function wrappedSettle(bytes calldata settleData, bytes calldata wrapperData) external {
// Revert if not a valid solver
require(AUTHENTICATOR.isSolver(msg.sender), NotASolver(msg.sender));

// Find out how long the next wrapper data is supposed to be
// We use 2 bytes to decode the length of the wrapper data because it allows for up to 64KB of data for each wrapper.
// This should be plenty of length for all identified use-cases of wrappers in the forseeable future.
uint256 nextWrapperDataLen = uint16(bytes2(wrapperData[0:2]));

// Delegate to the wrapper's custom logic
uint256 remainingWrapperDataStart = 2 + nextWrapperDataLen;
_wrap(settleData, wrapperData[2:remainingWrapperDataStart], wrapperData[remainingWrapperDataStart:]);
}

/// @notice Internal function containing the wrapper's custom logic
/// @dev Must be implemented by concrete wrapper contracts. Should execute custom logic
/// then eventually call _next() to continue the wrapped settlement chain.
Comment on lines +156 to +157
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not having this abstract contract calling _next in wrappedSettle after the call to _wrap?
In this way, this function signature becomes simpler (_wrap(bytes calldata settleData, bytes calldata wrapperData) and implementations don't have to worry about making the chain continue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we cant do this because some wrappers (such as euler) want to execute calls/data after the settlement. or they want to execute _next inside of a special context (such as the EVC). In these cases, adding _next to the end of the wrappedSettle call would not work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

/// @param settleData ABI-encoded call to ICowSettlement.settle()
/// @param wrapperData The wrapper data which should be consumed by this wrapper
/// @param remainingWrapperData Additional wrapper data. It is the reminder bytes resulting from consuming the current's wrapper data from the original `wrapperData` in the `wrappedSettle` call. This should be passed unaltered to `_next` that will call the settlement function if this remainder is empty, or delegate the settlement to the next wrapper
function _wrap(bytes calldata settleData, bytes calldata wrapperData, bytes calldata remainingWrapperData)
internal
virtual;

/// @notice Continues the wrapped settlement chain by calling the next wrapper or settlement contract
/// @dev Extracts the next target address from wrapperData and either:
Copy link

Choose a reason for hiding this comment

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

Suggested change
/// @dev Extracts the next target address from wrapperData and either:
/// @dev Extracts the next wrapper address from the remainingWrapperData and either:

/// - Calls ICowSettlement.settle() directly if no more wrappers remain, or
/// - Calls the next CowWrapper.wrappedSettle() to continue the chain
/// @param settleData ABI-encoded call to ICowSettlement.settle()
/// @param remainingWrapperData Remaining wrapper data starting with the next target address (20 bytes)
function _next(bytes calldata settleData, bytes calldata remainingWrapperData) internal {
if (remainingWrapperData.length == 0) {
// No more wrapper data - we're calling the final settlement contract
// Verify the settle data has the correct function selector
require(bytes4(settleData[:4]) == ICowSettlement.settle.selector, InvalidSettleData(settleData));
Copy link

Choose a reason for hiding this comment

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

Performance Consideration: Error data allocation

As @fedgiac mentioned, when custom errors include large calldata parameters like settleData, the error may be allocated in memory even when the require condition is true. With large settlement data, this could lead to unnecessary gas costs from extra mstore operations.

Recommendation: Consider benchmarking this with large settleData to verify gas impact. If significant, could refactor to:

if (bytes4(settleData[:4]) != ICowSettlement.settle.selector) {
    revert InvalidSettleData(settleData);
}

Though given your use of require() style elsewhere per the coding guidelines, this may be acceptable if the gas impact is negligible.


// Call the settlement contract directly with the settle data
(bool success, bytes memory returnData) = address(SETTLEMENT).call(settleData);

if (!success) {
// Bubble up the revert reason from the settlement contract
assembly ("memory-safe") {
revert(add(returnData, 0x20), mload(returnData))
}
}
} else {
// Extract the next wrapper address from the first 20 bytes of wrapperData
address nextWrapper = address(bytes20(remainingWrapperData[:20]));

Choose a reason for hiding this comment

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

I know that this will go through simulation but I wonder if we should do a check like this

require(nextWrapper != address(0), BadRemainingWrapperData());

Specially for debugging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could be helpful. but the more likely case is that the data is simply just not there (ie there are not 20 bytes to read)

Copy link

Choose a reason for hiding this comment

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

Enhancement: Consider zero address validation

While @carlos-cow and @kaze-cow discussed this, an explicit zero-address check provides clearer debugging output for malformed wrapper data:

address nextWrapper = address(bytes20(remainingWrapperData[:20]));
require(nextWrapper != address(0), InvalidWrapperAddress());

However, this is lower priority if bounds validation is added at line 140, which would catch most malformation cases. This is optional but could improve the developer experience.


// Skip past the address we just read
remainingWrapperData = remainingWrapperData[20:];

// More wrapper data remains - call the next wrapper in the chain
CowWrapper(nextWrapper).wrappedSettle(settleData, remainingWrapperData);
}
}
}
Loading