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

[NWC] Add get_budget command for per-connection budget limits. #1504

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[NWC] Add get_budget command for per-connection budget limits. #1504

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
17cd5be
[NWC] Add get_budget command for per-connection budget limits.
jklein24 Sep 18, 2024
2109664
Suggested comment clarification
jklein24 Sep 19, 2024
1c96bc4
Add renewal_period
jklein24 Sep 19, 2024
9dd2256
Switch to used budget instead of remaining
jklein24 Sep 23, 2024
1a65d51
Remove explicit msats in names
jklein24 Sep 23, 2024
587be06
Add back msats comment
jklein24 Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions 47.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,34 @@ Response:
}
```

### `get_budget`

Budgets can be used to limit the amount of funds that can be spent by a connection in a given time period.
Budgets are optional and can be set by the user if their wallet supports it. Support for this command indicates
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rolznz and I were discussing on the alby-hub implementation what should happen if there's no budget set for a connection. My initial thinking was that absence of a budget would be represented by an empty struct return value. However, @rolznz suggests to respond with an error instead. Some tradeoff off the top of my head:

Empty response:

  • [+] Logically makes sense to return no budget when no budget is set. It's not really an error case, but an expected scenario.
  • [-] Making each of the fields optional/nullable in the response is not great from an ergonomics standpoint. It's nicer to be able to just expect a full response

Error response

  • [+] No null fields. Pretty explicit that there's no budget and that the response is intentionally different.
  • [-] Global error handling in client apps may be triggered from this, when it's not really an error, but rather an expected state. I could imagine accidental error toasts, etc. in client apps.
  • [-] Client apps may not know ahead of time whether there's a budget on the connection, so they can't just avoid calling this method to avoid the error.

@rolznz wdyt of these tradeoffs? Anything I'm missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client apps may not know ahead of time whether there's a budget on the connection, so they can't just avoid calling this method to avoid the error.

@jklein24 if the app calls get_info it could know if it can call get_budget or not. I think this is a good thing for the app to do first so it knows what methods it can call. What do you think?

If it returned an empty response, would the developer check if total_budget is not empty? (e.g. in javascript: if (!!response.total_budget) { /* app has a budget */ }? ) - if we go this way, should we add some note in this spec so developers more easily know what to do?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jklein24 if the app calls get_info it could know if it can call get_budget or not. I think this is a good thing for the app to do first so it knows what methods it can call. What do you think?

Yeah, that's true. In general, is the assumption that apps normally call get_info before doing things with a connection?

If it returned an empty response, would the developer check if total_budget is not empty? (e.g. in javascript: if (!!response.total_budget) { /* app has a budget */ }? ) - if we go this way, should we add some note in this spec so developers more easily know what to do?

Yeah, agreed this is a little gross :-/. Either way we decide on this, we should definitely document it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's true. In general, is the assumption that apps normally call get_info before doing things with a connection?

Yeah, this is our recommendation. Because a wallet service could post an info event about what methods it supports, but the app connection itself could be different, based on what permissions the user selects when configuring the connection.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I guess I can be convinced on the error instead. @shreyav @bsiaotickchong curious if either of you have thoughts on this one.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect if wallets/users using budgets are also mainly creating connections with budgets, on average it'd be faster to just call get_budget without having to wait for the get_info response every time. In this case I think the empty response for get_budget is preferable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Alby Hub + JS SDK currently implement the empty response option for the no budget case.

that the wallet supports user-configured budgets. The budget will be reset at the `renews_at` timestamp.

Request:
```jsonc
{
"method": "get_budget",
"params": {
}
}
```

Response:
```jsonc
{
"result_type": "get_budget",
"result": {
"remaining_budget_msats": 10000,
jklein24 marked this conversation as resolved.
Show resolved Hide resolved
"total_budget_msats": 100000,
jklein24 marked this conversation as resolved.
Show resolved Hide resolved
"renews_at": 1693876973, // timestamp in seconds since epoch, optional. If not provided, the budget does not renew.
"renewal_period": "monthly", // daily|weekly|monthly|yearly|never
}
}
```

### `get_info`

Request:
Expand Down