Skip to content
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

feat: limit orders contract #1287

Open
wants to merge 23 commits into
base: release-candidate
Choose a base branch
from
Open

Conversation

MilGard91
Copy link
Contributor

@MilGard91 MilGard91 commented Nov 21, 2024

Description

Closes #1308

Add new contract that will handle verification and execution of cover limit orders

Testing

In progress

Checklist

  • Performed a self-review of my own code
  • Made corresponding changes to the documentation

@rackstar
Copy link
Contributor

relates to #1249

Copy link
Contributor

@rackstar rackstar left a comment

Choose a reason for hiding this comment

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

few comments

@MilGard91 MilGard91 force-pushed the feat/limit-orders branch 6 times, most recently from e05b18f to bccbe5a Compare November 28, 2024 14:13
@shark0der shark0der changed the title feat: add a contract for validating and executing limit orders feat: limit orders contract Nov 28, 2024
@MilGard91 MilGard91 force-pushed the feat/limit-orders branch 3 times, most recently from a8e3bde to 9fdbb38 Compare November 28, 2024 16:38
@@ -2,10 +2,12 @@

pragma solidity ^0.8.18;
import "../../generic/CoverGeneric.sol";
import "hardhat/console.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 🧹 🧹

@@ -467,7 +471,7 @@ contract Cover is ICover, MasterAwareV2, IStakingPoolBeacon, ReentrancyGuard, Mu

if (remainder > 0) {
// solhint-disable-next-line avoid-low-level-calls
(bool ok, /* data */) = address(msg.sender).call{value: remainder}("");
(bool ok, /* data */) = address(buyer).call{value: remainder}("");
Copy link
Contributor

Choose a reason for hiding this comment

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

this might be undesirable since the buyer pays with WETH, but let's leave it as is and I'll get to it when I rebase on top of this. It's going to create conflicts anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

would help to leave a todo though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the WETH is the payment method the buyer address is LimitOrders not the signer... That's where we do the conversion from WETH to ETH and other way around...

@@ -127,16 +130,14 @@ contract LimitOrders is ILimitOrders, MasterAwareV2, EIP712 {
emit OrderCancelled(id);
}

/// @notice Handles verification of the order signature
/// @notice Returns the hash of the structured data of the order
Copy link
Contributor

Choose a reason for hiding this comment

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

struct is ambiguous, build is a bit misleading too, why not call the function _getOrderHash?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also include the _hashTypedDataV4 function call here as well so you'd return the actual digest directly and can call the function getOrderId. The execution details hashing may be moved here as well. The _extractIdAndSigner function can then be removed as the only extra thing it does is ECDSA.recover which can be inlined. I would also make the getOrderId function public as it may be useful for testing but could be used by other contracts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit Order Contract
3 participants