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

Add the ability to update an existing order to provide more liquidity #19

Open
wants to merge 1 commit into
base: partial_order_fills
Choose a base branch
from

Conversation

kahuang
Copy link
Collaborator

@kahuang kahuang commented Jul 23, 2022

No description provided.

@kahuang kahuang requested a review from dcposch July 23, 2022 17:34
@kahuang
Copy link
Collaborator Author

kahuang commented Jul 23, 2022

Fixes #16

require(amountSats < 0 && uint256(int256(-(o.amountSats + amountSats))) <= MAX_SATS, "Must be adding a valid amount of liquidity");

uint256 additionalValue = uint256(int256(-amountSats * int128(o.priceTokPerSat)));
o.amountSats += amountSats;
Copy link
Owner

Choose a reason for hiding this comment

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

👍


if (o.amountSats < 0) {
// This is an ask. Receive the ether to be sold and update the order.
require(amountSats < 0 && uint256(int256(-(o.amountSats + amountSats))) <= MAX_SATS, "Must be adding a valid amount of liquidity");
Copy link
Owner

Choose a reason for hiding this comment

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

I'd calculate the new amount o.amountSats + amountSats first.

Gotta check that it's still negative, otherwise this function lets a user convert a bid into an ask.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If 0.amountSats and amountSats are the same sign, is that something we have to worry about?

Maybe in the overflow case, but since we're on solidity 0.8+ that's checked automatically AFAIK

Copy link
Owner

Choose a reason for hiding this comment

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

I see--so the goal is to only let people add liquidity?

Then, for an existing order, you can increase the size via updateOrder, but if you want to decrease the size or change the price, you have to cancel + place a new order.

LMK if I understood it right


// Receive the additional stake
_transferFromSender(expectedStakeTok);
}
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if this is worth the complexity.

The API, as it stands, is a big gnarly:

  • For a bid, passing positive amountSats increases the size of your bid
  • For an ask, passing positive amountSats decreases the size of your ask

In either case, we'd have to add a check to ensure bids are not turning into asks or vice versa.

I'm down to revisit later once we have testnet MMs. If it turns out that modifying bid size is a common need, we ship this.

My guess is that at least initially, immutable orders (that can only be cancelled and recreated) might be good enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I should probably have added more clarity:

This only lets you add additional liquidity, not remove it. The removing liquidity case strikes me as less likely, and therefore ok to go through the: cancel order -> place order flow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The adding liquidity case I think will be common. Lets take a WBTC <-> BTC portal where the spreads are tight and non-volatile. MM can have large liquidity, and most orders won't take the whole order from the consumer side. Being able to in place update your order to add additional liquidity strikes me as easier to manage than needing to place a separate order, which would then also fragment liquidity and make it harder to match orders.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That being said, I don't think it'd be too hard to add later so we can punt on this for now

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