[sql-7] accounts: add SQL implementation of the accounts store #954
Conversation
ellemouton
added
database
ellemouton
force-pushed
the
sql7Accounts7
branch
2 times, most recently
from
f326bdb to
87fa366
Compare
ellemouton
force-pushed
the
sql7Accounts7
branch
from
87fa366 to
a018629
Compare
ellemouton
force-pushed
the
sql7Accounts7
branch
3 times, most recently
from
27c7272 to
9207179
Compare
| if opts.errIfAlreadyPending && | ||
| currStatus != lnrpc.Payment_FAILED { | ||
|
|
||
| return fmt.Errorf("payment with hash %s is "+ | ||
| "already in flight or succeeded "+ | ||
| "(status %v)", hash, currStatus) | ||
| } |
There was a problem hiding this comment.
Should probably have commented this in kvdb PR, but this will return an error if the currStatus is lnrpc.Payment_UNKOWN, which seems a bit incorrect?
There was a problem hiding this comment.
as discussed offline, seems like a parallel issue right?
There was a problem hiding this comment.
I've rechecked this, and the current behavior is correct—it should return an error if currStatus != lnrpc.Payment_FAILED. In other words, it should also error on lnrpc.Payment_UNKNOWN.
I believe the confusion comes from the doc comments, which state that this only fails if the payment is "in-flight or succeeded." However, we actually treat lnrpc.Payment_UNKNOWN as "in-flight" as well, which I think would be clearer if the docs mentioned that.
I can update the doc comments in a separate PR after this is merged if you'd like.
There was a problem hiding this comment.
I can update the docs on next review round :)
There was a problem hiding this comment.
since the minimum compatible version of LND with LiT is v0.18.5-beta, and this UNKNOWN status will never be returned in that version since it is deprecated, i think it is fine to leave as is?
// Deprecated. This status will never be returned.
//
// Deprecated: Marked as deprecated in lightning.proto.
Payment_UNKNOWN Payment_PaymentStatus = 0
9207179 to
21c0677
Compare
|
@bitromortac: review reminder |
ellemouton
force-pushed
the
sql7Accounts7
branch
from
21c0677 to
9aa1c06
Compare
In this commit, we add the SQLStore type which implements the accounts.Store interface. To demonstrate that it works as expected, we also plug this implementation into all the account unit tests to show that they pass against the sqlite and postgres backends. One can use `make unit pkg=accounts tags=test_db_postgres` or `make unit pkg=accounts tags=test_db_sqlite` to test locally. Note that 2 small timestamp related changes are made to the unit tests. This is to compensate for timestamp precision in postgres.
ellemouton
force-pushed
the
sql7Accounts7
branch
from
9aa1c06 to
0ec5d94
Compare
Part of #917
In this commit, we add the SQLStore type which implements the
accounts.Store interface. To demonstrate that it works as expected, we
also plug this implementation into all the account unit tests to show
that they pass against the sqlite and postgres backends.
One can use
make unit pkg=accounts tags=test_db_postgresormake unit pkg=accounts tags=test_db_sqliteto test locally.