-
Notifications
You must be signed in to change notification settings - Fork 47
Expose historical account action fees via RPC #395
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
Expose historical account action fees via RPC #395
Conversation
9a56042 to
37cb234
Compare
|
@guggero does my solution look as if it's going in the right direction? (By the way, I haven't completed the logic yet and the formatting isn't correct.) |
37cb234 to
4aa83b9
Compare
guggero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks good! Allowed myself to add some early comments.
positiveblue
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks really nice @ffranr 🙏
4aa83b9 to
37ef2a5
Compare
|
These are the lnd transactions I'm trying to process: https://gist.github.com/ffranr/73b0624a96a93893a66ba73c5161b3b5
Ordering over the @positiveblue mentioned the following in chat:
I think he's right. After In this instance, I will only sort on |
bc91915 to
4a07bfd
Compare
11fd04c to
6a21dd1
Compare
|
I'm still stuck with a strange withdraw transaction. I would expect at least one Command:
{
"tx_hash": "755b00c399d3bf8eec0fe68156af9b90d443436bbb9f282078aeae855a2a0205",
"amount": "0",
"num_confirmations": 0,
"block_hash": "",
"block_height": 0,
"time_stamp": "1666710175",
"total_fees": "0",
"dest_addresses": [
"bcrt1p2agvu57xt3p6wut5xql59qpazhx2j02dswlprm2tv0ts2xp4xpvsed8wg8",
"bcrt1pr9tnxkd5txungm4tl4vjcl0mad3mu8cskyw96axnzla4j0qrwevsq5t4sg"
],
"output_details": [
{
"output_type": "SCRIPT_TYPE_PUBKEY_HASH",
"address": "bcrt1p2agvu57xt3p6wut5xql59qpazhx2j02dswlprm2tv0ts2xp4xpvsed8wg8",
"pk_script": "51205750ce53c65c43a77174303f42803d15cca93d4d83be11ed4b63d70518353059",
"output_index": "0",
"amount": "50000",
"is_our_address": false
},
{
"output_type": "SCRIPT_TYPE_PUBKEY_HASH",
"address": "bcrt1pr9tnxkd5txungm4tl4vjcl0mad3mu8cskyw96axnzla4j0qrwevsq5t4sg",
"pk_script": "512019573359b459b9346eabfd592c7dfbeb63be1f10b11c5d74d317fb593c037659",
"output_index": "1",
"amount": "142300",
"is_our_address": false
}
],
"raw_tx_hex": "0200000000010158b13fd365bee3ce6673efaae79bafdd59f7dba31df42b1353503c44a1b672ec0000000000000000000250c30000000000002251205750ce53c65c43a77174303f42803d15cca93d4d83be11ed4b63d70518353059dc2b02000000000022512019573359b459b9346eabfd592c7dfbeb63be1f10b11c5d74d317fb593c037659014047a40ba6132b9ab6da159aa7fd7bb814ee9f58751d5e50db07d92f8b3a7929f853c49f8488d0518a8404bd62ec164b7f724e554fa9c030839e1d8a4e7570da0100000000",
"label": " poold -- AccountModification(acct_key=038709293544c27d37c632f15bc5a2a2d0ab6d0fc46e6fbc28ec899015d26c3f9c, expiry=false, deposit=false, is_close=false)",
"previous_outpoints": [
{
"outpoint": "ec72b6a1443c5053132bf41da3dbf759ddaf9be7aaef7366cee3be65d33fb158:0",
"is_our_output": false
}
]
},lnd/lncli versions seems to be correct: And I'm using the latest commit (+ my changes) for pool and subasta. |
25ac20d to
4516636
Compare
guggero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments and tried to answer your questions.
4516636 to
31d9912
Compare
|
Thanks for the review @guggero and for answering my questions! I believe my latest update has addressed your comments, please let me know if I've missed anything. |
positiveblue
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
guggero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this out locally and found a few things that didn't work. Would be nice if you could do a quick manual test run before requesting review, please.
Other than that it looks very good, can get this merged soon I think.
dbd2381 to
1e600d5
Compare
|
Thank you for your latest review @guggero ! You're right I should always try out the code before I request a review. I'll make sure that I do. I've tried out the latest commit with account creation and it went as expected. Here's an example deposit label: And here's an example account creation label: Note Line 639 in 1e600d5
|
guggero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK, LGTM 🎉
This commit adds a new RPC endpoint which returns a per-account list of account modification fees.
1e600d5 to
cd63205
Compare
|
@guggero Thanks for the review. I've added You mentioned a "byte reverse order problem", what is that? |
By convention the ID of a transaction (a.k.a. TXID) is the little endian representation of its SHA256 hash. Which means the hex notation seen in most block explorers is actually the reverse of the hex representation of the hash. So in |
|
Thanks for the explanation! |
Pull Request Checklist
used.