Closed dan-da closed 10 months ago
I'm afraid Alan and I gave you some wrong information. The description in #6 says " The number of blocks (=confirmations) since the most recent balance update". If the last balance update is a outgoing transaction (UTXOs being spent), then any of the wallet's monitored UTXOs could have been spent, not just the last one. The monitored UTXOs are ordered by the blockheight at which they were received, and this ordering does not change. The get_balance_height
does actually return the relevant information that get_latest_balance_height
needs, as the returned list from get_balance_height
can be ordered by BlockHeight
and then the last element in that ordered list should contain the latest update. Sorry that we didn't catch that when you showed you suggestion in #6.
I would suggest:
BlockHeight
of latest balance update -- not latest incoming transaction as this PR's code does.get_latest_balance_height
in that.block_belongs_to_canonical_chain
in archival_state.rs
for this. If it does not, then find the next-most recent balance update etc. This point can also be addressed later, as you suggest, if you prefer thatu64
for blockheight should be more than enough. But why not just use the existing BlockHeight
struct that is a wrapped BFieldElement
which is a data structure that goes up to almost u64::MAX
($2^{64} - 2^{32} + 1$ to be precise.)
Fix logic to return BlockHeight of latest balance update -- not latest incoming transaction as this PR's code does.
Done. I think. Please review. ;-)
duh, whoops! nice catch on that. Now that you point it out, of course the list must be traversed to find latest spent utxo. I do wonder though if it could be possible to store the latest spent in the wallet to make this lookup o(1). Anyway I still tried to make the fn as efficient as possible given that it is called by the Dashboard overview. Maybe premature optimization, I dunno.
Write a test demonstrating correct behavior, or find an existing test that sets up the relevant states and add a call to get_latest_balance_height in that.
I didn't see such a test in wallet_state.rs. Perhaps I need to look elsewhere, or maybe you can point me to one. Anyway, I've run out of time tonight, so will look at that next coding session. We can keep the PR open.
Verify that the block in question belongs to the canonical chain
haven't looked into this yet. I will take a look and if it seems simple/clear and it works I will do it, else leave for a followup PR.
why not just use the existing BlockHeight struct
yes! That was my initial instinct, but I was a bit confused because the Dashboard was initially written to accept a u64 and BlockHeight::sub() returns i128. And I wasn't sure how pedantic we are being about BlockHeight vs say BlockCount. ie, a strict interpretation could say that a Height is only to be used in context of a count from block 0. Anyway, with your blessing I changed it to use BlockHeight everywhere in latest commit.
extra: I added an impl Display for Sign that just prints "-" if sign is negative. This helped me debug print a list of utxo's with amount and I figure it may be worth keeping, but happy to remove if you don't like it.
thx! super helpful comments. I'm away today, but will continue on it over the weekend.
I think this is ready for another review. See commit messages for summary of changes.
Addresses #6. (partial)
The major objective of this PR is adding the
get_confirmations
RPC. A secondary objective is displaying the confirmations field in overview screen of the Dashboard. Other changes are ancillary.Server:
implement get_confirmations
RPCget_dashboard_overview_data
RPCwallet_state::get_latest_balance_height()
block_height
to tuple returned fromwallet_state::get_balance_history
Dashboard:
usize
tou64
Notes:
get_confirmations()
RPC function has been simplified and made more efficient than the prototype here.get_latest_balance_height()
will need to be modified to take into account unsynced/abandoned utxo. There is already #28 for this, so I think it is best to do in a follow-up PR. Also, I'm not yet certain of the correct fix, as I did try to simply filter out byutxo.is_abandoned
inget_balance_history()
and that did not solve the balance/history sync issue. suggestions welcome.get_latest_balance_height()
and just calling itget_latest_balance()
. However the amount field requires an additional call+loop and since there is no present need, I decided to keep it as simple/fast as possible.block_height
field to values returned fromwallet_state::get_balance_history()
. That fn is no longer called fromget_confirmations()
however I left theblock_height
field in place because I think it would be useful to callers of theget_history()
RPC. I think it would be useful to display the height in Dashboard history, but decided to leave that change for a follow-up PR. Anyway, this note explains that diff.BlockHeight
type which the compiler treats asi128
. But it doesn't really make sense for a height to be signed. Also,i128
andu128
both seem like overkill... how many blocks are we anticipating? Isu64
not enough? The dashboard was previously usingusize
forconfirmations
field, which seems a bit loose as it could beu32
on some platform, so I changed it tou64
and also my reasoning is less data over the wire vsu128
. But I would rather just be consistent with a single type (based onu64
?) used everywhere to represent block heights. Perhaps the RPC and Dashboard should all be using theBlockHeight
forconfirmations
field rather than a primitive type. thoughts?Testing:
synced balance
synced balance
with top-most balance inHistory
screen. The history screen balance is higher thansynced balance
. This is wrong but expected until #28 is fixed.I wanted to create a unit test for
get_confirmations
RPC however it is unclear to me how to do so. There are a few unit tests in rpc_server.rs, but afaict none of them setup a wallet with a non-zero balance which would be a requirement for my test(s). If someone could provide such a setup fn, I would happily make use of it.