accounts: add functionality to update balance by given amount #962

ViktorTigerstrom · 2025-02-05T18:01:04Z

Fixes #648

Based on #954

This PR introduces functionality to update an account's balance by a given amount, rather than the current update method, which sets the balance to a specific value.

My plan is to deprecate the old functionality in a follow-up PR after this one is merged.

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.

The current go.sum file was out of sync with the go.mod file. This commit updates the go.sum file by running `go mod tidy` to match the go.mod file.

This commit introduces the structure for a new endpoint in the accounts subsystem, enabling balance adjustments by adding or deducting a specified amount. This contrasts with the existing `UpdateAccount` implementation, which directly sets the balance to a fixed value. For more details on the drawbacks of the current implementation, see [issue lightninglabs#648](lightninglabs#648). The actual implementation of the endpoint will be introduced in subsequent commits.

This commit implements the `updatebalance` command in `litcli`. The command is implemented with two different subcommands: `add`, which increases an existing off-chain account’s balance, and `deduct`, which decreases it accordingly.

To support updating an existing off-chain account’s balance by a specified amount in the database, we'll implement a corresponding functions for the different stores. This commit introduces the `AdjustAccountBalance` function in the kvdb store.

ViktorTigerstrom · 2025-02-05T18:05:25Z

db/sqlc/queries/accounts.sql

+-- name: AdjustAccountBalance :one
+UPDATE accounts
+SET current_balance_msat = current_balance_msat + CASE
+    WHEN sqlc.arg(is_addition)::BOOLEAN THEN sqlc.arg(amount)  -- If IsAddition is true, add the amount
+    ELSE -sqlc.arg(amount)         -- If IsAddition is false, subtract the amount
+END
+WHERE id = sqlc.arg(id)
+RETURNING id;
+


Let me know if you prefer that I use the current functionality instead to fetch the current balance, and then set the balance to the currentBalance +- amount.
My motivation for introducing this was to discourage such functionality in the future, which directly sets the balance.

agreed re having specific queries for how we prefer to use it 👍
see comment re splitting this out though

This commit introduces the `AdjustAccountBalance` function in the sqlc store, to support updating an existing off-chain account’s balance by a specified amount for sql backends.

With all underlying functionality in place to support updating an existing off-chain account’s balance by a specified amount, this commit implements the `UpdateBalance` endpoint to utilize that functionality.

ellemouton

took quick peek (before review was requested - hope you dont mind!) - just leaving some drive by comments in the mean time.

I think we can split this up into 2 PRs: 1) DB + Service additions & tests for those. 2) expose new functionality via RPC + CLI calling code

ellemouton · 2025-02-06T05:24:13Z

accounts/rpcserver.go

+
+	switch reqType := req.GetUpdate().(type) {
+	case *litrpc.UpdateAccountBalanceRequest_Add:
+		log.Infof("[addbalance] id=%s, label=%v, amount=%d",


what do you think of using "credit/debit" instead of "add/deduct"?

Ah yeah definitely! I was struggling with naming, and agree that credit & debit is better.

ellemouton · 2025-02-06T05:25:32Z

accounts/store_test.go

@@ -71,6 +72,18 @@ func TestAccountStore(t *testing.T) {
 	)
 	require.NoError(t, err)

+	// Adjust the account balance by first adding 10000, and then deducting
+	// 5000.


for review it's nicer if unit tests are in the same commit as the new addition :)

ellemouton · 2025-02-06T05:27:05Z

cmd/litcli/accounts.go

 			updateAccountCommand,
+			updateBalanceCommands,


wondering if we cant fit this under the existing update category somehow?

also, how about just having explicit credit/debit sub commands?

also, how about just having explicit credit/debit sub commands?

Went with this option!

ellemouton · 2025-02-06T05:32:35Z

accounts/rpcserver.go

@@ -132,6 +132,14 @@ func (s *RPCServer) UpdateAccount(ctx context.Context,
 	return marshalAccount(account), nil
 }

+// UpdateBalance adds or deducts an amount from an existing account in the


just an idea for standard PR flow: we always try to (as much as possible at least) to keep commits ordered such that things are working commit by commit. For 2 reasons: 1) reverting back to any single commit isnt a problem and 2) review story telling.

so the ideal flow for RPC additions is:

add DB/Service functionality along with any tests for those (in small PR world, that could honestly be the whole PR - since that is like 1 unit of code that is easy to reason about and also lets us continue the on the accounts SQL PR while we iterate on the second unit here which is proto/CLI design).

add proto definitions & call the new code from the rpc server code

add CLI calling code

the high level pattern im describing here is:

server implements new functionality but doesnt expose it yet

server exposes new functionality

client starts calling new functionality

Thanks a lot for the suggestion! I've tried to follow this with the introduction of the new #973 & #974 PRs.

ellemouton · 2025-02-06T05:36:19Z

accounts/rpcserver.go

+	if amount <= 0 {
+		return nil, fmt.Errorf("amount %v must be greater than 0",
+			int64(amount/1000))
+	}


why not use uints in the proto definitions then?

Good idea! Addressed in the new PR.

ellemouton · 2025-02-06T05:39:31Z

db/sqlc/queries/accounts.sql

+-- name: AdjustAccountBalance :one
+UPDATE accounts
+SET current_balance_msat = current_balance_msat + CASE
+    WHEN sqlc.arg(is_addition) THEN sqlc.arg(amount)  -- If IsAddition is true, add the amount
+    ELSE -sqlc.arg(amount)         -- If IsAddition is false, subtract the amount
+END
+WHERE id = sqlc.arg(id)
+RETURNING id;
+


i think let's keep things simple with just having
CreditAccount and DebitAccount queries?

Updated in the new PR :)

ellemouton · 2025-02-06T05:44:28Z

litrpc/lit-accounts.proto

@@ -148,6 +154,28 @@ message UpdateAccountRequest {
    string label = 4;
 }

+message UpdateAccountBalanceRequest {


so expanding on my other comment: it doesnt really make sense to starting off the PR with rpc definitions as we dont really know how this is going to be used yet etc.

Addressed in the new PR :)

ellemouton · 2025-02-06T05:46:09Z

litrpc/lit-accounts.proto

+    oneof update {
+        // Increase the account's balance by the given amount.
+        int64 add = 3;
+
+        // Deducts the given amount from the account balance.
+        int64 deduct = 4;
+    }


when defining enums like this, it's good to ask: is there a future where more than 2 options will actually be needed? if not, then this can either be a boolean. Or we can even just have separate Credit/Debit definitions

so either:

Update { - ID - Amount - Credit/Debit bool }

or

Credit { ID, Amount } _and_ Debit {ID, Amount}

redit { ID, Amount }
and
Debit {ID, Amount}

Thanks! Went with the second option :)

ellemouton · 2025-02-06T05:50:43Z

My plan is to deprecate the old functionality in a follow-up PR after this one is merged.

good idea. I think we always want to give notice of deprecated endpoints though. So i think in the proto definitions here, you can add a deprecated annotation along with the release where it will be completely removed. Then in the release notes, add a notice about this planned depreciation. Then our next release (or couple releases) will include supporting both to give folks a chance to update to the new endpoint

ViktorTigerstrom · 2025-02-11T20:12:12Z

Replaced by #973 & #974.

ellemouton and others added 6 commits January 31, 2025 08:54

make+GH: helpers for testing against new SQL stores

21c0677

litrpc: run go mod tidy

8c24c1a

The current go.sum file was out of sync with the go.mod file. This commit updates the go.sum file by running `go mod tidy` to match the go.mod file.

litcli: add updatebalance command

3cd1569

This commit implements the `updatebalance` command in `litcli`. The command is implemented with two different subcommands: `add`, which increases an existing off-chain account’s balance, and `deduct`, which decreases it accordingly.

ViktorTigerstrom commented Feb 5, 2025

View reviewed changes

ViktorTigerstrom added 4 commits February 6, 2025 01:41

accounts: implement sqlc AdjustAccountBalance

a01233b

This commit introduces the `AdjustAccountBalance` function in the sqlc store, to support updating an existing off-chain account’s balance by a specified amount for sql backends.

accounts: add AdjustAccountBalance store tests

debfa05

accounts: implement UpdateBalance endpoint

54005b0

With all underlying functionality in place to support updating an existing off-chain account’s balance by a specified amount, this commit implements the `UpdateBalance` endpoint to utilize that functionality.

docs: update release notes

aed7ef3

ViktorTigerstrom force-pushed the 2025-02-accounts-cli-update-rework branch from a851302 to aed7ef3 Compare February 6, 2025 00:41

ellemouton reviewed Feb 6, 2025

View reviewed changes

ViktorTigerstrom closed this Feb 11, 2025

accounts: add functionality to update balance by given amount #962

accounts: add functionality to update balance by given amount #962

Uh oh!

Conversation

ViktorTigerstrom commented Feb 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton commented Feb 6, 2025

Uh oh!

ViktorTigerstrom commented Feb 11, 2025

Uh oh!

Uh oh!

ellemouton left a comment •

edited

Loading

ellemouton Feb 6, 2025 •

edited

Loading