Rename Event PaymentFailed -> PaymentPathFailed #1084
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1084 +/- ##
==========================================
- Coverage 90.72% 90.72% -0.01%
==========================================
Files 65 65
Lines 34212 34225 +13
==========================================
+ Hits 31039 31049 +10
- Misses 3173 3176 +3
Continue to review full report at Codecov.
|
| /// The payment path that failed. | ||
| path: Vec<RouteHop>, |
There was a problem hiding this comment.
So this will be empty for backwards compatibility rather than None? I think that's probably ok but just want to make sure.
There was a problem hiding this comment.
True. Is that an issue for the scorer? It could be an option
There was a problem hiding this comment.
Yes, vec_type is an option, basically. I agree I think its fine, but should be called out in release notes.
There was a problem hiding this comment.
True. Is that an issue for the scorer? It could be an
option
Nah, would be better if neither event used Option for symmetry.
Yes,
vec_typeis an option, basically. I agree I think its fine, but should be called out in release notes.
Do we tag PRs that need to be reflected in the release? Or should we update as we go?
There was a problem hiding this comment.
We currently don't have a process for release notes, I generally write the compat section by reading the full-release diff and looking at serialization sections to see the implications of stuff when we cut the release. In the future, we should move towards including release note updates in the PR itself, maybe in a subfolder that just has a long list of release note files. Either way, doesn't have to happen here.
|
Will take after #997. |
valentinewallace
force-pushed
the
2021-09-rename-paymentfailed
branch
2 times, most recently
from
b6fc74d to
0302b7d
Compare
|
Rebased! |
There was a problem hiding this comment.
Will land after CI, range-diff is:
$ git range-diff 801d6e5256d6ac91d5d5668da1fa5a2b55303246..af73857278bd3d94e86ccc059fedde85697aea82 5262e77ae186e4ea58294a7c6ee05e5649a12fc5..e5310dd5f0347fcaaf21b2742a1084f97fc84bd8
1: 8ef997c8 ! 1: bf16dfd1 Rename PaymentFailed -> PaymentPathFailed
@@ lightning/src/ln/chanmon_update_fail_tests.rs: fn do_test_simple_monitor_permane
+ // PaymentPathFailed event
assert_eq!(nodes[0].node.list_channels().len(), 0);
- }
+ check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: "ChannelMonitor storage failure".to_string() });
@@ lightning/src/ln/chanmon_update_fail_tests.rs: fn do_test_simple_monitor_temporary_update_fail(disconnect: bool, persister_fail
check_closed_broadcast!(nodes[0], true);
@@ lightning/src/ln/chanmon_update_fail_tests.rs: fn do_test_simple_monitor_tempora
+ // PaymentPathFailed event
assert_eq!(nodes[0].node.list_channels().len(), 0);
- }
+ check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed);
@@ lightning/src/ln/chanmon_update_fail_tests.rs: fn test_monitor_update_on_pending_forwards() {
let events = nodes[0].node.get_and_clear_pending_events();
@@ lightning/src/ln/functional_test_utils.rs: pub fn fail_payment_along_route<'a, '
## lightning/src/ln/functional_tests.rs ##
@@ lightning/src/ln/functional_tests.rs: fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
- let events = nodes[1].node.get_and_clear_pending_events();
- assert_eq!(events.len(), if deliver_bs_raa { 1 } else { 2 });
- match events[0] {
+ _ => panic!("Unexepected event"),
+ }
+ match events[1] {
- Event::PaymentFailed { ref payment_hash, .. } => {
+ Event::PaymentPathFailed { ref payment_hash, .. } => {
assert_eq!(*payment_hash, fourth_payment_hash);
@@ lightning/src/ln/functional_tests.rs: fn do_test_commitment_revoked_fail_backwar
assert!(failed_htlcs.insert(payment_hash.0));
assert!(network_update.is_some());
},
+@@ lightning/src/ln/functional_tests.rs: fn fail_backward_pending_htlc_upon_channel_failure() {
+ assert_eq!(events.len(), 2);
+ // Check that Alice fails backward the pending HTLC from the second payment.
+ match events[0] {
+- Event::PaymentFailed { payment_hash, .. } => {
++ Event::PaymentPathFailed { payment_hash, .. } => {
+ assert_eq!(payment_hash, failed_payment_hash);
+ },
+ _ => panic!("Unexpected event"),
@@ lightning/src/ln/functional_tests.rs: fn test_simple_peer_disconnect() {
_ => panic!("Unexpected event"),
}
2: af738572 ! 2: e5310dd5 Add path field to PaymentPathFailed event
@@ lightning/src/routing/router.rs: use core::cmp;
pub pubkey: PublicKey,
## lightning/src/util/events.rs ##
-@@ lightning/src/util/events.rs: use chain::keysinterface::SpendableOutputDescriptor;
- use ln::msgs;
+@@ lightning/src/util/events.rs: use ln::msgs::DecodeError;
use ln::{PaymentPreimage, PaymentHash, PaymentSecret};
use routing::network_graph::NetworkUpdate;
+ use util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, VecReadWrapper, VecWriteWrapper};
+use routing::router::RouteHop;
- use util::ser::{Writeable, Writer, MaybeReadable, Readable, VecReadWrapper, VecWriteWrapper};
use bitcoin::blockdata::script::Script;
+
@@ lightning/src/util/events.rs: pub enum Event {
/// failed. This will be set to false if (1) this is an MPP payment and (2) other parts of the
/// larger MPP payment were still in flight when this event was generated.
Since we don't want to imply to users that a payment has completely failed when it really has just partially failed
valentinewallace
force-pushed
the
2021-09-rename-paymentfailed
branch
from
0302b7d to
e5310dd
Compare
And add
pathfield toPaymentPathFailed.Closes #1081