Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace common EventHandler with node-specific ones #885

Closed
Tracked by #975
luckysori opened this issue Jul 4, 2023 · 1 comment · Fixed by #1073
Closed
Tracked by #975

Replace common EventHandler with node-specific ones #885

luckysori opened this issue Jul 4, 2023 · 1 comment · Fixed by #1073
Assignees
@klochowicz
Labels
technical-debt
Milestone
1.3.0

Comments

@luckysori
Copy link
Contributor

luckysori commented Jul 4, 2023 •
edited
Loading

App and coordinator should handle LDK events differently. For instance:

  • The coordinator doesn't want to claim certain payments straight away.
  • The coordinator can intercept payments to create JIT channels.
  • The app needs to pay a channel-opening fee as soon as a new JIT channel has been created.

These systems will diverge more as we keep evolving 10101. It's a good idea to tackle this as soon as possible so that we don't keep building technical debt unnecessarily. At the moment this lack of separation doesn't even lead to simpler solutions, so it's hard to justify.

Some PRs where the status quo has created friction:
@luckysori luckysori added this to the 1.1.0 milestone Jul 4, 2023
@luckysori luckysori mentioned this issue Jul 4, 2023
Closed
@bonomat bonomat modified the milestones: 1.1.0, 1.2.0 Jul 10, 2023
@luckysori luckysori mentioned this issue Jul 19, 2023
Merged
5 tasks
@bonomat bonomat mentioned this issue Jul 21, 2023
Closed
@luckysori luckysori mentioned this issue Jul 22, 2023
Merged
bors bot added a commit that referenced this issue Jul 25, 2023
@bors @luckysori
Merge #979
290884c 
979: Fail intercepted HTLC backwards when we should r=luckysori a=luckysori

Fixes #943.
Fixes #944.

---

This is yet another instance where we have to consider the fact that the same event handler code runs for both app and coordinator even though certain parts need to be specific to each role. I've increased the implicit annoyance counter of #885.

Co-authored-by: Lucas Soriano del Pino <[email protected]>
bors bot added a commit that referenced this issue Jul 25, 2023
@bors @luckysori
Merge #979
0a9c34d 
979: Fail intercepted HTLC backwards when we should r=luckysori a=luckysori

Fixes #943.
Fixes #944.

---

This is yet another instance where we have to consider the fact that the same event handler code runs for both app and coordinator even though certain parts need to be specific to each role. I've increased the implicit annoyance counter of #885.

Co-authored-by: Lucas Soriano del Pino <[email protected]>
@bonomat
Copy link
Contributor

bonomat commented Jul 30, 2023

Imho we should do this soon because with #926 we will have another actor which might have difference needs for the EventHandler.

@klochowicz klochowicz self-assigned this Aug 1, 2023
@klochowicz klochowicz linked a pull request Aug 5, 2023 that will close this issue
Use different event handlers for app and coordinator #1073
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@klochowicz klochowicz

Labels
technical-debt
Projects
None yet
Milestone
1.3.0
Development

Successfully merging a pull request may close this issue.

Use different event handlers for app and coordinator get10101/10101
3 participants
@bonomat @klochowicz @luckysori