Fix protocol of channel renew and sub channel establish #116
Conversation
Tibo-lg
force-pushed
the
fix/channel-renew-ln-dlc-establish
branch
2 times, most recently
from
a127efd
to
ae4d228
Compare
| party_points: dlc_channel.counter_points, | ||
| per_update_point: dlc_channel.counter_per_update_point, |
There was a problem hiding this comment.
How come these have gone from own to counter?
There was a problem hiding this comment.
Because the code used to be for the offer party side, but now it's in the accept party side.
There was a problem hiding this comment.
This is slightly confusing still. I have two questions:
- Are we interpreting these fields differently depending on the value of
is_offer_party? For example, ifis_offer_party == false, do we takeparty_pointsto mean the offerer's points which are therefore the counterparty's? - What does the
counter_partyfield below mean if these fields depend onis_offer_party. It looks like we also need to flip that to be consistent with this change.
It might help to take an absolute perspective in this message, with fields belonging to "sender" and "receiver" rather than "ours" and "theirs". The is_offer_party field would remain inherently relative as constructed, although we could instead say pubkey_offer_party so that each side can compare against their own pubkey. Not sure if it's worth it though.
There was a problem hiding this comment.
Are we interpreting these fields differently depending on the value of is_offer_party? For example, if is_offer_party == false, do we take party_points to mean the offerer's points which are therefore the counterparty's?
Yes
What does the counter_party field below mean if these fields depend on is_offer_party. It looks like we also need to flip that to be consistent with this change.
The counter_party field is actually just the counter party pubkey, regardless of the is_offer_party value.
It might help to take an absolute perspective in this message, with fields belonging to "sender" and "receiver" rather than "ours" and "theirs".
So pretty much all fields are the ones of the "sender", there is really nothing about the "receiver".
The is_offer_party field would remain inherently relative as constructed, although we could instead say pubkey_offer_party so that each side can compare against their own pubkey. Not sure if it's worth it though.
If you find it clearer or easier I'm ok with such a refactor, but maybe not as part of this PR :). I agree it might prevent some errors (where the field is set incorrectly).
There was a problem hiding this comment.
Thanks for clarifying!
If you find it clearer or easier I'm ok with such a refactor, but maybe not as part of this PR :). I agree it might prevent some errors (where the field is set incorrectly).
I agree that we shouldn't do it with this PR. I might give it a go in the future 😄
Tibo-lg
force-pushed
the
fix/channel-renew-ln-dlc-establish
branch
from
ae4d228
to
634d8fd
Compare
| party_points: dlc_channel.counter_points, | ||
| per_update_point: dlc_channel.counter_per_update_point, |
There was a problem hiding this comment.
This is slightly confusing still. I have two questions:
- Are we interpreting these fields differently depending on the value of
is_offer_party? For example, ifis_offer_party == false, do we takeparty_pointsto mean the offerer's points which are therefore the counterparty's? - What does the
counter_partyfield below mean if these fields depend onis_offer_party. It looks like we also need to flip that to be consistent with this change.
It might help to take an absolute perspective in this message, with fields belonging to "sender" and "receiver" rather than "ours" and "theirs". The is_offer_party field would remain inherently relative as constructed, although we could instead say pubkey_offer_party so that each side can compare against their own pubkey. Not sure if it's worth it though.
Tibo-lg
force-pushed
the
fix/channel-renew-ln-dlc-establish
branch
from
634d8fd
to
00b952e
Compare
There was a problem hiding this comment.
If you address/dismiss https://github.com/p2pderivatives/rust-dlc/pull/116/files#r1257590214 I have no further comments. LGTM!
Tibo-lg
force-pushed
the
fix/channel-renew-ln-dlc-establish
branch
from
00b952e
to
7560cad
Compare
This PR fixes the protocols of the channel renewal and sub channel establishment. The issue was that with the previous protocol the offering party had the ability to force close the channel without the accepting party being able to finalize the closing, which meant the offering party could chose whether to close the DLC channel using a CET or wait for a refund.