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

FS-154 New TransactionId Derivation #1001

Merged
merged 13 commits into from
Jul 19, 2024
Merged

Conversation

lnsiegel
Copy link
Contributor

@lnsiegel lnsiegel commented Jul 18, 2024

Motivation

transaction_log_ids created by full-service are used to lookup records in the transaction_log within the local wallet database.

Many third-party full-service clients expose this id to their users, expecting it to be the equivalent of other blockchain's transaction ids -- a hash that can be used to find evidence of the transaction on the blockchain itself.

Up until now, this has not been the case. While it was a 32 byte hash, represented as a 64 character hex string, the information being hashed included information private to the wallet database and so could not be used to find evidence of the transaction on the blockchain itself.

In this PR

This PR updates the algorithm for creating new transaction_log_ids (historical ids will remain unchanged) to use public blockchain information only. Specifically, we now use the public_key of one of the transaction's payload_txos as the transaction_log.id for newly created transaction_log entries. This will result in improved experiences for when the transaction_log.id is shared with end users and used by them to, for example, search for the transaction in the block-explorer.

Meanwhile clients that use the transaction_log.id to look up transaction_logs can still use it the same as always.

@holtzman holtzman changed the title Transaction id new derivation FS-154 New TransactionId Derivation Jul 18, 2024
Copy link
Collaborator

@holtzman holtzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 47.22222% with 19 lines in your changes missing coverage. Please review.

Project coverage is 55.57%. Comparing base (ab2af32) to head (d7606da).
Report is 203 commits behind head on main.

Files Patch % Lines
full-service/src/db/transaction_log.rs 60.71% 4 Missing and 7 partials ⚠️
full-service/src/json_rpc/v1/api/wallet.rs 0.00% 2 Missing and 2 partials ⚠️
full-service/src/json_rpc/v2/api/wallet.rs 0.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1001      +/-   ##
==========================================
- Coverage   60.12%   55.57%   -4.56%     
==========================================
  Files          88      125      +37     
  Lines       12356    16517    +4161     
  Branches     2010     2840     +830     
==========================================
+ Hits         7429     9179    +1750     
- Misses       3238     5234    +1996     
- Partials     1689     2104     +415     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lnsiegel lnsiegel merged commit d1a01f5 into main Jul 19, 2024
3 checks passed
@lnsiegel lnsiegel deleted the transaction-id-new-derivation branch July 19, 2024 00:33
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.

3 participants