This repository has been archived by the owner on Jan 22, 2025. It is now read-only.
Accountsdb plugin transaction part 3: Transaction Notifier #21374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lijunwangs
commented
Nov 20, 2021
Changes to transaction_status_service to notify the notifier of the transaction data. Interface to query the plugin's interest in transaction data
lijunwangs
force-pushed
the
accountsdb_plugin_transaction_part_3.3
branch
from
4b354e9 to
fa7cda8
Compare
lijunwangs
requested review from
CriesofCarrots,
t-nelson,
carllin and
sakridge
Codecov Report
@@ Coverage Diff @@
## master #21374 +/- ##
=========================================
Coverage 81.5% 81.5%
=========================================
Files 500 505 +5
Lines 140604 141639 +1035
=========================================
+ Hits 114704 115549 +845
- Misses 25900 26090 +190 |
|
Might be good to include a comment describing the high level flow for the components/logic in this notification model. From what I gather it looks something like: |
carllin
reviewed
Nov 23, 2021
| /// Check if the plugin is interested in transaction data | ||
| /// Default is false -- if the plugin is not interested in | ||
| /// transaction data, please return false. | ||
| fn to_notify_transaction_data(&self) -> bool { |
There was a problem hiding this comment.
nit:
to_notify_transaction_data -> transaction_notifications_enabled
to_notify_account_data -> account_data_notifications_enabled
carllin
reviewed
Nov 23, 2021
Comment on lines
65
to
66
| let transaction_log_info = | ||
| Self::build_replica_transaction_info(signature, transaction_status_meta, transaction); |
There was a problem hiding this comment.
can move this outside of the self.plugin_manager.write().unwrap(); lock above
carllin
reviewed
Nov 23, 2021
| transaction_status_meta: &TransactionStatusMeta, | ||
| transaction: &SanitizedTransaction, | ||
| ) { | ||
| self.notify_transaction_info(slot, signature, transaction_status_meta, transaction); |
There was a problem hiding this comment.
seems like we could just move the body of notify_transaction_info inside here, the function signatures look exactly the same
carllin
reviewed
Nov 23, 2021
| ); | ||
| } | ||
|
|
||
| pub type TransactionNotifier = Arc<RwLock<dyn TransactionNotifierInterface + Sync + Send>>; |
There was a problem hiding this comment.
I think I prefer to be more explicit:
- TransactionNotifierInterface -> TransactinNotifier
- TransactionNotifier -> TransactionNotifierLock
carllin
approved these changes
Nov 23, 2021
dankelleher
pushed a commit
to identity-com/solana
that referenced
this pull request
Nov 24, 2021
…bs#21374) The TransactionNotifierInterface interface for notifying transactions. Changes to transaction_status_service to notify the notifier of the transaction data. Interface to query the plugin's interest in transaction data
frits-metalogix
added a commit
to identity-com/solana
that referenced
this pull request
Nov 24, 2021
…olana-labs#21374)" This reverts commit 53ea9b8.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
This enables transaction data to be notified through the plugin framework.
Summary of Changes
Fixes #
This is the 3rd part of original PR #21247