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

Allow call option writers to bid on spread #3

Merged

Conversation

regynald
Copy link
Contributor

If the strike price of a call option is 1000 wei and the current high bid (from non option writer) is 1001 wei the option writer would only need to bid 2 wei to become the new high bidder.

This PR adds that functionality for bidding and logic for settling. Also tests for this.

@regynald regynald requested a review from jake-nyquist April 25, 2022 02:07
@linear
Copy link

linear bot commented Apr 25, 2022

HOOK-805 [call options] allow option writer to bid using only the spread

The option writer currently must put up the full amount of ETH to make a settlement bid. This would allow them to bid using only the difference between the strike and their spread. It also requires that all points where eth is distributed are aware of this (possible refactoring).

Copy link
Contributor

@jake-nyquist jake-nyquist left a comment

Choose a reason for hiding this comment

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

I think we should discuss the following alternative method:

 if bidder == writer:
    _writerBid();
 else:
    _generalBid();

....

factoring out methods to return the previous high bidders funds & to distribute the assets correctly.

....

Comment on lines 272 to 279
uint256 unnormalizedHighBid = call.bid;
if (call.highBidder == call.writer) {
unnormalizedHighBid -= call.strike;
}

// return current bidder's money
(bool sent, ) = call.highBidder.call{value: call.bid}("");
(bool sent, ) = call.highBidder.call{value: unnormalizedHighBid}("");
require(sent, "Failed to send Ether");
Copy link
Contributor

Choose a reason for hiding this comment

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

my sense is that this string of logic (+ the previous logic) may have enough complexity to just be factored out into _acceptBidFromNewBidder and _returnBidToPreviousBidder


) = call.writer.call{value: call.strike}("");
require(writerSent, "Failed to send Ether to option writer");
// If the option writer is the high bidder then they don't recieve the strike and only the underlying asset
Copy link
Contributor

Choose a reason for hiding this comment

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

... they don't receive [sp] the strike because, when they place a bid, they only paid the spread.

);
}

function testWriterCanOutbidOnSpread() public {
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 like to see a test for these cases:

  • writer gets outbid again themselves
  • settlement happens in each case (writer bids first, writer bids last, writer outbid again)
  • reclaiming happens in each case
  • multiple options exist in the contract at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

in particular, I think testing the reclaim logic is important here. Even if it works now I'm concerned that future changes might be breaking for the reclaims.

@jake-nyquist
Copy link
Contributor

nit: in the future, would be good to do the PR for removing all of the .gitignored files separate from this PR.

@regynald regynald force-pushed the regynald/hook-805-call-options-allow-option-writer-to-bid branch from b29eeb9 to 4f90328 Compare May 3, 2022 21:49
@@ -28,7 +30,8 @@ contract HookCoveredCallImplV1 is
IHookCoveredCall,
ERC721Burnable,
ReentrancyGuard,
Initializable
Initializable,
Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove Test here

@@ -20,6 +20,8 @@ import "./interfaces/IHookCoveredCall.sol";
import "./interfaces/IHookProtocol.sol";
import "./interfaces/IWETH.sol";

import "forge-std/Test.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

remove test here

@jake-nyquist
Copy link
Contributor

lgtm

@regynald regynald merged commit 02cd9c6 into main May 3, 2022
@regynald regynald deleted the regynald/hook-805-call-options-allow-option-writer-to-bid branch May 4, 2022 04:06
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.

2 participants