Add get_budget implementation #726
Conversation
jklein24
force-pushed
the
feat/get_budget
branch
from
6208eff
to
a724bd1
Compare
constants/constants.go
Outdated
| @@ -22,6 +22,7 @@ const ( | |||
| const ( | |||
| PAY_INVOICE_SCOPE = "pay_invoice" // also covers pay_keysend and multi_* payment methods | |||
| GET_BALANCE_SCOPE = "get_balance" | |||
| GET_BUDGET_SCOPE = "get_budget" | |||
There was a problem hiding this comment.
I am wondering if we should not have a scope for this one - instead it chould always be callable. Is there any benefit of having a scope? (also, an extra scope requires including an extra checkbox in the UI that the user has to understand when configuring permissions)
There was a problem hiding this comment.
Agreed having it always be callable makes sense. Is there another method which has that behavior right now that I could copy? If not, I'll just dig a bit deeper to make that work.
There was a problem hiding this comment.
Added an always granted scope with some tests for this. That way, if there are other "always allowed" methods, they can just be added there.
There was a problem hiding this comment.
I think the get_info method also bypasses the check here: https://github.com/getAlby/hub/blob/master/nip47/event_handler.go#L272
We need to come up with a better solution, but for now to see if it works you could try adding an && nip47Request.Method != models.GET_BUDGET_METHOD
There was a problem hiding this comment.
Oh cool, should I just add get_info to my new ALWAYS_GRANTED scope instead?
There was a problem hiding this comment.
I'm not sure about the ALWAYS_GRANTED scope. This seems to have some side effects (for example, the new scope is now returned and the frontend does not know how to display it)
Also the scope is now included in only new apps in the database (there is a new app_permissions row for always_granted), but only new ones. The older app connections will not have it.
And it seems a bit hacky to even call HasPermission explicitly with the always_granted scope which actually just always returns true anyway.
I know my original suggestion is not the best, but on the plus side it's isolated to a single line of code and simply skips the permission check for that request method.
@bumi @im-adithya any thoughts on this one?
There was a problem hiding this comment.
Ah, thanks for catching the UI issue, I hadn't noticed that! FWIW, my intent here was actually to not store this scope in the DB or show it in the UI at all. Instead, I was hoping to just have it be implicitly granted to all requests. I'll try to make that the case now, but can definitely switch to a specific GET_BUDGET_SCOPE if you prefer. I just think it's a bit weird to show that in the UI in general.
There was a problem hiding this comment.
Adjusted to avoid always_granted in the db and UI. Let me know if you'd still prefer an explicit budget scope though.
There was a problem hiding this comment.
Thanks! I also don't think we should have a GET_BUDGET_SCOPE. I was just wondering if a single line check like I suggested would be simpler than having this ALWAYS_GRANTED scope, and in the end we just bypass the logic anyway.
|
|
||
| maxAmount := appPermission.MaxAmountSat | ||
| if maxAmount == 0 { | ||
| publishResponse(&models.Response{ |
There was a problem hiding this comment.
We actually did not catch this in your proposal. I am not sure an empty result makes sense. Maybe the connection should not support this method and return an error instead if the app tries to call this method when a budget is not set?
There was a problem hiding this comment.
Hmm that's a good alternative too. Maybe we should discuss on the NIP PR. There are some tradeoffs to think through for sure.
|
@jklein24 thanks for the PR. Awesome! |
- replace always granted scope with always granted method list - return nil for budget renews at for "never" budget renewal - add extra tests
|
@jklein24 I have added some changes in a feedback PR here jklein24#1 I've tested the flow, and everything looks good to me. Please let me know if you have any concerns with the changes! |
|
JS SDK implementation: getAlby/js-sdk#264 |
LGTM, thanks! |
Closes #703