-
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 all 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,197 @@ | ||||||
| // 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.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 | ||||||
| */ | ||||||
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 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; | ||||||
| } | ||||||
|
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 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 | ||||||
|
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 | ||||||
| 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(); | ||||||
| } | ||||||
|
|
||||||
| /// @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
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. Why not having this abstract contract calling
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. we cant do this because some wrappers (such as euler) want to execute calls/data after the settlement. or they want to execute
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. see how it works with euler close position: https://github.com/cowprotocol/euler-integration-contracts/pull/15/files |
||||||
| /// @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: | ||||||
|
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.
Suggested change
|
||||||
| /// - 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)); | ||||||
kaze-cow marked this conversation as resolved.
Show resolved
Hide resolved
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. Performance Consideration: Error data allocation As @fedgiac mentioned, when custom errors include large calldata parameters like Recommendation: Consider benchmarking this with large if (bytes4(settleData[:4]) != ICowSettlement.settle.selector) {
revert InvalidSettleData(settleData);
}Though given your use of |
||||||
|
|
||||||
| // 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])); | ||||||
|
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 know that this will go through simulation but I wonder if we should do a check like this Specially for debugging
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. 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 commentThe 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); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
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.