Add get_budget implementation #726
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ nostr-wallet-connect | |
| nwc.db | ||
| .breez | ||
| .data | ||
| .idea | ||
|
|
||
| frontend/dist | ||
| frontend/node_modules | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package controllers | ||
|
|
||
| import ( | ||
| "context" | ||
| "github.com/getAlby/hub/db/queries" | ||
| "github.com/nbd-wtf/go-nostr" | ||
|
|
||
| "github.com/getAlby/hub/db" | ||
| "github.com/getAlby/hub/logger" | ||
| "github.com/getAlby/hub/nip47/models" | ||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| type getBudgetResponse struct { | ||
| UsedBudget uint64 `json:"used_budget"` | ||
| TotalBudget uint64 `json:"total_budget"` | ||
| RenewsAt uint64 `json:"renews_at"` | ||
| RenewalPeriod string `json:"renewal_period"` | ||
| } | ||
|
|
||
| func (controller *nip47Controller) HandleGetBudgetEvent(ctx context.Context, nip47Request *models.Request, requestEventId uint, app *db.App, publishResponse publishFunc) { | ||
|
|
||
| logger.Logger.WithFields(logrus.Fields{ | ||
| "request_event_id": requestEventId, | ||
| }).Debug("Getting budget") | ||
|
|
||
| appPermission := db.AppPermission{} | ||
| controller.db.Where("app_id = ? AND scope = ?", app.ID, models.PAY_INVOICE_METHOD).First(&appPermission) | ||
|
|
||
| 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. |
||
| ResultType: nip47Request.Method, | ||
| Result: struct{}{}, | ||
| }, nostr.Tags{}) | ||
| return | ||
| } | ||
|
|
||
| usedBudget := queries.GetBudgetUsageSat(controller.db, &appPermission) | ||
| responsePayload := &getBudgetResponse{ | ||
| TotalBudget: uint64(maxAmount * 1000), | ||
| UsedBudget: usedBudget * 1000, | ||
| RenewalPeriod: appPermission.BudgetRenewal, | ||
| RenewsAt: uint64(queries.GetBudgetRenewsAt(appPermission.BudgetRenewal)), | ||
| } | ||
|
|
||
| publishResponse(&models.Response{ | ||
| ResultType: nip47Request.Method, | ||
| Result: responsePayload, | ||
| }, nostr.Tags{}) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| package controllers | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/nbd-wtf/go-nostr" | ||
| "github.com/stretchr/testify/assert" | ||
|
|
||
| "github.com/getAlby/hub/constants" | ||
| "github.com/getAlby/hub/db" | ||
| "github.com/getAlby/hub/nip47/models" | ||
| "github.com/getAlby/hub/nip47/permissions" | ||
| "github.com/getAlby/hub/tests" | ||
| "github.com/getAlby/hub/transactions" | ||
| ) | ||
|
|
||
| const nip47GetBudgetJson = ` | ||
| { | ||
| "method": "get_budget" | ||
| } | ||
| ` | ||
|
|
||
| func TestHandleGetBudgetEvent_noneUsed(t *testing.T) { | ||
| ctx := context.TODO() | ||
| defer tests.RemoveTestService() | ||
| svc, err := tests.CreateTestService() | ||
| assert.NoError(t, err) | ||
|
|
||
| nip47Request := &models.Request{} | ||
| err = json.Unmarshal([]byte(nip47GetBudgetJson), nip47Request) | ||
| assert.NoError(t, err) | ||
|
|
||
| app, _, err := tests.CreateApp(svc) | ||
| assert.NoError(t, err) | ||
| now := time.Now() | ||
|
|
||
| appPermission := &db.AppPermission{ | ||
| AppId: app.ID, | ||
| App: *app, | ||
| Scope: constants.PAY_INVOICE_SCOPE, | ||
| MaxAmountSat: 400, | ||
| BudgetRenewal: constants.BUDGET_RENEWAL_MONTHLY, | ||
| } | ||
| err = svc.DB.Create(appPermission).Error | ||
| assert.NoError(t, err) | ||
|
|
||
| dbRequestEvent := &db.RequestEvent{} | ||
| err = svc.DB.Create(&dbRequestEvent).Error | ||
| assert.NoError(t, err) | ||
|
|
||
| var publishedResponse *models.Response | ||
|
|
||
| publishResponse := func(response *models.Response, tags nostr.Tags) { | ||
| publishedResponse = response | ||
| } | ||
|
|
||
| permissionsSvc := permissions.NewPermissionsService(svc.DB, svc.EventPublisher) | ||
| transactionsSvc := transactions.NewTransactionsService(svc.DB, svc.EventPublisher) | ||
| NewNip47Controller(svc.LNClient, svc.DB, svc.EventPublisher, permissionsSvc, transactionsSvc). | ||
| HandleGetBudgetEvent(ctx, nip47Request, dbRequestEvent.ID, app, publishResponse) | ||
|
|
||
| assert.Equal(t, uint64(400000), publishedResponse.Result.(*getBudgetResponse).TotalBudget) | ||
| assert.Equal(t, uint64(0), publishedResponse.Result.(*getBudgetResponse).UsedBudget) | ||
| renewsAt := time.Date(now.Year(), now.Month(), 1, 0, 0, 0, 0, now.Location()).AddDate(0, 1, 0).Unix() | ||
| assert.Equal(t, uint64(renewsAt), publishedResponse.Result.(*getBudgetResponse).RenewsAt) | ||
| assert.Equal(t, constants.BUDGET_RENEWAL_MONTHLY, publishedResponse.Result.(*getBudgetResponse).RenewalPeriod) | ||
| assert.Nil(t, publishedResponse.Error) | ||
| } | ||
|
|
||
| func TestHandleGetBudgetEvent_halfUsed(t *testing.T) { | ||
| ctx := context.TODO() | ||
| defer tests.RemoveTestService() | ||
| svc, err := tests.CreateTestService() | ||
| assert.NoError(t, err) | ||
|
|
||
| nip47Request := &models.Request{} | ||
| err = json.Unmarshal([]byte(nip47GetBudgetJson), nip47Request) | ||
| assert.NoError(t, err) | ||
|
|
||
| app, _, err := tests.CreateApp(svc) | ||
| assert.NoError(t, err) | ||
| now := time.Now() | ||
|
|
||
| appPermission := &db.AppPermission{ | ||
| AppId: app.ID, | ||
| App: *app, | ||
| Scope: constants.PAY_INVOICE_SCOPE, | ||
| MaxAmountSat: 400, | ||
| BudgetRenewal: constants.BUDGET_RENEWAL_MONTHLY, | ||
| } | ||
| err = svc.DB.Create(appPermission).Error | ||
| assert.NoError(t, err) | ||
|
|
||
| svc.DB.Create(&db.Transaction{ | ||
| AppId: &app.ID, | ||
| State: constants.TRANSACTION_STATE_SETTLED, | ||
| Type: constants.TRANSACTION_TYPE_OUTGOING, | ||
| AmountMsat: 200000, | ||
| }) | ||
|
|
||
| dbRequestEvent := &db.RequestEvent{} | ||
| err = svc.DB.Create(&dbRequestEvent).Error | ||
| assert.NoError(t, err) | ||
|
|
||
| var publishedResponse *models.Response | ||
|
|
||
| publishResponse := func(response *models.Response, tags nostr.Tags) { | ||
| publishedResponse = response | ||
| } | ||
|
|
||
| permissionsSvc := permissions.NewPermissionsService(svc.DB, svc.EventPublisher) | ||
| transactionsSvc := transactions.NewTransactionsService(svc.DB, svc.EventPublisher) | ||
| NewNip47Controller(svc.LNClient, svc.DB, svc.EventPublisher, permissionsSvc, transactionsSvc). | ||
| HandleGetBudgetEvent(ctx, nip47Request, dbRequestEvent.ID, app, publishResponse) | ||
|
|
||
| assert.Equal(t, uint64(400000), publishedResponse.Result.(*getBudgetResponse).TotalBudget) | ||
| assert.Equal(t, uint64(200000), publishedResponse.Result.(*getBudgetResponse).UsedBudget) | ||
| renewsAt := time.Date(now.Year(), now.Month(), 1, 0, 0, 0, 0, now.Location()).AddDate(0, 1, 0).Unix() | ||
| assert.Equal(t, uint64(renewsAt), publishedResponse.Result.(*getBudgetResponse).RenewsAt) | ||
| assert.Equal(t, constants.BUDGET_RENEWAL_MONTHLY, publishedResponse.Result.(*getBudgetResponse).RenewalPeriod) | ||
| assert.Nil(t, publishedResponse.Error) | ||
| } | ||
|
|
||
| func TestHandleGetBudgetEvent_noBudget(t *testing.T) { | ||
| ctx := context.TODO() | ||
| defer tests.RemoveTestService() | ||
| svc, err := tests.CreateTestService() | ||
| assert.NoError(t, err) | ||
|
|
||
| nip47Request := &models.Request{} | ||
| err = json.Unmarshal([]byte(nip47GetBudgetJson), nip47Request) | ||
| assert.NoError(t, err) | ||
|
|
||
| app, _, err := tests.CreateApp(svc) | ||
| assert.NoError(t, err) | ||
|
|
||
| appPermission := &db.AppPermission{ | ||
| AppId: app.ID, | ||
| App: *app, | ||
| Scope: constants.PAY_INVOICE_SCOPE, | ||
| } | ||
| err = svc.DB.Create(appPermission).Error | ||
| assert.NoError(t, err) | ||
|
|
||
| svc.DB.Create(&db.Transaction{ | ||
| AppId: &app.ID, | ||
| State: constants.TRANSACTION_STATE_SETTLED, | ||
| Type: constants.TRANSACTION_TYPE_OUTGOING, | ||
| AmountMsat: 200000, | ||
| }) | ||
|
|
||
| dbRequestEvent := &db.RequestEvent{} | ||
| err = svc.DB.Create(&dbRequestEvent).Error | ||
| assert.NoError(t, err) | ||
|
|
||
| var publishedResponse *models.Response | ||
|
|
||
| publishResponse := func(response *models.Response, tags nostr.Tags) { | ||
| publishedResponse = response | ||
| } | ||
|
|
||
| permissionsSvc := permissions.NewPermissionsService(svc.DB, svc.EventPublisher) | ||
| transactionsSvc := transactions.NewTransactionsService(svc.DB, svc.EventPublisher) | ||
| NewNip47Controller(svc.LNClient, svc.DB, svc.EventPublisher, permissionsSvc, transactionsSvc). | ||
| HandleGetBudgetEvent(ctx, nip47Request, dbRequestEvent.ID, app, publishResponse) | ||
|
|
||
| assert.Equal(t, struct{}{}, publishedResponse.Result) | ||
| assert.Nil(t, publishedResponse.Error) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
get_infomethod also bypasses the check here: https://github.com/getAlby/hub/blob/master/nip47/event_handler.go#L272We 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_METHODThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool, should I just add get_info to my new ALWAYS_GRANTED scope instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the
ALWAYS_GRANTEDscope. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thisALWAYS_GRANTEDscope, and in the end we just bypass the logic anyway.