account: improve account onchain transaction labels #396
Conversation
This file contains hidden or 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
ffranr
force-pushed
the
new_account_mod_tx_labels
branch
3 times, most recently
from
25c02f1 to
f5d3203
Compare
1 task
ffranr
force-pushed
the
new_account_mod_tx_labels
branch
2 times, most recently
from
450451a to
645daf6
Compare
ffranr
changed the title
guggero
reviewed
Oct 28, 2022
Contributor
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Looks pretty good, I like the new label format!
I'm wondering if we should also start writing the same account modification data into the database alongside the accounts to make retrieval even easier and independent of lnd's label database (re-scanning the wallet can drop the labels, which users do sometimes).
Then we kind of have redundant information, but the new label is also just nice for the user to see what happened, so I don't think it's overkill.
ffranr
force-pushed
the
new_account_mod_tx_labels
branch
2 times, most recently
from
71a3a35 to
758c156
Compare
guggero
reviewed
Nov 2, 2022
Contributor
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Very nice work, looks very good!
Just a few comments about error handling and nits, nothing major 🎉
ffranr
force-pushed
the
new_account_mod_tx_labels
branch
from
758c156 to
df2f6a3
Compare
ffranr
force-pushed
the
new_account_mod_tx_labels
branch
from
df2f6a3 to
7d8bc1b
Compare
This commit reformulates transaction labels as JSON strings. The JSON contains the following fields: * An `action` field which takes one of the following values: create, deposit, withdraw, renew, or close. * `key` which is the account key. * `expiry_height` which is an integer block height at which the account expires. This value is important to ease the process of account recovery. * `output_index` which is an integer denoting the account balance transaction output. * `expiry_spend` which is a boolean denoting whether the transaction spends via the expiry output path. * `tx_fee` which is the onchain transaction fee for executing the account action. * `balance_diff` which is a signed integer denoting the difference (+/-) in the account balance as a result of the transaction. The poold specific transaction label prefix tag is also modified such that its prefixes spaces are removed.
ffranr
force-pushed
the
new_account_mod_tx_labels
branch
from
7d8bc1b to
d76f0f1
Compare
guggero
reviewed
Nov 4, 2022
Contributor
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Getting very close here! Only a few nits remaining 🎉
Contributor
|
prematurely merged, follow up PR to continue improvements and reference this PR |
Contributor
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Keep prefix tag, but drop the leading space:
poold --. Faraday looks for this tag.Label structure:
{ "account":{ "action": "<action>", "key": "<string>", "expiry_height": <int>, "output_index": <int>, "expiry_spend": <bool>, "tx_fee": <int>, "balance_diff": <int>, } }The
actionfield takes one of the following values:createdepositwithdrawrenewclosekeyis the account key.expiry_heightis an integer block height at which the account expires. This value is important to ease the process of account recovery, see here.output_indexis an integer denoting the account balance transaction output.expiry_spendis a boolean denoting whether the transaction spends via the expiry output path.tx_feeis the onchain transaction fee for executing the account action.balance_diffis a signed integer denoting the difference (+/-) in the account balance as a result of the transaction.Here's a full minimized example (211 characters):
Pull Request Checklist
used.