Add user_channel_id to accept_inbound_channel method
#1381
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1381 +/- ##
==========================================
- Coverage 90.81% 90.79% -0.02%
==========================================
Files 73 73
Lines 41209 41211 +2
Branches 41209 41211 +2
==========================================
- Hits 37424 37419 -5
- Misses 3785 3792 +7
Continue to review full report at Codecov.
|
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Would it be trivial to add a test for this?
LGTM though after squash.
shamardy
force-pushed
the
2022-03-set-inbound-user-chan-id
branch
from
c156eea to
a385b67
Compare
It's not needed in my opinion but can add a test if you think otherwise. |
lightning/src/ln/channelmanager.rs
Outdated
| /// | ||
| /// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest | ||
| /// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id | ||
| pub fn accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], user_channel_id: Option<u64>) -> Result<(), APIError> { |
There was a problem hiding this comment.
I don't see a reason to set this to an Option, the alternative is user_id is always 0 so there's no difference between None and 0.
lightning/src/ln/channelmanager.rs
Outdated
| /// For inbound channels, the `user_channel_id` parameter will be provided back in | ||
| /// [`Event::ChannelClosed::user_channel_id`] to allow tracking of which events correspond | ||
| /// with which `accept_inbound_channel` call. | ||
| /// |
There was a problem hiding this comment.
nit: here and in events.rs you have EOL whitespace. git show locally should highlight them for you so its easy to remove them.
lightning/src/ln/functional_tests.rs
Outdated
| match events[0] { | ||
| Event::OpenChannelRequest { temporary_channel_id, .. } => { | ||
| nodes[1].node.accept_inbound_channel(&temporary_channel_id).unwrap(); | ||
| nodes[1].node.accept_inbound_channel(&temporary_channel_id, None).unwrap(); |
There was a problem hiding this comment.
Should be trivial to just set a value here and test that you get it back if the channel is closed, no? Given its trivial might as well add a test to ensure we don't regress...you never know :p
fix docs edit user_channel_id docs for Event::ChannelClosed review fixes
shamardy
force-pushed
the
2022-03-set-inbound-user-chan-id
branch
from
a385b67 to
edd4bab
Compare
| channel_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel { | ||
| node_id: counterparty_node_id.clone(), | ||
| msg: channel.accept_inbound_channel(), | ||
| msg: channel.accept_inbound_channel(0), |
There was a problem hiding this comment.
Oops, this now overwrites the user id for outbound channels - can either make the channel.rs function an Option again or re-set it by fetching the channel's ID here.
There was a problem hiding this comment.
It's not clear to me how this can happen since accept_inbound_channel doesn't allow outbound channels modifications
rust-lightning/lightning/src/ln/channel.rs
Lines 4748 to 4751 in edd4bab
But I guess it's better to use get_user_id anyways.
There was a problem hiding this comment.
Oh! I'm sorry, yes. So this comment is in internal_open_channel, not accept_inbound_channel, but I clearly wasn't thinking, that is also for inbound channels so no problem.
4 participants
This PR allows the user to track inbound channels closures by
user_channel_idifuser_channel_idis added when processing anOpenChannelRequestevent.