[LSPS5] Change notification cooldown time to 1 minute, also update docs by martinsaposnic · Pull Request #3994 · lightningdevkit/rust-lightning

martinsaposnic · 2025-08-07T14:30:11Z

LSPS5 follow ups. Addressing comments #3975 (review) and #3975 (comment)

changes:

notification_cooldown is not a config so it does not make sense to call the constant DEFAULT.
notification_cooldown it's not measured in hours, so changed the name, and also changed the constant from 1 hour to 1 minute (reasons explained in this comment LSPS5: Correct notification cooldown & reset logic #3975 (comment)

ldk-reviews-bot · 2025-08-07T14:30:15Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-07T15:20:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.91%. Comparing base (6d47f0a) to head (c7694f4).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3994 +/- ##
==========================================
+ Coverage 88.74% 88.91% +0.17% 
==========================================
Files 173 174 +1 Lines 124899 125114 +215 Branches 124899 125114 +215 ==========================================
+ Hits 110841 111249 +408 + Misses 11631 11354 -277 - Partials 2427 2511 +84

Flag	Coverage Δ
fuzzing	`22.12% <0.00%> (?)`
tests	`88.74% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

This needs a rebase now.

martinsaposnic · 2025-08-07T17:39:53Z

rebase done!

tnull

As mentioned elsewhere, we might need a spec change in parallel to this.

Also, nit: 'LSP5', 'dooldown' and 'alsp' typos in the commit message (which could otherwise also use a bit more detail what this change is and why we want it).

ldk-reviews-bot · 2025-08-08T07:01:20Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

martinsaposnic · 2025-08-11T16:33:52Z

we might need a spec change in parallel to this

lightning/blips#64 spec update PR

tnull

Thanks! Changes look good to me, but please fixup the typos in the commit message, as mentioned above.

tnull · 2025-08-12T06:46:50Z

 .get(&notification.method)
 .map(|last_sent| now.duration_since(&last_sent))
- .map_or(false, |duration| duration < DEFAULT_NOTIFICATION_COOLDOWN_HOURS)
+ .map_or(false, |duration: Duration| duration < NOTIFICATION_COOLDOWN_TIME)


Why do we need this : Duration suddenly?

FYI force-pushed this small change without a fixup https://github.com/lightningdevkit/rust-lightning/compare/d638113b68b2de0ec02dd5d2a7574f7854d5005c..8b326536eed3eaff61d4c261dd5f83d96620f76b

martinsaposnic · 2025-08-12T19:57:53Z

also fixed the commit message. sorry about the typos!

tnull

Thanks, LGTM.

Simple enough, so landing this.

(the commit message could have still used some motiviation, etc ;P)

martinsaposnic · 2025-08-07T14:30:11Z

LSPS5 follow ups. Addressing comments #3975 (review) and #3975 (comment)

changes:

notification_cooldown is not a config so it does not make sense to call the constant DEFAULT.
notification_cooldown it's not measured in hours, so changed the name, and also changed the constant from 1 hour to 1 minute (reasons explained in this comment LSPS5: Correct notification cooldown & reset logic #3975 (comment)

ldk-reviews-bot · 2025-08-07T14:30:15Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-07T15:20:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.91%. Comparing base (6d47f0a) to head (c7694f4).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3994 +/- ##
==========================================
+ Coverage 88.74% 88.91% +0.17% 
==========================================
Files 173 174 +1 Lines 124899 125114 +215 Branches 124899 125114 +215 ==========================================
+ Hits 110841 111249 +408 + Misses 11631 11354 -277 - Partials 2427 2511 +84

Flag	Coverage Δ
fuzzing	`22.12% <0.00%> (?)`
tests	`88.74% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

This needs a rebase now.

martinsaposnic · 2025-08-07T17:39:53Z

rebase done!

tnull

As mentioned elsewhere, we might need a spec change in parallel to this.

Also, nit: 'LSP5', 'dooldown' and 'alsp' typos in the commit message (which could otherwise also use a bit more detail what this change is and why we want it).

ldk-reviews-bot · 2025-08-08T07:01:20Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

martinsaposnic · 2025-08-11T16:33:52Z

we might need a spec change in parallel to this

lightning/blips#64 spec update PR

tnull

Thanks! Changes look good to me, but please fixup the typos in the commit message, as mentioned above.

tnull · 2025-08-12T06:46:50Z

 .get(&notification.method)
 .map(|last_sent| now.duration_since(&last_sent))
- .map_or(false, |duration| duration < DEFAULT_NOTIFICATION_COOLDOWN_HOURS)
+ .map_or(false, |duration: Duration| duration < NOTIFICATION_COOLDOWN_TIME)


Why do we need this : Duration suddenly?

FYI force-pushed this small change without a fixup https://github.com/lightningdevkit/rust-lightning/compare/d638113b68b2de0ec02dd5d2a7574f7854d5005c..8b326536eed3eaff61d4c261dd5f83d96620f76b

martinsaposnic · 2025-08-12T19:57:53Z

also fixed the commit message. sorry about the typos!

tnull

Thanks, LGTM.

Simple enough, so landing this.

(the commit message could have still used some motiviation, etc ;P)

martinsaposnic · 2025-08-07T14:30:11Z

LSPS5 follow ups. Addressing comments #3975 (review) and #3975 (comment)

changes:

notification_cooldown is not a config so it does not make sense to call the constant DEFAULT.
notification_cooldown it's not measured in hours, so changed the name, and also changed the constant from 1 hour to 1 minute (reasons explained in this comment LSPS5: Correct notification cooldown & reset logic #3975 (comment)

ldk-reviews-bot · 2025-08-07T14:30:15Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-07T15:20:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.91%. Comparing base (6d47f0a) to head (c7694f4).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3994 +/- ##
==========================================
+ Coverage 88.74% 88.91% +0.17% 
==========================================
Files 173 174 +1 Lines 124899 125114 +215 Branches 124899 125114 +215 ==========================================
+ Hits 110841 111249 +408 + Misses 11631 11354 -277 - Partials 2427 2511 +84

Flag	Coverage Δ
fuzzing	`22.12% <0.00%> (?)`
tests	`88.74% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

This needs a rebase now.

martinsaposnic · 2025-08-07T17:39:53Z

rebase done!

tnull

As mentioned elsewhere, we might need a spec change in parallel to this.

Also, nit: 'LSP5', 'dooldown' and 'alsp' typos in the commit message (which could otherwise also use a bit more detail what this change is and why we want it).

ldk-reviews-bot · 2025-08-08T07:01:20Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

martinsaposnic · 2025-08-11T16:33:52Z

we might need a spec change in parallel to this

lightning/blips#64 spec update PR

tnull

Thanks! Changes look good to me, but please fixup the typos in the commit message, as mentioned above.

tnull · 2025-08-12T06:46:50Z

 .get(&notification.method)
 .map(|last_sent| now.duration_since(&last_sent))
- .map_or(false, |duration| duration < DEFAULT_NOTIFICATION_COOLDOWN_HOURS)
+ .map_or(false, |duration: Duration| duration < NOTIFICATION_COOLDOWN_TIME)


Why do we need this : Duration suddenly?

FYI force-pushed this small change without a fixup https://github.com/lightningdevkit/rust-lightning/compare/d638113b68b2de0ec02dd5d2a7574f7854d5005c..8b326536eed3eaff61d4c261dd5f83d96620f76b

martinsaposnic · 2025-08-12T19:57:53Z

also fixed the commit message. sorry about the typos!

tnull

Thanks, LGTM.

Simple enough, so landing this.

(the commit message could have still used some motiviation, etc ;P)

martinsaposnic · 2025-08-07T14:30:11Z

LSPS5 follow ups. Addressing comments #3975 (review) and #3975 (comment)

changes:

notification_cooldown is not a config so it does not make sense to call the constant DEFAULT.
notification_cooldown it's not measured in hours, so changed the name, and also changed the constant from 1 hour to 1 minute (reasons explained in this comment LSPS5: Correct notification cooldown & reset logic #3975 (comment)

ldk-reviews-bot · 2025-08-07T14:30:15Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-07T15:20:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.91%. Comparing base (6d47f0a) to head (c7694f4).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3994 +/- ##
==========================================
+ Coverage 88.74% 88.91% +0.17% 
==========================================
Files 173 174 +1 Lines 124899 125114 +215 Branches 124899 125114 +215 ==========================================
+ Hits 110841 111249 +408 + Misses 11631 11354 -277 - Partials 2427 2511 +84

Flag	Coverage Δ
fuzzing	`22.12% <0.00%> (?)`
tests	`88.74% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

This needs a rebase now.

martinsaposnic · 2025-08-07T17:39:53Z

rebase done!

tnull

As mentioned elsewhere, we might need a spec change in parallel to this.

Also, nit: 'LSP5', 'dooldown' and 'alsp' typos in the commit message (which could otherwise also use a bit more detail what this change is and why we want it).

ldk-reviews-bot · 2025-08-08T07:01:20Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

martinsaposnic · 2025-08-11T16:33:52Z

we might need a spec change in parallel to this

lightning/blips#64 spec update PR

tnull

Thanks! Changes look good to me, but please fixup the typos in the commit message, as mentioned above.

tnull · 2025-08-12T06:46:50Z

 .get(&notification.method)
 .map(|last_sent| now.duration_since(&last_sent))
- .map_or(false, |duration| duration < DEFAULT_NOTIFICATION_COOLDOWN_HOURS)
+ .map_or(false, |duration: Duration| duration < NOTIFICATION_COOLDOWN_TIME)


Why do we need this : Duration suddenly?

FYI force-pushed this small change without a fixup https://github.com/lightningdevkit/rust-lightning/compare/d638113b68b2de0ec02dd5d2a7574f7854d5005c..8b326536eed3eaff61d4c261dd5f83d96620f76b

martinsaposnic · 2025-08-12T19:57:53Z

also fixed the commit message. sorry about the typos!

tnull

Thanks, LGTM.

Simple enough, so landing this.

(the commit message could have still used some motiviation, etc ;P)

martinsaposnic · 2025-08-07T14:30:11Z

LSPS5 follow ups. Addressing comments #3975 (review) and #3975 (comment)

changes:

notification_cooldown is not a config so it does not make sense to call the constant DEFAULT.
notification_cooldown it's not measured in hours, so changed the name, and also changed the constant from 1 hour to 1 minute (reasons explained in this comment LSPS5: Correct notification cooldown & reset logic #3975 (comment)

ldk-reviews-bot · 2025-08-07T14:30:15Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-07T15:20:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.91%. Comparing base (6d47f0a) to head (c7694f4).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3994 +/- ##
==========================================
+ Coverage 88.74% 88.91% +0.17% 
==========================================
Files 173 174 +1 Lines 124899 125114 +215 Branches 124899 125114 +215 ==========================================
+ Hits 110841 111249 +408 + Misses 11631 11354 -277 - Partials 2427 2511 +84

Flag	Coverage Δ
fuzzing	`22.12% <0.00%> (?)`
tests	`88.74% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

This needs a rebase now.

martinsaposnic · 2025-08-07T17:39:53Z

rebase done!

tnull

As mentioned elsewhere, we might need a spec change in parallel to this.

Also, nit: 'LSP5', 'dooldown' and 'alsp' typos in the commit message (which could otherwise also use a bit more detail what this change is and why we want it).

ldk-reviews-bot · 2025-08-08T07:01:20Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

martinsaposnic · 2025-08-11T16:33:52Z

we might need a spec change in parallel to this

lightning/blips#64 spec update PR

tnull

Thanks! Changes look good to me, but please fixup the typos in the commit message, as mentioned above.

tnull · 2025-08-12T06:46:50Z

 .get(&notification.method)
 .map(|last_sent| now.duration_since(&last_sent))
- .map_or(false, |duration| duration < DEFAULT_NOTIFICATION_COOLDOWN_HOURS)
+ .map_or(false, |duration: Duration| duration < NOTIFICATION_COOLDOWN_TIME)


Why do we need this : Duration suddenly?

FYI force-pushed this small change without a fixup https://github.com/lightningdevkit/rust-lightning/compare/d638113b68b2de0ec02dd5d2a7574f7854d5005c..8b326536eed3eaff61d4c261dd5f83d96620f76b

martinsaposnic · 2025-08-12T19:57:53Z

also fixed the commit message. sorry about the typos!

tnull

Thanks, LGTM.

Simple enough, so landing this.

(the commit message could have still used some motiviation, etc ;P)

martinsaposnic · 2025-08-07T14:30:11Z

LSPS5 follow ups. Addressing comments #3975 (review) and #3975 (comment)

changes:

notification_cooldown is not a config so it does not make sense to call the constant DEFAULT.
notification_cooldown it's not measured in hours, so changed the name, and also changed the constant from 1 hour to 1 minute (reasons explained in this comment LSPS5: Correct notification cooldown & reset logic #3975 (comment)

ldk-reviews-bot · 2025-08-07T14:30:15Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-07T15:20:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.91%. Comparing base (6d47f0a) to head (c7694f4).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3994 +/- ##
==========================================
+ Coverage 88.74% 88.91% +0.17% 
==========================================
Files 173 174 +1 Lines 124899 125114 +215 Branches 124899 125114 +215 ==========================================
+ Hits 110841 111249 +408 + Misses 11631 11354 -277 - Partials 2427 2511 +84

Flag	Coverage Δ
fuzzing	`22.12% <0.00%> (?)`
tests	`88.74% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

This needs a rebase now.

martinsaposnic · 2025-08-07T17:39:53Z

rebase done!

tnull

As mentioned elsewhere, we might need a spec change in parallel to this.

Also, nit: 'LSP5', 'dooldown' and 'alsp' typos in the commit message (which could otherwise also use a bit more detail what this change is and why we want it).

ldk-reviews-bot · 2025-08-08T07:01:20Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

martinsaposnic · 2025-08-11T16:33:52Z

we might need a spec change in parallel to this

lightning/blips#64 spec update PR

tnull

Thanks! Changes look good to me, but please fixup the typos in the commit message, as mentioned above.

tnull · 2025-08-12T06:46:50Z

 .get(&notification.method)
 .map(|last_sent| now.duration_since(&last_sent))
- .map_or(false, |duration| duration < DEFAULT_NOTIFICATION_COOLDOWN_HOURS)
+ .map_or(false, |duration: Duration| duration < NOTIFICATION_COOLDOWN_TIME)


Why do we need this : Duration suddenly?

FYI force-pushed this small change without a fixup https://github.com/lightningdevkit/rust-lightning/compare/d638113b68b2de0ec02dd5d2a7574f7854d5005c..8b326536eed3eaff61d4c261dd5f83d96620f76b

martinsaposnic · 2025-08-12T19:57:53Z

also fixed the commit message. sorry about the typos!

tnull

Thanks, LGTM.

Simple enough, so landing this.

(the commit message could have still used some motiviation, etc ;P)

martinsaposnic · 2025-08-07T14:30:11Z

LSPS5 follow ups. Addressing comments #3975 (review) and #3975 (comment)

changes:

notification_cooldown is not a config so it does not make sense to call the constant DEFAULT.
notification_cooldown it's not measured in hours, so changed the name, and also changed the constant from 1 hour to 1 minute (reasons explained in this comment LSPS5: Correct notification cooldown & reset logic #3975 (comment)

ldk-reviews-bot · 2025-08-07T14:30:15Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-07T15:20:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.91%. Comparing base (6d47f0a) to head (c7694f4).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3994 +/- ##
==========================================
+ Coverage 88.74% 88.91% +0.17% 
==========================================
Files 173 174 +1 Lines 124899 125114 +215 Branches 124899 125114 +215 ==========================================
+ Hits 110841 111249 +408 + Misses 11631 11354 -277 - Partials 2427 2511 +84

Flag	Coverage Δ
fuzzing	`22.12% <0.00%> (?)`
tests	`88.74% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

This needs a rebase now.

martinsaposnic · 2025-08-07T17:39:53Z

rebase done!

tnull

As mentioned elsewhere, we might need a spec change in parallel to this.

Also, nit: 'LSP5', 'dooldown' and 'alsp' typos in the commit message (which could otherwise also use a bit more detail what this change is and why we want it).

ldk-reviews-bot · 2025-08-08T07:01:20Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

martinsaposnic · 2025-08-11T16:33:52Z

we might need a spec change in parallel to this

lightning/blips#64 spec update PR

tnull

Thanks! Changes look good to me, but please fixup the typos in the commit message, as mentioned above.

tnull · 2025-08-12T06:46:50Z

 .get(&notification.method)
 .map(|last_sent| now.duration_since(&last_sent))
- .map_or(false, |duration| duration < DEFAULT_NOTIFICATION_COOLDOWN_HOURS)
+ .map_or(false, |duration: Duration| duration < NOTIFICATION_COOLDOWN_TIME)


Why do we need this : Duration suddenly?

FYI force-pushed this small change without a fixup https://github.com/lightningdevkit/rust-lightning/compare/d638113b68b2de0ec02dd5d2a7574f7854d5005c..8b326536eed3eaff61d4c261dd5f83d96620f76b

martinsaposnic · 2025-08-12T19:57:53Z

also fixed the commit message. sorry about the typos!

tnull

Thanks, LGTM.

Simple enough, so landing this.

(the commit message could have still used some motiviation, etc ;P)

martinsaposnic · 2025-08-07T14:30:11Z

LSPS5 follow ups. Addressing comments #3975 (review) and #3975 (comment)

changes:

notification_cooldown is not a config so it does not make sense to call the constant DEFAULT.
notification_cooldown it's not measured in hours, so changed the name, and also changed the constant from 1 hour to 1 minute (reasons explained in this comment LSPS5: Correct notification cooldown & reset logic #3975 (comment)

ldk-reviews-bot · 2025-08-07T14:30:15Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-07T15:20:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.91%. Comparing base (6d47f0a) to head (c7694f4).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3994 +/- ##
==========================================
+ Coverage 88.74% 88.91% +0.17% 
==========================================
Files 173 174 +1 Lines 124899 125114 +215 Branches 124899 125114 +215 ==========================================
+ Hits 110841 111249 +408 + Misses 11631 11354 -277 - Partials 2427 2511 +84

Flag	Coverage Δ
fuzzing	`22.12% <0.00%> (?)`
tests	`88.74% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

This needs a rebase now.

martinsaposnic · 2025-08-07T17:39:53Z

rebase done!

tnull

As mentioned elsewhere, we might need a spec change in parallel to this.

Also, nit: 'LSP5', 'dooldown' and 'alsp' typos in the commit message (which could otherwise also use a bit more detail what this change is and why we want it).

ldk-reviews-bot · 2025-08-08T07:01:20Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

martinsaposnic · 2025-08-11T16:33:52Z

we might need a spec change in parallel to this

lightning/blips#64 spec update PR

tnull

Thanks! Changes look good to me, but please fixup the typos in the commit message, as mentioned above.

tnull · 2025-08-12T06:46:50Z

 .get(&notification.method)
 .map(|last_sent| now.duration_since(&last_sent))
- .map_or(false, |duration| duration < DEFAULT_NOTIFICATION_COOLDOWN_HOURS)
+ .map_or(false, |duration: Duration| duration < NOTIFICATION_COOLDOWN_TIME)


Why do we need this : Duration suddenly?

FYI force-pushed this small change without a fixup https://github.com/lightningdevkit/rust-lightning/compare/d638113b68b2de0ec02dd5d2a7574f7854d5005c..8b326536eed3eaff61d4c261dd5f83d96620f76b

martinsaposnic · 2025-08-12T19:57:53Z

also fixed the commit message. sorry about the typos!

tnull

Thanks, LGTM.

Simple enough, so landing this.

(the commit message could have still used some motiviation, etc ;P)

martinsaposnic mentioned this pull request Aug 7, 2025
LSPS5 Follow ups #3944
Closed
18 tasks

ldk-reviews-bot requested a review from joostjager August 7, 2025 14:40

tnull reviewed Aug 7, 2025
View reviewed changes

tnull removed the request for review from joostjager August 7, 2025 17:30

martinsaposnic force-pushed the lsps5-cooldown-time-fix branch from ecf258c to 43eeb48Compare August 7, 2025 17:37

martinsaposnic requested a review from tnull August 7, 2025 17:39

graphite-appBot reviewed Aug 7, 2025
View reviewed changes

Comment threadlightning-liquidity/src/lsps5/service.rs

martinsaposnic force-pushed the lsps5-cooldown-time-fix branch from 43eeb48 to 24712cfCompare August 7, 2025 17:42

tnull reviewed Aug 8, 2025
View reviewed changes

martinsaposnic changed the title ~~[LSP5] Change notification dooldown time to 1 minute, alsp update docs~~[LSP5] Change notification cooldown time to 1 minute, also update docsAug 11, 2025

martinsaposnic force-pushed the lsps5-cooldown-time-fix branch from 24712cf to d638113Compare August 11, 2025 15:15

martinsaposnic changed the title ~~[LSP5] Change notification cooldown time to 1 minute, also update docs~~[LSPS5] Change notification dooldown time to 1 minute, alsp update docsAug 11, 2025

martinsaposnic requested a review from tnull August 11, 2025 16:33

tnull reviewed Aug 12, 2025
View reviewed changes

martinsaposnic force-pushed the lsps5-cooldown-time-fix branch from d638113 to 8b32653Compare August 12, 2025 12:55

[LSPS5] Change notification cooldown time to 1 minute, also update docs.
c7694f4

martinsaposnic force-pushed the lsps5-cooldown-time-fix branch from 8b32653 to c7694f4Compare August 12, 2025 13:46

martinsaposnic changed the title ~~[LSPS5] Change notification dooldown time to 1 minute, alsp update docs~~[LSPS5] Change notification cooldown time to 1 minute, also update docsAug 12, 2025

martinsaposnic requested a review from tnull August 12, 2025 18:28

tnull approved these changes Aug 13, 2025
View reviewed changes

tnull merged commit efb8d58 into lightningdevkit:mainAug 13, 2025
23 of 24 checks passed