Neptune-Crypto / neptune-core

anonymous peer-to-peer cash
Apache License 2.0
24 stars 7 forks source link

Height in history pr #59

Closed dan-da closed 10 months ago

dan-da commented 11 months ago

This is a draft PR as a starting point for discussion of the changes mentioned below.

There is no existing issue for this, just a small change I thought would be useful.

This builds on #56 (not yet merged) and adds a height field to data returned from get_history() rpc. This field is then displayed in the dashboard history screen. Example:

Screenshot_2023-10-07_13-51-34

It can be argued that height is not necessary to the wallet end-user and thus need not be displayed in dashboard history, but I would counter that:

The position of the height column could be changed if desired. I'm open to suggestions. Also it could be called "block", "block height", or "height". I chose the latter for now.

Even if we decide not to display height in the dashboard history screen, I think it is still useful to return it from the get_history() rpc, for future callers.

The block hash/digest is also available (returned from get_balance_history() and perhaps should be returned from get_history() as well. I think it might be useful for wallets, for example so they can create a URL to a block explorer for each block.

Even more useful would be a TX id/hash, which is typically displayed/used in history of cryptocurrency wallets. That is presently not returned from get_balance_history(). I'm not yet certain how to do that, as get_balance_history() iterates over monitored_utxos and I don't immediately see how to find the Transaction that a Utxo or MonitoredUtxo is part of.

So concretely then, before finalizing this PR, I would like to add fields for transaction_hash and block_hash to get_history() RPC. These would not be displayed in the dashboard history. I need some guidance how to get transaction_hash.

Sword-Smith commented 11 months ago

I like this change. Regarding the transaction hash, it might not really a useful value as each block only contains one transaction. So maybe transactions are better identified by block hash?

dan-da commented 11 months ago

Ok, interesting point. I've understood from the papers that all user initiated tx are merged into a single block TX, but maybe I haven't fully understood/internalized the ramifications yet. And I guess I just assumed there must be some tx identifier passed encrypted between sender and receiver.

I interpret your statement to mean there is no equivalent to bitcoin's "Transaction ID" available to wallets. Well, clearly a wallet could generate a unique TX hash when sending a TX, but then poof that individual tx is gone after it gets confirmed in a block, so all the wallet can find out about later is confirmed/spent UTXO's.

Just to confirm. If I initiated 5 separate spends, using say 12 utxo, and they all get confirmed in a single block -- is there any way for the wallet to know from the blockchain that it was 5 spends, or it will just appear as though the 12 utxo are part of a single tx?

I believe the answer is "single tx" and I can see how it is really good for privacy if a third party observer eg block explorer cannot identify individual TX.

In terms of my own wallet though, I'd like to be able to distinguish individual actions. I suppose the wallet could keep this as local metadata when spending (at least) but that info would be lost when restoring wallet from seed.

Regarding the transaction hash, it might not really a useful value as each block only contains one transaction. So maybe transactions are better identified by block hash?

Ok, well if there truly is no spend-level TX identifier, then yeah it seems that TX-id is a bit redundant. It is 1:1 with block hash anyway. There might be good reasons to include it that will become apparent later, but I agree we could leave it out for now.

It's fun to think about things in new ways.

Sword-Smith commented 10 months ago

Looks like there are some conflicts with master here. Probably from the merge of #56. Can you fix, @dan-da? :)

dan-da commented 10 months ago

yes, I will rebase later today or tmw.

dan-da commented 10 months ago

rebased, ready to merge.

Sword-Smith commented 10 months ago

Happy to approve and merge. On the subject of tx history presentation: Would it make sense to only show seconds, and not also milliseconds as we're doing now?