Skip to content
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

Height in history pr #59

Merged
merged 2 commits into from
Oct 15, 2023
Merged

Conversation

dan-da
Copy link
Collaborator

@dan-da dan-da commented Oct 7, 2023

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:

  • a CLI wallet like the dashboard will be used by devs and power users
  • the height is a useful display field for me and probably others.

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
Copy link
Member

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
Copy link
Collaborator Author

dan-da commented Oct 8, 2023

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.

dan-da added a commit to dan-da/neptune-core that referenced this pull request Oct 11, 2023
@dan-da dan-da marked this pull request as ready for review October 11, 2023 17:57
@Sword-Smith
Copy link
Member

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

@Sword-Smith Sword-Smith self-requested a review October 14, 2023 10:45
@dan-da
Copy link
Collaborator Author

dan-da commented Oct 14, 2023

yes, I will rebase later today or tmw.

Returns `height` field from get_history() rpc and displays it in the
dashboard's history screen.
@dan-da dan-da force-pushed the height_in_history_pr branch from 5fc265e to 5a1f08c Compare October 14, 2023 22:40
@dan-da
Copy link
Collaborator Author

dan-da commented Oct 14, 2023

rebased, ready to merge.

@Sword-Smith
Copy link
Member

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?

@Sword-Smith Sword-Smith merged commit d157225 into Neptune-Crypto:master Oct 15, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants