simln-lib/feat: do not allow defined activity with zero amount #218
Conversation
|
@carlaKC what do you think about my approach to creating the test? |
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
approach looks good, just some minor comments to cut down the test a little
simln-lib/src/lib.rs
Outdated
| ))); | ||
| } | ||
|
|
||
| if payment_flow.amount_msat.value() == 0 { |
There was a problem hiding this comment.
If you'd like to cut down the size of the test a bit, happy to move this to the top of the loop so that you don't have to expect the get_info calls in the test?
simln-lib/src/lib.rs
Outdated
| ); | ||
| clients.insert( | ||
| node_2.pubkey, | ||
| std::sync::Arc::new(tokio::sync::Mutex::new(mock_node_2)), |
There was a problem hiding this comment.
nit: can import these in use statements
f3r10
force-pushed
the
do_not_allow_defined_activity_with_zero_amount
branch
from
eac7c6c to
2be6ae5
Compare
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
just some last nitpicks on the test, non-blocking if you'd like to merge as-is!
| .clone(); | ||
| node_1.features.set_keysend_optional(); | ||
| let mut node_2 = test_utils::create_nodes(1, 0).first().unwrap().0.clone(); | ||
| node_2.features.set_keysend_optional(); |
There was a problem hiding this comment.
Rather than setting keysend here, let's update create_nodes to set this feature bit? We just didn't need it in the past, which is why it's an empty vector.
There was a problem hiding this comment.
ok, maybe it would be better to open another PR doing this? because several places call create_nodes that are not related with this PR. WDYT?
There was a problem hiding this comment.
I don't think it'll break any existing tests, so I think we can go ahead in this PR? I was thinking just set it inside of create_nodes so that fn signature doesn't change so it would be a small change.
But if I'm wrong (always the most likely outcome 😅 ) and it breaks a bunch of stuff then yes let's do a followup!
simln-lib/src/lib.rs
Outdated
|
|
||
| #[tokio::test] | ||
| async fn test_validate_zero_amount_no_valid() { | ||
| let mut node_1 = test_utils::create_nodes(1, 100_000) |
There was a problem hiding this comment.
nit: create two nodes at once using create_nodes(2, 100_000)?
simln-lib/src/lib.rs
Outdated
| let mock_node_1 = MockLightningNode::new(); | ||
| let mock_node_2 = MockLightningNode::new(); | ||
| let mut clients: HashMap< | ||
| bitcoin::secp256k1::PublicKey, |
There was a problem hiding this comment.
nit: import with use, same for SimulationConfig below - generally have always been importing like this in the project rather than inlining
f3r10
force-pushed
the
do_not_allow_defined_activity_with_zero_amount
branch
2 times, most recently
from
2492a2e to
4883019
Compare
f3r10
force-pushed
the
do_not_allow_defined_activity_with_zero_amount
branch
from
4883019 to
bb4b7e4
Compare
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
tACK bb4b7e4!
Going to go ahead and merge this so that we don't run into conflicts with any of the incoming PRs. I would like to move keysend into test utils, but we can do that in another PR.
2 participants
fix #176