Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables token swapping functionality for buy orders by introducing a new BuyOrderSwapFacet that allows users to swap tokens for GHST and place ERC721/ERC1155 buy orders in a single transaction.
- Adds new
BuyOrderSwapFacetwith swap-and-place functions for both ERC721 and ERC1155 buy orders - Refactors existing buy order facets to use shared validation and order placement logic from
LibBuyOrder - Includes comprehensive integration tests for the new swap functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/Aavegotchi/facets/BuyOrderSwapFacet.sol | New facet providing swap-and-place functionality for buy orders |
| contracts/Aavegotchi/libraries/LibBuyOrder.sol | Adds shared validation and order placement functions, moves events from facets |
| contracts/Aavegotchi/facets/ERC721BuyOrderFacet.sol | Refactored to use shared LibBuyOrder functions, removes duplicate validation logic |
| contracts/Aavegotchi/facets/ERC1155BuyOrderFacet.sol | Refactored to use shared LibBuyOrder functions, removes duplicate validation logic |
| test/swapAndPlaceERC721BuyOrder.test.ts | Integration test for ERC721 swap-and-place functionality |
| test/swapAndPlaceERC1155BuyOrder.test.ts | Integration test for ERC1155 swap-and-place functionality |
| scripts/upgrades/upgrade-addSwapAndBuyOrders.ts | Upgrade script to deploy the new BuyOrderSwapFacet |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // require(_priceInWei >= 1e18, "ERC721BuyOrder: price should be 1 GHST or larger"); | ||
|
|
There was a problem hiding this comment.
[nitpick] Remove commented out code instead of leaving it in place. The validation has been moved to LibBuyOrder.validateERC721Params.
| // require(_priceInWei >= 1e18, "ERC721BuyOrder: price should be 1 GHST or larger"); |
| // Refund any excess GHST to the recipient using shared library, leave totalCost in the diamond | ||
| LibTokenSwap.refundExcessGHST(params.recipient, initialBalance + totalCost); | ||
|
|
||
| emit SwapAndPlaceERC1155BuyOrder(msg.sender, params.buyOrderId, params.tokenIn, ghstReceived); |
There was a problem hiding this comment.
The buyOrderId parameter is used in the event emission but it's not set during the function execution. The actual buyOrderId is generated inside LibBuyOrder._placeERC1155BuyOrder and LibBuyOrder._placeERC721BuyOrder functions, but those values are not returned or captured here.
| // Refund any excess GHST to the recipient using shared library, leave totalCost in the diamond | ||
| LibTokenSwap.refundExcessGHST(params.recipient, initialBalance + totalCost); | ||
|
|
||
| emit SwapAndPlaceERC721BuyOrder(msg.sender, params.buyOrderId, params.tokenIn, ghstReceived); |
There was a problem hiding this comment.
The buyOrderId parameter is used in the event emission but it's not set during the function execution. The actual buyOrderId is generated inside LibBuyOrder._placeERC1155BuyOrder and LibBuyOrder._placeERC721BuyOrder functions, but those values are not returned or captured here.
enable swaps for buy orders
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
No description provided.