feat: close position wrapper #15
Conversation
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| uint256 debtAmount = IBorrowing(vault).debtOf(account); | ||
|
|
||
| // repay as much debt as we can | ||
| uint256 repayAmount = asset.balanceOf(owner); |
There was a problem hiding this comment.
Logic Issue: This line reads the balance from owner but the comment on line 239 says funds will be pulled from the user. There's a mismatch here.
The balance check should be on address(this) since this is where the settlement contract sends the repayment assets after the swap. The CoW settlement should have already transferred the assets to this contract before helperRepay is called.
Consider:
uint256 repayAmount = asset.balanceOf(address(this));Then transfer from address(this) to the vault instead of pulling from owner.
| } | ||
|
|
||
| // pull funds from the user (they should have approved spending by this contract) | ||
| SafeERC20Lib.safeTransferFrom(asset, owner, address(this), repayAmount, address(0)); |
There was a problem hiding this comment.
Logic Issue: This attempts to pull funds from owner, but based on the documented flow, the repayment assets should already be in this contract from the settlement.
The settlement executes first (step 1 in _wrap), which swaps collateral → repayment assets. According to the contract comments (lines 17-18), the recipient of the swap should be THIS contract. Therefore, this line should not be pulling from owner.
If you need to pull from owner, the order receiver in the settlement should be set to owner, not this contract. Currently there's an inconsistency between the documented flow and implementation.
| if (params.kind == KIND_BUY) { | ||
| (uint256 collateralVaultPrice, uint256 borrowPrice) = | ||
| _findRatePrices(settleData, params.collateralVault, params.borrowVault); | ||
| transferAmount = params.repayAmount * borrowPrice / collateralVaultPrice; |
There was a problem hiding this comment.
Potential Integer Division Precision Loss: This calculation could lose precision due to integer division. Consider whether rounding should be in favor of transferring slightly more collateral to prevent settlement failures.
// Consider rounding up to ensure sufficient collateral
transferAmount = (params.repayAmount * borrowPrice + collateralVaultPrice - 1) / collateralVaultPrice;
|
|
||
| /** | ||
| * @dev | ||
| */ |
There was a problem hiding this comment.
Missing Documentation: The collateralAmount field lacks a description. Based on usage, it appears to be the amount of collateral shares to sell. Please add:
/**
* @dev The amount of collateral vault shares to sell/withdraw for repayment
*/
| // - 32 bytes: signature length | ||
| // - N bytes: signature data (padded to 32-byte boundary) | ||
| // We can just math this out | ||
| uint256 consumed = 256 + 64 + ((signature.length + 31) & ~uint256(31)); |
There was a problem hiding this comment.
Code Quality: The manual calculation of consumed bytes is error-prone and duplicates similar logic in OpenPositionWrapper. Consider extracting this to a shared helper function or library to reduce duplication and maintenance burden.
The calculation differs slightly between wrappers (256 vs 224 bytes for params), making it easy to introduce bugs when adding/removing fields.
| new ICowSettlement.Interaction[](2), | ||
| new ICowSettlement.Interaction[](0) | ||
| ]; | ||
| r.interactions[1][0] = getWithdrawInteraction(sellVaultToken, buyAmount * r.clearingPrices[1] / 1e18); |
There was a problem hiding this comment.
Test Issue: The withdraw interaction calculation appears incorrect. You're using buyAmount * r.clearingPrices[1] / 1e18 but this doesn't account for the vault share-to-asset conversion rate.
For a KIND_BUY order where you want to buy exactly buyAmount of repayment token, you need to calculate how many vault shares to sell based on the vault's conversion rate, not just the price ratio.
|
|
||
| // Build the EVC batch items for closing a position | ||
| // 1. Settlement call | ||
| items[itemIndex++] = IEVC.BatchItem({ |
There was a problem hiding this comment.
Ordering Concern: The settlement call (item 0) executes before the permit (item 1) in the non-pre-approved flow. This means the settlement happens before the wrapper has permission to operate on behalf of the user.
Consider reordering so permit comes first:
- Permit (if needed)
- Settlement call
This matches the OpenPositionWrapper pattern where authorization happens before the core operations.
Refactor CowEvcClosePositionWrapper test files to reduce code duplication and improve maintainability: Integration tests (CowEvcClosePositionWrapper.t.sol): - Add helper functions for common test setup patterns - _createDefaultParams(): standardized param creation - _setupClosePositionApprovals(): approval setup for close position - _setupPreApprovedFlow(): pre-approved hash flow setup - _createPermitSignature(): permit signature creation - _encodeWrapperData(): wrapper data encoding - _executeWrappedSettlement(): settlement execution - Add DEFAULT_SELL_AMOUNT and DEFAULT_BUY_AMOUNT constants - Reduce test function size significantly while maintaining clarity Unit tests (CowEvcClosePositionWrapper.unit.t.sol): - Add helper functions for common patterns - _getDefaultParams(): returns default params (tests modify as needed) - _getEmptySettleData(): creates empty settlement data - _getSettleDataWithTokens(): creates settlement data with tokens/prices - _encodeWrapperData(): wrapper data encoding - _setupPreApprovedHash(): pre-approved hash setup - Add DEFAULT_REPAY_AMOUNT and KIND_BUY constants - Remove 5 redundant tests with no coverage impact: - test_Constructor_SetsName (trivial getter) - test_GetApprovalHash_Consistency (deterministic function) - test_ParseWrapperData_WithSignature (duplicate code path) - test_GetSignedCalldata_FullRepay (covered by other tests) - test_GetSignedCalldata_ContainsRepayItem (duplicate of RepayItem) - Reduce from 32 to 27 unit tests with no loss in coverage All tests pass: 7/7 integration tests, 27/27 unit tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Description
Summarize what this PR does and why in a single, clear sentence.
A reader should understand the purpose and scope of the change just from this line.
Context
Explain the motivation or problem this PR addresses.
Include links to any relevant documentation, tickets, or discussions.
Provide background that helps reviewers understand why the change is needed.
Highlight any important design decisions, dependencies, or related components.
Out of Scope
Specify what is not covered in this PR.
This helps reviewers focus on the intended scope and prevents scope creep.
Testing Instructions
Provide clear, step-by-step instructions for verifying this change locally.
Assume the reviewer has no prior context about your setup.
This section reinforces the scope of the PR by outlining what should be tested and what the expected outcomes are.