Skip to content

accounts: Add credit and debit account functionality #974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

ViktorTigerstrom
Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom commented Feb 11, 2025

Fixes #648

This PR adds the actual RPC server implementation and exposes the functionality to credit or debit an account by a given amount.

This PR also introduces a depreciation warning for the litcli accounts update --new_balance flag.

Copy link
Contributor

@bitromortac bitromortac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, this will be useful and safer than the current approach 👍.

I tested a bit and found that the following works (although one is only able to credit and debit with the rpcs in the intended directions, so no sign change is possible).

No change (due to overflow):
litcli.sh accounts credit account_id 9223372036854775808

Credit X sat:
litcli.sh accounts credit account_id 9223372036854775808 + X

Debit X sat:
litcli.sh accounts debit account_id 9223372036854775808 + X

Maybe we want to prevent this on the RPC level?

Edit: not sure it's possible to detect as that may depend on the flag library

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.
@ViktorTigerstrom ViktorTigerstrom force-pushed the 2025-02-credit-debit-accounts-impl branch from 26ad81b to 5b4be2f Compare February 26, 2025 11:15
@ViktorTigerstrom
Copy link
Contributor Author

Thanks a lot for the review @bitromortac!

Edit: not sure it's possible to detect as that may depend on the flag library

As also discussed offline earlier: Yeah I'm not sure there's anything we could do to avoid that issue unfortunately :/. But thanks a lot for testing this and good spotting of the issue!

@ViktorTigerstrom ViktorTigerstrom force-pushed the 2025-02-credit-debit-accounts-impl branch from 5b4be2f to 7bd42e9 Compare February 26, 2025 11:24
Copy link
Contributor

@bitromortac bitromortac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ⚡

@lightninglabs-deploy
Copy link

@ViktorTigerstrom, remember to re-request review from reviewers when ready

@ViktorTigerstrom ViktorTigerstrom force-pushed the 2025-02-credit-debit-accounts-impl branch from 7bd42e9 to 20c0eb8 Compare March 13, 2025 22:04
@ViktorTigerstrom
Copy link
Contributor Author

ViktorTigerstrom commented Mar 18, 2025

Thanks for the reviews @bitromortac & @ellemouton! I've addressed your feedback with the latest push :)

Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good! basically g2g

@ViktorTigerstrom ViktorTigerstrom force-pushed the 2025-02-credit-debit-accounts-impl branch from 20c0eb8 to 9b4bca1 Compare March 25, 2025 17:34
@ViktorTigerstrom
Copy link
Contributor Author

Thanks for the review @ellemouton! I've addressed your comments in the latest push :)

Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Final suggestion I promise :) what do you think of moving the new commands to rather be sub-commands of the update command? that way we keep things organised.

litcli.patch

This commit introduces the structure for new endpoints in the accounts
subsystem, enabling balance adjustments by crediting or debiting 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 endpoints will be introduced in
subsequent commits.
This commit implements the `CreditAccount` and `DebitAccount` endpoints
for the accounts subsystem.
This commit implements the `credit` and `debit` commands in `litcli`,
allowing users to increase or decrease the balance of an existing
off-chain account, respectively.
@ViktorTigerstrom ViktorTigerstrom force-pushed the 2025-02-credit-debit-accounts-impl branch from 9b4bca1 to f1d737c Compare March 31, 2025 00:19
This commit adds a deprecation notice regarding the
`accounts update --new_balance` flag. This flag will be removed in the
next major release of lit.
@ViktorTigerstrom ViktorTigerstrom force-pushed the 2025-02-credit-debit-accounts-impl branch from f1d737c to 0c520ce Compare March 31, 2025 00:46
@ViktorTigerstrom
Copy link
Contributor Author

Looks good! Final suggestion I promise :) what do you think of moving the new commands to rather be sub-commands of the update command? that way we keep things organised.

litcli.patch

Cool thanks :)! I've updated the PR to address your feedback. Just a question of how we actually will implement the deprecation, as this will be my first time doing so:
In v16.0, will we just remote the account_balance field in the UpdateAccountRequest proto, and then also remove the new_balance argument in the litcli update command?

Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work 🙏

Just a question of how we actually will implement the deprecation, as this will be my first time doing so:
In v16.0, will we just remote the account_balance field in the UpdateAccountRequest proto, and then also remove the new_balance argument in the litcli update command?

good question - tbh I think it's sort of up to us here cause: in LND, i think we've only really ever marked things as deprecated but then never actually followed through with removing them. So I think it's up to us and it depends on how urgently we want it to be deprecated and also what we expect the usage of it to be.
For this, perhaps yes a 1 release cycle heads up is good enough. We can perhaps add an explicit Deprecation notice section in the release notes to make things way more clear. If users complain at some point that the 1 release cycle heads up was too short, then we can either revert the change temporarily or just take note of the feedback for future deprecations.

@ellemouton ellemouton merged commit 9e5578c into lightninglabs:master Apr 1, 2025
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

accounts: extend update command functionality
4 participants