sim_network: Add unit tests + squash bugs #175
Conversation
sim-lib/src/sim_node.rs
Outdated
| } | ||
|
|
||
| impl<'a> DispatchPaymentTestKit<'a> { | ||
| /// Creates a test graph with a set of nodes connected by channels three channels, with all the capacity of |
There was a problem hiding this comment.
nit: "connected by three channels"
sim-lib/src/sim_node.rs
Outdated
| // Test insufficient fee. | ||
| let htlc_amount = 1000; | ||
| let htlc_fee = (channel_state.policy.base_fee as f64 | ||
| + ((channel_state.policy.fee_rate_prop as f64 * htlc_amount as f64) / 1000000.0)) |
There was a problem hiding this comment.
Would extracting this into a constant improve readability? I'd wondered why the division with 1,000,000.0 until I consulted the lightning textbook to review how the fee rate is calculated.
There was a problem hiding this comment.
Pretty standard to have this peppered around LN fee calculations :') changed to 1e6 to make a little more readable, but leaving as-is for now.
There was a problem hiding this comment.
@carlaKC I think you've done a really good job with the tests. I have found them very helpful in navigating sim_node and better understanding aspects of the code I'd cursorily reviewed before.
I have a few comments/questions on flakiness, decimal precision, multiple assertions, and readability.
We handle balances in settle_htlc, so do not need to shift local balance in remove_outgoing_htlc.
With the previous code, our removal loop would not execute for a resolution index of zero. To fix this, we make the loop inclusive and provide the appropriate index for successful HTLCs (failing HTLCs already use the correct failure index).
To allow more isolated testing of channel state, move balance updates to function on the individual sides of the channel.
|
Thanks for the review @enigbe! Updated to address most feedback, left some of the test setup as-is for the sake of simplicity (can always come back and improve as is required). |
This PR adds coverage for our simulated network and (unsurprisingly) squashes some bugs:
=0.Removal bugs not caught in original testing because we evenly distribute liquidity, so didn't run into any payment failure - going to set liquidity to zero and re-run for a while to see if anything falls apart.