Add an override optional UserConfig per new outbound channel #517
Conversation
lightning/src/ln/functional_tests.rs
Outdated
|
|
||
| // Node0 initiates a channel to node1 using the override config. | ||
| let mut override_config = UserConfig::default(); | ||
| override_config.own_channel_config.our_to_self_delay = 200; |
There was a problem hiding this comment.
It looks like the existing testing for bad UserConfig values is pretty light -- this test could be a good opportunity to add some more :)
lightning/src/ln/channelmanager.rs
Outdated
| if channel_value_satoshis < 1000 { | ||
| return Err(APIError::APIMisuseError { err: "channel_value must be at least 1000 satoshis" }); | ||
| } | ||
|
|
||
| let channel = Channel::new_outbound(&*self.fee_estimator, &self.keys_manager, their_network_key, channel_value_satoshis, push_msat, user_id, Arc::clone(&self.logger), &self.default_configuration)?; | ||
| let channel = match override_config { |
There was a problem hiding this comment.
You may try
if override_config.is_some() { override_config.as_ref().unwrap() } else { &self.default_configuration }
There was a problem hiding this comment.
So in regards to the git jobs, I've squashed and dropped the fixup. My bad, looking back at the contribution guidelines I missed this:
Further, each commit, individually, should compile and pass tests, in order to ensure git bisect and other automated tools function properly.
There was a problem hiding this comment.
@ariard's suggested change here would probably make this more readable. Also, it looks like you replaced all the indentation here with spaces. Please don't mix indentation types in a file.
There was a problem hiding this comment.
thanks and appreciate the review @TheBlueMatt, I'll start work on the requested changes
|
thank for the review @valentinewallace and @ariard, I'll get started on the comments :) |
ccdle12
force-pushed
the
2020-02-per-channel-config
branch
from
b4e7ec6 to
fe154c8
Compare
lightning/src/ln/channelmanager.rs
Outdated
| if channel_value_satoshis < 1000 { | ||
| return Err(APIError::APIMisuseError { err: "channel_value must be at least 1000 satoshis" }); | ||
| } | ||
|
|
||
| let channel = Channel::new_outbound(&*self.fee_estimator, &self.keys_manager, their_network_key, channel_value_satoshis, push_msat, user_id, Arc::clone(&self.logger), &self.default_configuration)?; | ||
| let channel = match override_config { |
There was a problem hiding this comment.
@ariard's suggested change here would probably make this more readable. Also, it looks like you replaced all the indentation here with spaces. Please don't mix indentation types in a file.
lightning/src/ln/functional_tests.rs
Outdated
|
|
||
| #[test] | ||
| fn test_override_channel_config() { | ||
| let chanmon_cfgs = create_chanmon_cfgs(2); |
There was a problem hiding this comment.
nit: Same here, please use indentation characters instead of spaces.
There was a problem hiding this comment.
fixed both, perhaps better to work on the requested updates to the tests before squashing? - might be easier for everyone to match the review requested per commit
|
Looks good. Needs rebase but otherwise probably fine to go. I dont know that this needs more tests for the new functionality, only that our general config infrastructure likely does. |
ccdle12
force-pushed
the
2020-02-per-channel-config
branch
from
0a08644 to
849c514
Compare
ccdle12
force-pushed
the
2020-02-per-channel-config
branch
from
849c514 to
53c894b
Compare
|
Ok sounds good, maybe I could help review a PR or look into further testing on the config infrastructure. Squashed and rebased. |
fixes #485
This PR adds an optional
UserConfigparameter when callingcreate_channel()inChannelManager.The idea is that if this option is None,
ChannelManagerwill revert to thedefault_configurationfound in its own fields or the user can provide their own configuration for a particular channel.Also to add that in the original issue, there was some discussion about implementing a tracked peer-per channel config as in a "whitelist" of pubkeys. It seems like an interesting idea, but I thought that would deserve its own attempt at a PR - since I think we would still need to switch between a
defaultand anoverrideconfig per channel.