wallet-rpc: switch the order of transactions returned to most recent #475
Conversation
| @@ -1232,7 +1232,7 @@ class RPC extends RPCBase { | |||
| const valid = new Validator(args); | |||
| let name = valid.str(0); | |||
| const count = valid.u32(1, 10); | |||
| const from = valid.u32(2, 0); | |||
| const skip = valid.u32(2, 0); | |||
There was a problem hiding this comment.
We should also change the argument name in the help message up on line 1228
There was a problem hiding this comment.
Good call, adding this in
| const to = Math.min(end, txs.length); | ||
| const start = txs.length - skip - 1; | ||
| const end = start - count; | ||
| const to = Math.max(end, -1); |
There was a problem hiding this comment.
I think I'd prefer a 0 here and a >= below
|
See bcoin-org/bcoin#605 -- this rpc call is already problematic for two reasons:
So although I think this PR is useful and makes the API match Bitcoin Core rpc listtransactions it should be noted that its suboptimal. Also, if we merge this we need to add a message to the CHANGELOG and update the api-docs. |
|
Following up here, I think without pagination, even these sight edits are somewhat useless when it comes to a wallet with a large amount of transactions. I'm currently seeing roughly 1-2 second response times for this RPC call, and so I think the next step would be to mimic bitcoin core and introduce some kind of pagination. I'll need to do some reading to see how they do it, and if it will require a change to how the transactions are indexed in the walletDB, but will report back here when I get the time to investigate. |
|
Be sure to check out bcoin #605, Braydon wrote pagination stuff already, it just never got reviewed or merged. |
|
@kilpatty should we close this and work on a deeper fix instead? |
|
I might actually pull the edits from bcoin #605 into this PR and rename. This is a relatively high priority for me as I need it for some internal software, so I'll try to get on it tonight and/or tomorrow. If you want me to just pop it into a new one happy to do that as well. EDIT: Also I noticed there are further improvements to pagination in bitcoin core recently so I will likely snag those in here as well. |
|
Ok cool, up to you on which branch you push to :-) |
nodech
added
quick
Re: discussion in the Telegram group. Looks like the intended behavior of this RPC call is to return the most recent transactions first, and then the "from/skip" parameter jumps ahead x number of txs.