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: Rollover DLC #1122

Merged
merged 3 commits into from
Aug 22, 2023
Merged

feat: Rollover DLC #1122

merged 3 commits into from
Aug 22, 2023

Conversation

holzeis
Copy link
Contributor

@holzeis holzeis commented Aug 18, 2023

This introduces the rollover of a signed dlc. It uses the renew offer api of the rust dlc.

  • The payout amount is calculated from the original average entry price - as we are rolling over a position nothing should change. However, this is most likely the place where we would like to add paying the funding rates for the rollover.

  • The state Rollover is introduced to the position on the coordinator and the app side. It helps to show a rollover in progress as with the dlc creation a lot of messages are exchanged in between. We also need that state to prevent the coordinator from accidentally setting the position to Closed as the underlying contract is Closed after the new one is set. Therefore the temporary_contract_id is also updated.

I left a some todos regarding the dlc messages. It would be nice if we could process them similar to the lightning messages through the or a event handler. That would allow for the design to be nicer decoupled.

resolves #1068

@holzeis holzeis requested review from bonomat and luckysori August 18, 2023 15:18
@holzeis holzeis self-assigned this Aug 18, 2023
@holzeis holzeis force-pushed the feat/rollover-dlc branch 3 times, most recently from a4268f4 to 8f30240 Compare August 18, 2023 17:54
crates/ln-dlc-node/src/ln/dlc_channel_details.rs Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/ln/dlc_channel_details.rs Outdated Show resolved Hide resolved
coordinator/src/lib.rs Outdated Show resolved Hide resolved
coordinator/src/node.rs Outdated Show resolved Hide resolved
coordinator/src/rollover.rs Outdated Show resolved Hide resolved
coordinator/src/rollover.rs Show resolved Hide resolved
)
}

/// Finalizes the rollover protocol with the app setting the position to open.
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 This talks about the app but this is coordinator code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well the coordinator talks with the app to finalize the rollover protocol. what would you want to change here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you were meaning to say that the coordinator would set the position to open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does set it from Rollover to Open 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I see why I was confused now. I read

Finalizes the rollover protocol**,** with the app setting the position to open

which is weird because this is coordinator code. But you meant

Finalizes the rollover protocol with the app**,** setting the position to open

which makes sense. I think the comma would help, but it's not important.

coordinator/src/routes.rs Show resolved Hide resolved
crates/tests-e2e/tests/rollover_position.rs Show resolved Hide resolved
mobile/native/src/ln_dlc/node.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

LGTM!

IMHO this is a bit cleaner as the api is simplified if we just call the calculate_settlement_amount on the position.
It appears the channel_id is not always correct. Using the `get_dlc_channel_id(0)` seems to cover more edge cases.
This introduces the rollover of a signed dlc. It uses the renew offer api of the rust dlc. A couple of design choices.

- The payout amount is calculated from the original average entry price - as we are rolling over a position nothing should change. However, this is most likely the place where we would like to add paying the funding rates for the rollover.
- The state `Rollover` is introduced to the position on the coordinator and the app side. It helps to show a rollover in progress as with the dlc creation a lot of messages are exchanged in between. We also need that state to prevent the coordinator from accidentally setting the position to `Closed` as the underlying contract is Closed after the new one is set. Therefore the `temporary_contract_id` is also updated.

I left a some todos regarding the dlc messages. It would be nice if we could process them similar to the lightning messages through the or a event handler. That would allow for the design to be nicer decoupled.
@holzeis holzeis force-pushed the feat/rollover-dlc branch from 92ca89f to 6b20ca3 Compare August 21, 2023 12:07
@holzeis holzeis added this pull request to the merge queue Aug 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 21, 2023
@holzeis holzeis added this pull request to the merge queue Aug 22, 2023
Merged via the queue into main with commit ba9e230 Aug 22, 2023
@holzeis holzeis deleted the feat/rollover-dlc branch August 22, 2023 04:53
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.

Rollover DLC
3 participants