-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Populate memo in blockstore signatures-for-address #19515
Populate memo in blockstore signatures-for-address #19515
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19515 +/- ##
=========================================
- Coverage 82.7% 82.4% -0.3%
=========================================
Files 461 468 +7
Lines 131215 131675 +460
=========================================
+ Hits 108519 108628 +109
- Misses 22696 23047 +351 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will require a stubbed TransactionMemos column in v1.6, or backporting both memo-population PRs to v1.6. Preference?
A full backport seems worthwhile as this looks low risk and gets us nice memos in RPC sooner
However, it may still be worth plumbing a new AddressSignatures column sometime in the future to fix the column key to return signatures in proper execution order
Can you please remind me of the limitation here? Signatures from the same block aren't returned in chronological entry order?
@mvines , yes, that's right. The key on the AddressSignatures column is |
f2cbacc
to
6547d6f
Compare
6547d6f
to
e27024b
Compare
Thanks. This may become an issue with the confidential token transfer work that we're looking at, I'll make a note and we can revisit it soon-ish if so! |
* Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57) # Conflicts: # core/src/transaction_status_service.rs # ledger/src/blockstore.rs # transaction-status/src/extract_memos.rs
* Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57) # Conflicts: # ledger/src/blockstore.rs
…19605) * Populate memo in blockstore signatures-for-address (#19515) * Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57) # Conflicts: # ledger/src/blockstore.rs * Fix conflicts Co-authored-by: Tyera Eulberg <[email protected]> Co-authored-by: Tyera Eulberg <[email protected]>
* Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57)
* Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57)
* Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57)
* Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57)
* Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57)
* Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57)
* Add TransactionMemos column family * Traitify extract_memos * Write TransactionMemos in TransactionStatusService * Populate memos from column * Dedupe and add unit test (cherry picked from commit 5fa3e57) Co-authored-by: Tyera Eulberg <[email protected]>
Problem
cc #19512 Payment providers would like to use memos to annotate transactions, but wallets can't easily display these memos in the tx-history list.
ConfirmedTransactionStatusWithSignature
andRpcConfirmedTransactionStatusWithSignature
contain memo fields for this purpose, but these are not being populated.Summary of Changes
Store memos in new blockstore TransactionMemos column via the TransactionStatusService, and use it to populate the memo field for
getSignaturesForAddress
response objects that are queried from blockstore.This will require a stubbed TransactionMemos column in v1.6, or backporting both memo-population PRs to v1.6. Preference?
I also considered the alternate solution of a new AddressSignatures column to store the memo alongside each address, similar to the schema in bigtable, but ultimately decided that the very small perf savings in
Blockstore::get_confirmed_signatures_for_address2()
are not worth the additional disk to duplicate memos for every address. (However, it may still be worth plumbing a new AddressSignatures column sometime in the future to fix the column key to return signatures in proper execution order.)