Validate zero amount in a defined activity #180
Conversation
sim-lib/src/lib.rs
Outdated
| // lets validate the amount_msats to be greater than zero | ||
| if payment_flow.amount_msat==0 { | ||
| return Err(LightningError::ValidationError( | ||
| "Expected amount_msat should be greater than zero.".to_string(), |
There was a problem hiding this comment.
I think the error message here can be more descriptive, because some users may find this hard to understand at first.
| "Expected amount_msat should be greater than zero.".to_string(), | |
| ""We do not allow defined activity amount_msat with zero values".to_string(), |
sim-lib/src/lib.rs
Outdated
| @@ -475,6 +475,12 @@ impl Simulation { | |||
| } | |||
|
|
|||
| for payment_flow in self.activity.iter() { | |||
| // lets validate the amount_msats to be greater than zero | |||
| if payment_flow.amount_msat==0 { | |||
There was a problem hiding this comment.
This needs proper formatting. The contributing guide recommends running a make check command to check for lint errors and adhering to the linter suggestions made by clippy.
sim-lib/src/lib.rs
Outdated
| @@ -475,6 +475,12 @@ impl Simulation { | |||
| } | |||
|
|
|||
| for payment_flow in self.activity.iter() { | |||
| // lets validate the amount_msats to be greater than zero | |||
| if payment_flow.amount_msat==0 { | |||
There was a problem hiding this comment.
nit: I think having the zero amount validation after the source and destination nodes validation makes more sense because the payment amount_msat is coming from a valid source node to a valid destination node.
There was a problem hiding this comment.
@Anyitechs. I see. This makes sense
There was a problem hiding this comment.
Agree with @Anyitechs's review here. It would be very nice to include some test coverage for validate_activity with this PR as well. We've been quite shocking with tests so far in the project's existence, but that's certainly something we want to change.
… and the destination node validation
|
Hello @carlaKC we were having a chat with @Anyitechs on the where to place test files. The current implementation has tests written on each file , for example defined_activity.rs has tests written on it. |
We're following the rust convention of including tests in the same file in a different module - I don't really see a compelling reason to move them into another file? |
|
Closing due to inactivity, please feel free to re-open if you've got time to pick this up again. |
|
Hi @carlaKC Been working on it on the side . Been trying to work on mocking a Simulation as per your instructions. So its work in progress |
This PR addresses the issue #176 . This has been done by updating the validate_activity function