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

feat(sdk)!: return consensus errors from broadcast methods #2274

Merged
merged 10 commits into from
Oct 29, 2024
56 changes: 22 additions & 34 deletions packages/rs-dapi-client/src/dapi_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use tracing::Instrument;

use crate::address_list::AddressListError;
use crate::connection_pool::ConnectionPool;
use crate::transport::TransportError;
use crate::{
transport::{TransportClient, TransportRequest},
Address, AddressList, CanRetry, RequestSettings,
Expand All @@ -18,11 +19,12 @@ use crate::{
/// General DAPI request error type.
#[derive(Debug, thiserror::Error)]
#[cfg_attr(feature = "mocks", derive(serde::Serialize, serde::Deserialize))]
pub enum DapiClientError<TE: Mockable> {
pub enum DapiClientError {
/// The error happened on transport layer
#[error("transport error with {1}: {0}")]
Transport(
#[cfg_attr(feature = "mocks", serde(with = "dapi_grpc::mock::serde_mockable"))] TE,
#[cfg_attr(feature = "mocks", serde(with = "dapi_grpc::mock::serde_mockable"))]
TransportError,
Address,
),
/// There are no valid DAPI addresses to use.
Expand All @@ -38,7 +40,7 @@ pub enum DapiClientError<TE: Mockable> {
Mock(#[from] crate::mock::MockError),
}

impl<TE: CanRetry + Mockable> CanRetry for DapiClientError<TE> {
impl CanRetry for DapiClientError {
fn can_retry(&self) -> bool {
use DapiClientError::*;
match self {
Expand All @@ -51,17 +53,10 @@ impl<TE: CanRetry + Mockable> CanRetry for DapiClientError<TE> {
}
}

#[cfg(feature = "mocks")]
#[derive(serde::Serialize, serde::Deserialize)]
struct TransportErrorData {
transport_error: Vec<u8>,
address: Address,
}

/// Serialization of [DapiClientError].
///
/// We need to do manual serialization because of the generic type parameter which doesn't support serde derive.
impl<TE: Mockable> Mockable for DapiClientError<TE> {
impl Mockable for DapiClientError {
#[cfg(feature = "mocks")]
fn mock_serialize(&self) -> Option<Vec<u8>> {
Some(serde_json::to_vec(self).expect("serialize DAPI client error"))
Expand All @@ -81,11 +76,11 @@ pub trait DapiRequestExecutor {
&self,
request: R,
settings: RequestSettings,
) -> Result<R::Response, DapiClientError<<R::Client as TransportClient>::Error>>
) -> Result<R::Response, DapiClientError>
where
R: TransportRequest + Mockable,
R::Response: Mockable,
<R::Client as TransportClient>::Error: Mockable;
TransportError: Mockable;
}

/// Access point to DAPI.
Expand Down Expand Up @@ -126,11 +121,11 @@ impl DapiRequestExecutor for DapiClient {
&self,
request: R,
settings: RequestSettings,
) -> Result<R::Response, DapiClientError<<R::Client as TransportClient>::Error>>
) -> Result<R::Response, DapiClientError>
where
R: TransportRequest + Mockable,
R::Response: Mockable,
<R::Client as TransportClient>::Error: Mockable,
TransportError: Mockable,
{
// Join settings of different sources to get final version of the settings for this execution:
let applied_settings = self
Expand Down Expand Up @@ -163,9 +158,10 @@ impl DapiRequestExecutor for DapiClient {
.read()
.expect("can't get address list for read");

let address_result = address_list.get_live_address().cloned().ok_or(
DapiClientError::<<R::Client as TransportClient>::Error>::NoAvailableAddresses,
);
let address_result = address_list
.get_live_address()
.cloned()
.ok_or(DapiClientError::NoAvailableAddresses);
Comment on lines +148 to +151
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle NoAvailableAddresses error correctly

The change at lines 148-151 introduces DapiClientError::NoAvailableAddresses when there are no live addresses. Verify that this error is handled appropriately in calling code, and that user-facing error messages are clear when this condition occurs.

Consider adding user-friendly messages or logging to help diagnose issues when no addresses are available, enhancing the developer experience during debugging.


drop(address_list);

Expand Down Expand Up @@ -200,22 +196,12 @@ impl DapiRequestExecutor for DapiClient {
&applied_settings,
&pool,
)
.map_err(|e| {
DapiClientError::<<R::Client as TransportClient>::Error>::Transport(
e,
address.clone(),
)
})?;
.map_err(|e| DapiClientError::Transport(e, address.clone()))?;

let response = transport_request
.execute_transport(&mut transport_client, &applied_settings)
.await
.map_err(|e| {
DapiClientError::<<R::Client as TransportClient>::Error>::Transport(
e,
address.clone(),
)
});
.map_err(|e| DapiClientError::Transport(e, address.clone()));

match &response {
Ok(_) => {
Expand All @@ -226,8 +212,9 @@ impl DapiRequestExecutor for DapiClient {
.write()
.expect("can't get address list for write");

address_list.unban_address(&address)
.map_err(DapiClientError::<<R::Client as TransportClient>::Error>::AddressList)?;
address_list
.unban_address(&address)
.map_err(DapiClientError::AddressList)?;
}

tracing::trace!(?response, "received {} response", response_name);
Expand All @@ -240,8 +227,9 @@ impl DapiRequestExecutor for DapiClient {
.write()
.expect("can't get address list for write");

address_list.ban_address(&address)
.map_err(DapiClientError::<<R::Client as TransportClient>::Error>::AddressList)?;
address_list
.ban_address(&address)
.map_err(DapiClientError::AddressList)?;
}
} else {
tracing::trace!(?error, "received error");
Expand Down
11 changes: 3 additions & 8 deletions packages/rs-dapi-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ pub use address_list::AddressList;
pub use connection_pool::ConnectionPool;
pub use dapi_client::DapiRequestExecutor;
pub use dapi_client::{DapiClient, DapiClientError};
use dapi_grpc::mock::Mockable;
#[cfg(feature = "dump")]
pub use dump::DumpData;
use futures::{future::BoxFuture, FutureExt};
Expand All @@ -34,21 +33,19 @@ pub use request_settings::RequestSettings;
/// let mut client = MockDapiClient::new();
/// let request: proto::GetIdentityRequest = proto::get_identity_request::GetIdentityRequestV0 { id: b"0".to_vec(), prove: true }.into();
/// let response = request.execute(&mut client, RequestSettings::default()).await?;
/// # Ok::<(), DapiClientError<_>>(())
/// # Ok::<(), DapiClientError>(())
/// # };
/// ```
pub trait DapiRequest {
/// Response from DAPI for this specific request.
type Response;
/// An error type for the transport this request uses.
type TransportError: Mockable;

/// Executes the request.
fn execute<'c, D: DapiRequestExecutor>(
self,
dapi_client: &'c D,
settings: RequestSettings,
) -> BoxFuture<'c, Result<Self::Response, DapiClientError<Self::TransportError>>>
) -> BoxFuture<'c, Result<Self::Response, DapiClientError>>
shumkov marked this conversation as resolved.
Show resolved Hide resolved
where
Self: 'c;
}
Expand All @@ -57,13 +54,11 @@ pub trait DapiRequest {
impl<T: transport::TransportRequest + Send> DapiRequest for T {
type Response = T::Response;

type TransportError = <T::Client as transport::TransportClient>::Error;

fn execute<'c, D: DapiRequestExecutor>(
self,
dapi_client: &'c D,
settings: RequestSettings,
) -> BoxFuture<'c, Result<Self::Response, DapiClientError<Self::TransportError>>>
) -> BoxFuture<'c, Result<Self::Response, DapiClientError>>
where
Self: 'c,
{
Expand Down
5 changes: 1 addition & 4 deletions packages/rs-dapi-client/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
//! See `tests/mock_dapi_client.rs` for an example.

use crate::{
transport::{TransportClient, TransportRequest},

Check warning on line 15 in packages/rs-dapi-client/src/mock.rs

View workflow job for this annotation

GitHub Actions / Rust packages (dash-sdk) / Linting

unused import: `TransportClient`

warning: unused import: `TransportClient` --> packages/rs-dapi-client/src/mock.rs:15:17 | 15 | transport::{TransportClient, TransportRequest}, | ^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default

Check warning on line 15 in packages/rs-dapi-client/src/mock.rs

View workflow job for this annotation

GitHub Actions / Rust packages (rs-dapi-client) / Linting

unused import: `TransportClient`

warning: unused import: `TransportClient` --> packages/rs-dapi-client/src/mock.rs:15:17 | 15 | transport::{TransportClient, TransportRequest}, | ^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default
DapiClientError, DapiRequestExecutor, RequestSettings,
};
use dapi_grpc::mock::Mockable;
Expand All @@ -35,10 +35,7 @@
expectations: Expectations,
}
/// Result of executing a mock request
pub type MockResult<T> = Result<
<T as TransportRequest>::Response,
DapiClientError<<<T as TransportRequest>::Client as TransportClient>::Error>,
>;
pub type MockResult<T> = Result<<T as TransportRequest>::Response, DapiClientError>;

impl MockDapiClient {
/// Create a new mock client
Expand Down Expand Up @@ -77,7 +74,7 @@
///
/// Panics if the file can't be read or the data can't be parsed.
#[cfg(feature = "dump")]
pub fn load<T: TransportRequest, P: AsRef<std::path::Path>>(

Check warning on line 77 in packages/rs-dapi-client/src/mock.rs

View workflow job for this annotation

GitHub Actions / Rust packages (rs-dapi-client) / Linting

bound is defined in more than one place

warning: bound is defined in more than one place --> packages/rs-dapi-client/src/mock.rs:77:17 | 77 | pub fn load<T: TransportRequest, P: AsRef<std::path::Path>>( | ^ ... 82 | T: Mockable, | ^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#multiple_bound_locations = note: `#[warn(clippy::multiple_bound_locations)]` on by default
&mut self,
file: P,
) -> Result<(T, MockResult<T>), std::io::Error>
Expand Down
45 changes: 39 additions & 6 deletions packages/rs-dapi-client/src/transport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,54 @@ pub trait TransportRequest: Clone + Send + Sync + Debug + Mockable {
self,
client: &'c mut Self::Client,
settings: &AppliedRequestSettings,
) -> BoxFuture<'c, Result<Self::Response, <Self::Client as TransportClient>::Error>>;
) -> BoxFuture<'c, Result<Self::Response, TransportError>>;
}

/// Transport error type.
#[derive(Debug, thiserror::Error)]
#[cfg_attr(feature = "mocks", derive(serde::Serialize, serde::Deserialize))]
pub enum TransportError {
/// gRPC error
#[error("grpc error: {0}")]
Grpc(
#[from]
#[cfg_attr(feature = "mocks", serde(with = "dapi_grpc::mock::serde_mockable"))]
dapi_grpc::tonic::Status,
),
}

impl CanRetry for TransportError {
fn can_retry(&self) -> bool {
match self {
TransportError::Grpc(status) => status.can_retry(),
}
}
}

/// Serialization of [TransportError].
///
/// We need to do manual serialization because of the generic type parameter which doesn't support serde derive.
impl Mockable for TransportError {
#[cfg(feature = "mocks")]
fn mock_serialize(&self) -> Option<Vec<u8>> {
Some(serde_json::to_vec(self).expect("serialize Transport error"))
}

#[cfg(feature = "mocks")]
fn mock_deserialize(data: &[u8]) -> Option<Self> {
Some(serde_json::from_slice(data).expect("deserialize Transport error"))
}
}

/// Generic way to create a transport client from provided [Uri].
pub trait TransportClient: Send + Sized {
/// Error type for the specific client.
type Error: CanRetry + Send + Debug + Mockable;

/// Build client using node's url.
fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, Self::Error>;
fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, TransportError>;

/// Build client using node's url and [AppliedRequestSettings].
fn with_uri_and_settings(
uri: Uri,
settings: &AppliedRequestSettings,
pool: &ConnectionPool,
) -> Result<Self, Self::Error>;
) -> Result<Self, TransportError>;
}
17 changes: 7 additions & 10 deletions packages/rs-dapi-client/src/transport/grpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use std::time::Duration;

use super::{CanRetry, TransportClient, TransportRequest};
use super::{CanRetry, TransportClient, TransportError, TransportRequest};
use crate::connection_pool::{ConnectionPool, PoolPrefix};
use crate::{request_settings::AppliedRequestSettings, RequestSettings};
use dapi_grpc::core::v0::core_client::CoreClient;
Expand Down Expand Up @@ -39,9 +39,7 @@ fn create_channel(
}

impl TransportClient for PlatformGrpcClient {
type Error = dapi_grpc::tonic::Status;

fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, Self::Error> {
fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, TransportError> {
Ok(pool
.get_or_create(PoolPrefix::Platform, &uri, None, || {
match create_channel(uri.clone(), None) {
Expand All @@ -59,7 +57,7 @@ impl TransportClient for PlatformGrpcClient {
uri: Uri,
settings: &AppliedRequestSettings,
pool: &ConnectionPool,
) -> Result<Self, Self::Error> {
) -> Result<Self, TransportError> {
Ok(pool
.get_or_create(
PoolPrefix::Platform,
Expand All @@ -78,9 +76,7 @@ impl TransportClient for PlatformGrpcClient {
}

impl TransportClient for CoreGrpcClient {
type Error = dapi_grpc::tonic::Status;

fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, Self::Error> {
fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, TransportError> {
Ok(pool
.get_or_create(PoolPrefix::Core, &uri, None, || {
match create_channel(uri.clone(), None) {
Expand All @@ -98,7 +94,7 @@ impl TransportClient for CoreGrpcClient {
uri: Uri,
settings: &AppliedRequestSettings,
pool: &ConnectionPool,
) -> Result<Self, Self::Error> {
) -> Result<Self, TransportError> {
Ok(pool
.get_or_create(
PoolPrefix::Core,
Expand Down Expand Up @@ -155,7 +151,7 @@ macro_rules! impl_transport_request_grpc {
self,
client: &'c mut Self::Client,
settings: &AppliedRequestSettings,
) -> BoxFuture<'c, Result<Self::Response, <Self::Client as TransportClient>::Error>>
) -> BoxFuture<'c, Result<Self::Response, TransportError>>
{
let mut grpc_request = self.into_request();

Expand All @@ -165,6 +161,7 @@ macro_rules! impl_transport_request_grpc {

client
.$($method)+(grpc_request)
.map_err(TransportError::Grpc)
.map_ok(|response| response.into_inner())
.boxed()
}
Expand Down
30 changes: 22 additions & 8 deletions packages/rs-sdk/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
//! Definitions of errors
use std::fmt::Debug;
use std::time::Duration;

use dapi_grpc::mock::Mockable;
use dapi_grpc::tonic;
use dpp::consensus::ConsensusError;
use dpp::serialization::PlatformDeserializable;
use dpp::version::PlatformVersionError;
use dpp::ProtocolError;
use rs_dapi_client::{CanRetry, DapiClientError};

pub use drive_proof_verifier::error::ContextProviderError;
use rs_dapi_client::transport::TransportError;
use rs_dapi_client::{CanRetry, DapiClientError};
use std::fmt::Debug;
use std::time::Duration;

/// Error type for the SDK
#[derive(Debug, thiserror::Error)]
Expand Down Expand Up @@ -73,8 +74,21 @@ pub enum Error {
StaleNode(#[from] StaleNodeError),
}

impl<T: Debug + Mockable> From<DapiClientError<T>> for Error {
fn from(value: DapiClientError<T>) -> Self {
// TODO: Return a more specific errors like connection, node error instead of DAPI client error
impl From<DapiClientError> for Error {
fn from(value: DapiClientError) -> Self {
if let DapiClientError::Transport(TransportError::Grpc(status), _) = &value {
if let Some(consensus_error_value) = status.metadata().get_bin("drive-error-data-bin") {
return ConsensusError::deserialize_from_bytes(
consensus_error_value.as_encoded_bytes(),
)
.map(|consensus_error| {
Self::Protocol(ProtocolError::ConsensusError(Box::new(consensus_error)))
})
.unwrap_or_else(Self::Protocol);
shumkov marked this conversation as resolved.
Show resolved Hide resolved
}
}

Self::DapiClientError(format!("{:?}", value))
}
}
Expand Down
6 changes: 4 additions & 2 deletions packages/rs-sdk/src/platform/delegate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
//! Each of these enums has a variant for each request/response pair. Name of variant in request enum is the same as
//! the name of variant in response.

use rs_dapi_client::transport::TransportError;

/// Delegate the execution of a transport request to the appropriate variant of an enum wrapper.
///
/// Given two enums, request and response, that wrap multiple requests/responses for one object type, this macro
Expand Down Expand Up @@ -43,11 +45,11 @@ macro_rules! delegate_transport_request_variant {
self,
client: &'c mut Self::Client,
settings: &$crate::platform::dapi::transport::AppliedRequestSettings,
) -> $crate::platform::dapi::transport::BoxFuture<'c, Result<Self::Response, <Self::Client as $crate::platform::dapi::transport::TransportClient>::Error>> {
) -> $crate::platform::dapi::transport::BoxFuture<'c, Result<Self::Response, TransportError>> {
use futures::FutureExt;
use $request::*;

let settings =settings.clone();
let settings = settings.clone();

// We need to build new async box because we have to map response to the $response type
match self {$(
Expand Down
Loading
Loading