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 distinct clearing price per lease duration bucket #142

Closed
wants to merge 11 commits into from

Conversation

guggero
Copy link
Member

@guggero guggero commented Nov 7, 2020

This PR contains the client part of the change to switch over to distinct lease duration buckets for batches.

UPDATE: Made this non-breaking. Clients with batch version 0 won't be able to submit orders with a lease duration != 2016.

UPDATE2: Depends on #193 which was split off to make the server changes easier to split into multiple PRs.

@guggero guggero requested review from Roasbeef and wpaulino November 7, 2020 10:14
@halseth halseth requested review from halseth and removed request for Roasbeef November 18, 2020 10:54
@guggero guggero force-pushed the duration-buckets branch 2 times, most recently from 2245fbf to 6940df3 Compare November 20, 2020 09:31
@Roasbeef
Copy link
Member

I think this is ready for review now?

@guggero
Copy link
Member Author

guggero commented Nov 24, 2020

Yes, has been for a while.

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

First pass. Main question is whether we can avoid breaking the API.

poolrpc/auctioneer.proto Outdated Show resolved Hide resolved
poolrpc/auctioneer.proto Outdated Show resolved Hide resolved
clientdb/batch_snapshot.go Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
poolrpc/auctioneer.proto Outdated Show resolved Hide resolved
@guggero guggero force-pushed the duration-buckets branch 4 times, most recently from 60fc034 to b830718 Compare November 27, 2020 17:02
@guggero
Copy link
Member Author

guggero commented Nov 27, 2020

I was able to make everything non-breaking!
Still need to do some manual tests to confirm, but I think this PR is ready for a second look.

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Non-breaking changes look good on first pass!

clientdb/batch_snapshot.go Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Great work on this PR! The latest version reads well to me.

Also needs a rebase!

poolrpc/trader.proto Outdated Show resolved Hide resolved
poolrpc/auctioneer.proto Show resolved Hide resolved
order/rpc_parse.go Show resolved Hide resolved
clientdb/batch_snapshot.go Outdated Show resolved Hide resolved
@guggero guggero force-pushed the duration-buckets branch 3 times, most recently from 812fc13 to 37793f9 Compare December 7, 2020 15:57
@guggero guggero requested review from wpaulino and halseth December 7, 2020 16:02
clientdb/batch_snapshot.go Show resolved Hide resolved
clientdb/batch_snapshot_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Awesome, this turned out very clean in the end 👌

Only nit from me is that I think we should make deprecated fields more clear by removing all but the deprecated message from their doc.

poolrpc/auctioneer.proto Show resolved Hide resolved
poolrpc/auctioneer.proto Show resolved Hide resolved
poolrpc/auctioneer.proto Outdated Show resolved Hide resolved
poolrpc/auctioneer.proto Outdated Show resolved Hide resolved
clientdb/batch_snapshot.go Show resolved Hide resolved
@guggero
Copy link
Member Author

guggero commented Dec 9, 2020

Rebased and addressed all comments.

Even though this is ready to be merged now, I think we should wait until the server part is close to being merged as well.

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Re-ACK ✅

order/rpc_parse.go Outdated Show resolved Hide resolved
clientdb/batch_snapshot.go Show resolved Hide resolved
@guggero guggero force-pushed the duration-buckets branch 2 times, most recently from b626b3d to 1ca3245 Compare December 22, 2020 09:37
Because the batch Versions are in the order package, the use of
order.DefaultVersion is confusing and not clear to be related to the
version of the _batch_, just from its name. We fix that by renaming the
versions of the batch.
Even though there still only is one batch version, we want to properly
encode/decode it as a preparation for future version changes.
Because the order/lease duration is now governed by the lease durations,
the maximum order duration is no longer needed.
We add a new order version that is allowed to submit lease durations
outside of the default/legacy 2016 block duration.
No new fields are added, so nothing changes in the serialization or the
digest. This version only signals that the client is able to process
batch requests with multiple duration buckets.
@guggero
Copy link
Member Author

guggero commented Jan 12, 2021

Replaced by #199.

@guggero guggero closed this Jan 12, 2021
@guggero guggero deleted the duration-buckets branch January 12, 2021 18:47
Roasbeef added a commit that referenced this pull request Jan 14, 2021
Fix lnd v0.12.0-beta compatibility, combine #142 and #185
positiveblue pushed a commit to positiveblue/pool that referenced this pull request Oct 11, 2022
…tweak

monitoring/dashboards: don't specify an explicit datasource
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.

4 participants