-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add wrapper #6
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: master
Are you sure you want to change the base?
Changes from 38 commits
70bb676
4a1683a
a7c03fe
511f7aa
e82bc8a
edce0df
594613e
d9812ed
8803d2a
b164062
19b8010
1e37149
e583a6e
323e7ce
640529a
00ff9ba
cdf82fd
e5faddf
961e71a
7192697
7fea7ba
61051cd
6b12785
b1d1127
9739ab1
bcda0c2
8529191
41ddd46
63cc658
a464c98
ca2c6b4
549d740
3a1654f
972f958
b4c9b97
fe6446a
242e7c5
13e09aa
7c6be09
0313ff2
d358b09
694e7b0
bccb8db
c4f4aea
9bac22f
5e3e4ee
d828cdb
daf68fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| # CLAUDE.md | ||
|
|
||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing of this is vendored so it should be moved to
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the reason I put How about moving it (and any related sources) to a new subdirectory There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah,
It makes sense to separate the wrapper code. Overall, I'm ok with keeping
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
|
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,175 @@ | ||||||
| // SPDX-License-Identifier: MIT OR Apache-2.0 | ||||||
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| pragma solidity >=0.7.6 <0.9.0; | ||||||
| pragma abicoder v2; | ||||||
|
|
||||||
| /** | ||||||
| * @title CoW Wrapper all-in-one integration file | ||||||
| * @author CoW Protocol Developers | ||||||
| * @notice 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 | ||||||
| */ | ||||||
|
Comment on lines
4
to
13
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tags here don't apply to the file but to the next entry in the file, so you'd get that
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| /// @title CoW Protocol Authentication Interface | ||||||
| /// @author CoW DAO developers | ||||||
| interface ICowAuthentication { | ||||||
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| /// @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 hooks | ||||||
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| struct Interaction { | ||||||
| address target; | ||||||
| uint256 value; | ||||||
| bytes callData; | ||||||
| } | ||||||
|
|
||||||
| /// @notice Returns the authentication contract used by the settlement contract. | ||||||
| function authenticator() external returns (ICowAuthentication); | ||||||
|
|
||||||
| /// @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; | ||||||
| } | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
| function name() external view returns (string memory); | ||||||
|
|
||||||
| /// @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() | ||||||
| /// @param wrapperData Encoded chain of wrapper-specific data followed by addresses of next wrappers/settlement | ||||||
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| 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 | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not the same as the previous point?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: the first says the second says |
||||||
| /// - Eventually call settle() on the approved ICowSettlement contract | ||||||
| /// - Implement _wrap() for custom logic | ||||||
| /// - Implement parseWrapperData() for validation of implementation-specific wrapperData | ||||||
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| 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); | ||||||
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| /// @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(); | ||||||
| } | ||||||
|
|
||||||
| /// @notice Initiates a wrapped settlement call | ||||||
| /// @dev Entry point for solvers to execute wrapped settlements. Verifies the caller is a solver, | ||||||
| /// validates wrapper data, then delegates to _wrap() for custom logic. | ||||||
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| /// @param settleData ABI-encoded call to ICowSettlement.settle() containing trade data | ||||||
| /// @param wrapperData Encoded data for this wrapper and the chain of next wrappers/settlement. | ||||||
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| /// Format: [2-byte len][wrapper-specific-data][next-address]([2-byte len][wrapper-specific-data][next-address]...) | ||||||
|
||||||
| /// Format: [2-byte len][wrapper-specific-data][next-address]([2-byte len][wrapper-specific-data][next-address]...) | |
| /// Format: [2-byte len][wrapper-specific-data]([next-address][2-byte len][wrapper-specific-data]...) |
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
Critical: Missing bounds validation
This line reads 2 bytes without validating wrapperData has sufficient length. Solidity calldata slicing returns zero-padding for out-of-bounds access rather than reverting, allowing malformed data to be processed.
Add validation:
require(wrapperData.length >= 2, InvalidWrapperData());
uint16 nextWrapperDataLen = uint16(bytes2(wrapperData[0:2]));
require(wrapperData.length >= 2 + nextWrapperDataLen, InvalidWrapperData());Define error: error InvalidWrapperData();
This has been flagged by multiple reviewers but remains unaddressed.
Outdated
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.
Critical: Missing bounds validation
This line reads 2 bytes without first validating that wrapperData has sufficient length. In Solidity, calldata slicing returns zero-padding for out-of-bounds access rather than reverting, which can lead to silent failures.
Recommendation:
require(wrapperData.length >= 2, InvalidWrapperData());
uint16 nextWrapperDataLen = uint16(bytes2(wrapperData[0:2]));
require(wrapperData.length >= 2 + nextWrapperDataLen, InvalidWrapperData());Add error definition:
error InvalidWrapperData();This prevents malformed wrapper data from causing unpredictable behavior in the wrapper chain.
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.
This makes sense, though the first require seems unneeded since if the check were to fail but be ignored then nextWrapperDataLen would be zero and the next require would cause a revert.
But maybe this isn't needed at all, do you know if Solidity calldata array access checks boundary conditions? In case it would be nice to have a test.
Outdated
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.
Critical Security Issue: Missing bounds validation
This line reads 2 bytes from wrapperData without validating that the data has sufficient length. While Solidity calldata slicing returns zero-padding for out-of-bounds access rather than reverting, this can lead to silent failures:
- Empty or 1-byte
wrapperDatawould read as length 0 (or partial value) - Line 144 could then slice beyond available data
- Malformed data could pass through without appropriate validation
Recommendation:
require(wrapperData.length >= 2, InvalidWrapperData());
uint16 nextWrapperDataLen = uint16(bytes2(wrapperData[0:2]));
require(wrapperData.length >= 2 + nextWrapperDataLen, InvalidWrapperData());Add error definition:
error InvalidWrapperData();This was flagged by @fedgiac at line 140 but remains unaddressed. It's critical for preventing malformed wrapper data from causing unpredictable behavior.
Outdated
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.
does it make sense to save 2 + nextWrapperDataLen in a variable so u can make it easier on the eyes?
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.
we can do it... just adverse to it due to stackToDeep errors
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 d358b09
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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.
| /// @dev Extracts the next target address from wrapperData and either: | |
| /// @dev Extracts the next wrapper address from the remainingWrapperData and either: |
kaze-cow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
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.
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.
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.
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
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.
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)
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
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.
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.
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.
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.
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.
kill this.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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 toreadme.mdif it makes it simpler to maintain the actual readme.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.
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.mdliterally just beRead README.mdor soThere 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.
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.