[sql-1]accounts: preparatory commits for SQL-izing accounts #934
+296
−188
Conversation
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
We rename the `store.go` file to `store_kvdb.go` to indicate that this file contains the kvdb implementation of the accounts DB. This is in preparation for adding a sql-backed implementation later on. We do this early on in the PR so that any changes that need to be made during the review process can be easily addressed with fix-up commits that edit the newly named file.
Update the accounts `Store` and `Service` interfaces take a context. This is in preparation for when the backend DB of the accounts service is a SQL store which will have methods that take a context.
We want to be able to pass different DB implementations to NewService. In preparation for this, we make it implementation agnostic by letting it take a `Store` instead of constructing one itself. This this change, we also let LiT handle the closing of the accounts Store instead of the accounts service
This commit adds two new test helpers, NewTestDB and NewTestDBFromPath in a file that is only built when the test_db_postgres and test_db_sqlite build flags are not set. When we add sql backends, we will add helpers with the same names for each new backend. We will then use the appropriate build flags to run our unit tests against all backends.
ellemouton
requested review from
ViktorTigerstrom
and removed request for
ViktorTigerstrom
ellemouton
added
no-changelog
In later commits, we will use this `storeAccount` helper quite often. Instead of needing to remember to update the timestamp outside the call, it make sense to instead update the timestamp within the function. Yes this does mean that sometimes we make no overall changes but do update the timestamp but this is a pretty standard pattern that a "last updated" timestamp is updated at any point that we re-write a record (even if it does not have a net change).
In preparation for when we have a SQL DB implementation, we want our unit tests to run smoothly against all DB backends and have the same results. To achieve this, we need to turn some errors into global error variables that can be matched against instead. In this commit, we do this for the unique constraint violation of the account label.
ellemouton
force-pushed
the
sql1Accounts1
branch
from
15b65ee to
96ebd3c
Compare
|
updated commit messages to be more descriptive |
ViktorTigerstrom
approved these changes
Jan 15, 2025
| @@ -193,7 +188,7 @@ func (s *InterceptorService) Start(ctx context.Context, | |||
| return | |||
| } | |||
|
|
|||
| if err := s.invoiceUpdate(invoice); err != nil { | |||
| if err := s.invoiceUpdate(ctx, invoice); err != nil { | |||
There was a problem hiding this comment.
nit: line length, so might be worth breaking this into two lines.
| @@ -645,7 +642,7 @@ func TestAccountService(t *testing.T) { | |||
| } | |||
|
|
|||
| assertEventually(t, func() bool { | |||
| addIdx, settleIdx, err := s.store.LastIndexes() | |||
| addIdx, settleIdx, err := s.store.LastIndexes(ctx) | |||
There was a problem hiding this comment.
nit: line length here and in the assertEventually call below as well, so might be worth pushing the ctx arg to the next line.
| @@ -200,6 +198,8 @@ func (s *BoltStore) UpdateAccount(_ context.Context, | |||
| func storeAccount(accountBucket kvdb.RwBucket, | |||
| account *OffChainBalanceAccount) error { | |||
|
|
|||
| account.LastUpdate = time.Now() | |||
accounts/checkers_test.go
Outdated
| @@ -523,7 +523,9 @@ func testSendPayment(t *testing.T, uri string) { | |||
| errFunc := func(err error) { | |||
| lndMock.mainErrChan <- err | |||
| } | |||
| service, err := NewService(t.TempDir(), errFunc) | |||
| store, err := NewBoltStore(t.TempDir(), DBFilename) | |||
There was a problem hiding this comment.
Should we close the store at the end of the tests here? I see this is changed in the next commits, with addition of the interface, so maybe it'd be worth adding a cleanup return arg for that interface that closes the store?
accounts/service_test.go
Outdated
| @@ -829,7 +829,9 @@ func TestAccountService(t *testing.T) { | |||
| errFunc := func(err error) { | |||
| lndMock.mainErrChan <- err | |||
| } | |||
| service, err := NewService(t.TempDir(), errFunc) | |||
| store, err := NewBoltStore(t.TempDir(), DBFilename) | |||
There was a problem hiding this comment.
Same here if not already closed in the validate func.
| @@ -0,0 +1,30 @@ | |||
| package accounts | |||
There was a problem hiding this comment.
Nit: the commit message contains info regarding build flags logic, which isn't present in the actual commit (which i suspect will be added later?).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR contains a couple of minor clean-up and prep commits. These set
up the work that will follow to start massaging the accounts DB Store to
be ready for a SQL implementation.
Each commit is ultra tiny :)
Issue: #917