[sql-2] accounts: start replacing calls to UpdateAccount #938
Conversation
ellemouton
added
no-changelog
ellemouton
force-pushed
the
sql2Accounts2
branch
from
b0eb915 to
6cc5854
Compare
ellemouton
force-pushed
the
sql2Accounts2
branch
from
6cc5854 to
b499498
Compare
|
thanks @bitromortac , yeah this one has popped up a few times. I've made an issue for it now: #940 i'll still re-kick the job though (after next review) so that we can have a green here before merge |
There was a problem hiding this comment.
Looks good 🚀!
Agree with Tobias on idea of converting optional types one layer higher, so great to hear that there'll be a new PR for that :)
Also for clarification I've tested just for sanity sake that the update to use btcutil.Amount in the accounts UpdateAccount method doesn't affect the current cli method behaviour (as the actual rpc servers functions aren't covered by tests).
accounts/service.go
Outdated
| s.invoiceToAccount[hash] = id | ||
|
|
||
| return s.store.UpdateAccount(ctx, account) | ||
| return s.store.AddAccountInvoice(ctx, id, hash) |
There was a problem hiding this comment.
Just noting that this changes the behaviour from the previous code, where we will now assign s.invoiceToAccount[hash] the id even if an account for that id doesn't exist in the db.
So if users are were to manipulate the account id with the context to an unknown id, that would occur.
I do think this should be fine even if that would somehow happen, despite the "memory leak" with the added entry to the map, as it must represent a real invoice which has added a lot of memory allocation either way, and the user wouldn't get credited if it was paid.
There was a problem hiding this comment.
good catch! I've updated it to only assign to the in-mem map if the DB call succeeded. lemme know what you think
|
|
||
| // Now, update just the balance of the account. | ||
| newBalance := lnwire.MilliSatoshi(123) | ||
| err = store.UpdateAccountBalanceAndExpiry( |
There was a problem hiding this comment.
This applies to all tests:
I think we should also test how these db functions behave if an unknown id is passed into the functions.
In this commit, we remove one call to the UpdateAccount store method and replace it with a call to a new UpdateAccountBalanceAndExpiry method which updates an accounts balance and/or expiry fields and finds the account via the given ID. This method signature is more appropriate for a SQL backend than the UpdateAccount method.
And use it instead of UpdateAccount for the InterceptorService's AssociateInvoice method.
And use it instead of UpdateAccount in the InterceptorService's invoiceUpdate method.
... to take a more strict btcutil.Amount type for the account balance parameter.
accounts/service.go
Outdated
| s.invoiceToAccount[hash] = id | ||
|
|
||
| return s.store.UpdateAccount(ctx, account) | ||
| return s.store.AddAccountInvoice(ctx, id, hash) |
There was a problem hiding this comment.
good catch! I've updated it to only assign to the in-mem map if the DB call succeeded. lemme know what you think
ellemouton
force-pushed
the
sql2Accounts2
branch
from
08b1ca9 to
34d1f67
Compare
To prepare the
accounts.Storeto be more SQL compatible, we will want to replace the currentUpdateAccountmethod. Today it takes a fullOffChainAccountstruct, serialises it and replacesany existing in the DB. This method is not very SQL appropriate as the better pattern is to instead
have specific update methods that read: "Update field X and account with ID Y".
So this PR starts this process of adding more SQL friendly methods to the interface and thereby
phasing out the use of
UpdateAccount. By doing so, we also move very kvdb specific logic outof the
accounts.InterceptorService.I'm splitting this up over about 3 or 4 PRs since some of the replacements are more complicated than
others.
This PR focuses on just replacing the simple ones:
UpdateAccountBalanceAndExpiryAddAccountInvoiceIncreaseAccountBalance