[sql-3] accounts: replace calls to UpdateAccount #939
Conversation
ellemouton
added
no-changelog
And use it in the InterceptorService's paymentUpdate method.
Instead of useing UpdateAccount.
And use it instead of UpdateAccount.
Instead of UpdateAccount.
ellemouton
force-pushed
the
sql3Accounts3
branch
from
daa5c0b to
e557374
Compare
There was a problem hiding this comment.
Generally looks good! I've left a few comments below, where there's mainly one that needs to be fixed.
As also communicated offline, I do think the added options to the API UpsertAccountPayment makes this updated version harder to read and reason about than the current implementation. I think it's worth considering if it could be designed a bit differently as I feel there's a high risk of introducing bugs when the API is a bit complicated.
| // succeeded. We can then match on the ErrAlreadySucceeded error and | ||
| // exit early if it is returned. | ||
| opts := []UpsertPaymentOption{ | ||
| WithErrIfAlreadySucceeded(), |
There was a problem hiding this comment.
We're not matching on ErrAlreadySucceeded below. This function should return nil if it is an ErrAlreadySucceeded error.
| pendingPayment.cancel() | ||
| delete(s.pendingPayments, hash) |
There was a problem hiding this comment.
Just noting that similar to the other PR, this will now run before we've checked if there's an account id for the pendingPayment. I do that should be unreachable though, and probably actually correct behaviour, so don't really think this needs to change.
| if opts.usePendingAmount { | ||
| fullAmount = entry.FullAmount | ||
| } |
There was a problem hiding this comment.
Just noting that this is a difference from the current behaviour, where this was set regardless of if an entry existed or not, as long as fullAmount was zero. That feels pretty dangerous 😅, so good fix 🚀!
| @@ -105,6 +105,188 @@ func assertEqualAccounts(t *testing.T, expected, | |||
| actual.LastUpdate = actualUpdate | |||
| } | |||
|
|
|||
| // TestAccountUpdateMethods tests that all the Store methods that update an | |||
| // account work correctly. | |||
| func TestAccountUpdateMethods(t *testing.T) { | |||
There was a problem hiding this comment.
Similar to the other PR, we should test the behaviour for an unknown account id :).
(similar PR description to #938 but they are not dependent on each other)
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 the calls that edit/add account payments:
UpsertAccountPaymentDeleteAccountPayment