-
Notifications
You must be signed in to change notification settings - Fork 49
feat(gRPC): Add transaction and read/write services for internal usage #8552
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
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 6 Skipped Deployments
|
0a5ec53
to
ebbe1d4
Compare
c62698b
to
707e542
Compare
b314d54
to
d6053fd
Compare
52aa127
to
89e7a56
Compare
726181b
to
83033fe
Compare
3050afb
to
8d40b0b
Compare
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.
Overall looks good. There are a few nits to improve.
fn indexes(&self) -> Option<&dyn RestIndexes>; | ||
|
||
/// Enable downcasting to concrete types for enhanced functionality | ||
fn as_any(&self) -> &dyn std::any::Any; |
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.
A weird method for a trait. Should RestStateReader
be extended with pub fn authority_state(&self) -> Arc<AuthorityState>
and pub fn load_epoch_store_one_call_per_task(&self) -> Arc<AuthorityPerEpochStore>
methods instead?
/// Get access to the underlying AuthorityState | ||
pub fn authority_state(&self) -> &Arc<AuthorityState> { | ||
&self.state | ||
} | ||
|
||
/// Load epoch store for transaction processing | ||
pub fn load_epoch_store_one_call_per_task(&self) -> Arc<AuthorityPerEpochStore> { | ||
self.state.load_epoch_store_one_call_per_task().clone() | ||
} |
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.
Why not move these methods and make them a part of RestStateReader
trait?
pub fn subscribe_events( | ||
&self, | ||
filter: EventFilter, | ||
) -> Box<dyn futures::Stream<Item = IotaEvent> + Send + Unpin> { |
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.
Should it be BoxStream<'_, IotaEvent>
? Or is Unpin
important here?
&self, | ||
filter: EventFilter, | ||
) -> Box<dyn futures::Stream<Item = IotaEvent> + Send + Unpin> { | ||
Box::new(Box::pin(self.event_streamer.subscribe(filter))) |
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.
Double Box
? Doesn't look right. With BoxStream
we can just use one Box::pin
.
) -> Box<dyn futures::Stream<Item = IotaTransactionBlockEffects> + Send + Unpin> { | ||
Box::new(Box::pin(self.transaction_streamer.subscribe(filter))) |
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.
Same here.
impl CheckpointService for CheckpointGrpcService { | ||
type StreamCheckpointsStream = | ||
Pin<Box<dyn futures::Stream<Item = Result<crate::checkpoint::Checkpoint, Status>> + Send>>; | ||
Pin<Box<dyn futures::Stream<Item = Result<Checkpoint, Status>> + Send>>; |
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.
Pin<Box<dyn futures::Stream<Item = Result<Checkpoint, Status>> + Send>>; | |
futures::BoxStream<Result<Checkpoint, Status>>; |
&self, | ||
request: Request<GetLatestCheckpointRequest>, | ||
) -> Result<Response<Checkpoint>, Status> { | ||
debug!("get_latest_checkpoint called"); |
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.
debug!("get_latest_checkpoint called"); |
request: Request<GetObjectRequest>, | ||
) -> Result<Response<GetObjectResponse>, Status> { | ||
let req = request.into_inner(); | ||
debug!("get_object called"); |
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.
debug!("get_object called"); |
#[tonic::async_trait] | ||
impl TransactionService for TransactionGrpcService { | ||
type StreamTransactionsStream = | ||
std::pin::Pin<Box<dyn futures::Stream<Item = Result<Transaction, Status>> + Send>>; |
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.
std::pin::Pin<Box<dyn futures::Stream<Item = Result<Transaction, Status>> + Send>>; | |
futures::BoxStream<Result<Transaction, Status>>; |
// Downcast to RestReadStore for enhanced functionality | ||
self.state_reader | ||
.as_any() | ||
.downcast_ref::<RestReadStore>() |
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 downcast suggests that the only type for the dyn object state_reader
is RestReadStore
, all other types are "fallback" or mock.
Description of change
GetLatestCheckpoint
,ExecuteTransaction
, andGetObject
.GetLatestCheckpoint
support get the latest checkpoint data or checkpoint summaryExecuteTransaction
is fully aligned with the current JSON-RPC design in IOTA and SUI (for the request, see the functionexecute_transaction_block
inpub trait WriteApi
; for the response, seeIOTATransactionBlockResponse
andSuiTransactionBlockResponse
, respectively.).iota-rest-api
, there is an additional fieldEffectsFinality
in the response, but it is not used in our codebase, so we do not implement this field.GetObject
is also aligned with the current JSON-RPC design in IOTA and SUI (see the functionget_object
in thepub trait ReadApi
for more details).IotaTransactionBlockEffects
,IotaTransactionBlockResponse
, andIotaObjectData
for transaction filter,ExecuteTransaction
, andGetObject
, respectively) we use serialized Json data for ease of usage and avoid additional effort, like we have the following for some of the fields. These annotations work for JSON but break BCS round-trips. In addition, to avoid repetitive work, we leveraged the existing types and functions (if available) in the JSON-RPC api.(NOTE: The above is deprecated, after discussion, we need to do bcs streaming for the response data, instead of using JSON string)
GrpcApiConfig
toiota-config
, and implemented thesubscribe_events
andsubscribe_transactions
inSubscriptionHandler
ofiota-node
directly, so as to resolve the cyclic dependency foriota-grpc-api
to useiota-core
.WriteService
andReadService
to ease of future extension of more API calls.GrpcStateReader
andRestStateReaderAdapter
iniota-grpc-api
crate, and leveraged the existing REST API infrastructure (including theRestStateReader
andRestReadStore
four our use cases). The plan is after REST API is deprecated, we can directly rename them to beGrpc*
, so as to avoid repetitive work in the future.RestStateReader
andRestReadStore
to accessAuthorityState
andAuthorityPerEpochStore
for usage inReadGrpcService
andWriteGrpcServce
. This is required to return more information like what JSON-RPC does.Links to any relevant issues
fixes #7724
How the change has been tested
Release Notes
GetLatestCheckpoint
,ExecuteTransaction
, andGetObject
.