-
Notifications
You must be signed in to change notification settings - Fork 47
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
feature: add outbound-liquidity auction market, bidder pays for channel acceptance #390
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick pass, looks almost ready. Nice go get this with such a small diff.
after we get ZC and unannounced channels merged, we'll need 2x review on this with Roas to cap it out |
Should be rebased on the compressed/combined ZC PR. |
fcd6509
to
35674c1
Compare
3007f3b
to
7d1dfa6
Compare
@positiveblue, remember to re-request review from reviewers when ready |
dd2a017
to
695bdfb
Compare
695bdfb
to
8b006e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a few more comments, but we're very close!
7222d26
to
0a8ef8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great work 🎉
c42a99a
to
0afb46c
Compare
0afb46c
to
a0d5896
Compare
…-status status: add `update status` endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an initial pass, but tbh I'm still working to get an accurate mental model in my head of: the way the new feee accounting works, and scenarios in which a buyer/seller would want to enter into this new engagement.
Would be it be accurate to say that (assuming a fixed number of connection slots), this lets a party sell/auction off an inbound (from their PoV) connection/channel slot?
auctionType := ourOrder.Details().AuctionType | ||
if auctionType == order.BTCOutboundLiquidity { | ||
premiumAmt += btcutil.Amount( | ||
selfChanBalance, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases though, we expect the channel amount to be nothing, and the actual value being the self channel balance right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, for the BTCOutboundLiquidity
the chan amount needs to be always 100k (1 unit). This way we can roll it out and check the demand for this feature without having to make big refactor
a0d5896
to
46df668
Compare
return fmt.Errorf("to participate in the outbound liquidity " + | ||
"market the order amt should be exactly 100k sats") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message talks about amount whereas the if conditional examines units. I expected something like:
if o.Details().AuctionType == order.BTCOutboundLiquidity &&
o.Details().Amt != order.BaseSupplyUnit {
return fmt.Errorf("to participate in the outbound liquidity " +
"market the order amt should be exactly the base supply amount (%v sats)", order.BaseSupplyUnit)
}
Does that seem reasonable?
EDIT: FYI, I've just noticed that testing on amt would mirror this change:
https://github.com/lightninglabs/pool/pull/390/files#diff-60f9a53c1dcbdd7baee29e9e4ae17725d00da191f07abcc50f9e3c035c87c247R509
case ourOrder.Details().AuctionType != BTCOutboundLiquidity && | ||
unitsFilled < ourOrder.Details().MinUnitsMatch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these lines are correct. I suppose that if the bid is for oubound liquidity only a single unit will be filled. I wound consider adding a comment.
Further, I wander whether a third case is necessary:
case ourOrder.Details().AuctionType == BTCOutboundLiquidity &&
unitsFilled == 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a few comments around the parts which I'm uncertain about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐞
defaultAuctionType = auctionTypeInboundLiquidity | ||
|
||
submitOrderDescription = ` | ||
Submit a new order in the given market. Currently, Pool supports two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great description 👍
46df668
to
d061931
Compare
d061931
to
b851699
Compare
Allow the submission of orders for different auction markets. Expand the current auctions, bidder pays for inbound liquidity, with another market where the bidder pays for channel acceptor, outbound liquidity.
To be able to participate in the new auctions:
The asker needs to:
BTCOutboundLiquidity
BaseSupply
)min_chan_size
to 100k or more. This will take into account the bidders funding supply.The bidder needs to:
BTCOutboundLiquidity
BaseSupply
)self_chan_balance
to 100k or more.Sidecar tickets are not supported yet for this new market.
NOTE: Only the last 5 commits need to be reviewed in this PR