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)!: support errors in mock #2277

Open
wants to merge 55 commits into
base: v1.6-dev-ugly
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 49 commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
7a77ccf
feat(sdk): provide request execution information
shumkov Oct 21, 2024
b85f52e
test: fix compilation errors
shumkov Oct 21, 2024
6de23ab
refactor: rename `unwrap` to `into_inner`
shumkov Oct 21, 2024
fba6ef8
refactor: rename response to inner
shumkov Oct 21, 2024
8af6776
test: increase timeout to pass the test
shumkov Oct 21, 2024
8e278e3
fix: `into_inner` doesn't need `CanRetry` bound
shumkov Oct 21, 2024
5af8822
fix: errored address is none
shumkov Oct 21, 2024
8269c1f
Merge branch 'v1.4-dev' into feat/sdk/execution_info
shumkov Oct 22, 2024
a9a96a0
chore: don't use empty uri
shumkov Oct 22, 2024
920b50d
docs: fix usage example
shumkov Oct 22, 2024
067db87
refactor: remove unused import
shumkov Oct 22, 2024
c684b3f
chore: update test vectors
shumkov Oct 22, 2024
9da89a8
chore: more test vectors
shumkov Oct 22, 2024
34ffdfb
feat(sdk): retry failed requests on sdk level
lklimek Oct 21, 2024
60d220c
fix(dapi-client): impl VersionedGrpcResponse for ExecutionResponse
lklimek Oct 22, 2024
80d19e0
feat(sdk): fech with retries
lklimek Oct 23, 2024
f78555e
chore(sdk): remove unnecessary changes
lklimek Oct 24, 2024
4343b6e
fix(sdk): correct retry logic
lklimek Oct 24, 2024
50ad84c
feat(sdk): retry impl
lklimek Oct 24, 2024
e4e667a
chore: self-review
lklimek Oct 24, 2024
4bdf0cd
refactor(sdk): mocks accept errors
lklimek Oct 22, 2024
5eac233
chore: testing
lklimek Oct 23, 2024
dfaaa4c
feat(sdk): retry impl
lklimek Oct 24, 2024
ac388a8
chore: apply feedback
lklimek Oct 25, 2024
016450f
chore: remove outdated comment
lklimek Oct 25, 2024
0738f76
chore(sdk): impl<T> From<ExecutionError<T>> for Error
lklimek Oct 25, 2024
438185f
doc(sdk): comment on sync::retry() logic
lklimek Oct 25, 2024
170d5a9
chore: self review
lklimek Oct 25, 2024
e20a29d
refactor: remove unused code
lklimek Oct 25, 2024
c9c6c2f
chore: fmt
lklimek Oct 25, 2024
e5e17a7
chore: doc
lklimek Oct 25, 2024
dace7fa
test: fix retry test
lklimek Oct 25, 2024
eca69f8
refactor(sdk): apply feedback
lklimek Oct 25, 2024
df706f4
test(sdk): add edge case to test_retry
lklimek Oct 25, 2024
1f954d5
fix: use settings in fetch_many
lklimek Oct 25, 2024
034bdb2
test(sdk): regenerate test vectors
lklimek Oct 28, 2024
1bc31a9
refactor: re-export specific types
shumkov Oct 28, 2024
bb21a21
fix(dapi-client): correcrlt serialize ExecutionResult and ExecutionEr…
lklimek Oct 28, 2024
5a497ca
Merge remote-tracking branch 'origin/feat/sdk/execution_info' into fe…
lklimek Oct 28, 2024
723b1c5
chore: fixes after merge
lklimek Oct 28, 2024
098f854
Merge remote-tracking branch 'origin/feat/sdk-retry' into refactor/sd…
lklimek Oct 28, 2024
88dda82
chore: fixes after merge + add InnerInto impl to mocks
lklimek Oct 28, 2024
cebc709
Merge remote-tracking branch 'origin/v1.4-dev' into feat/sdk-retry
lklimek Oct 28, 2024
a0b2427
Merge branch 'feat/sdk-retry' into refactor/sdk-mock-errors
lklimek Oct 28, 2024
4f2bb21
fix(sdk): option not serialized properly (work in progress)
lklimek Oct 28, 2024
100147b
build(deps): optimize mocks feature flag
lklimek Oct 29, 2024
38f073a
refactor(dapi-grpc): no default Mockable impl
lklimek Oct 29, 2024
bd7499d
chore: minor linter fixes
lklimek Oct 29, 2024
98bebee
Merge remote-tracking branch 'origin/v1.4-dev' into refactor/sdk-mock…
lklimek Oct 29, 2024
65cdabd
chore: test vector configs
lklimek Oct 29, 2024
e5f827c
chore: remove unneeded import
lklimek Oct 29, 2024
905955e
chore: typos
lklimek Oct 29, 2024
40963dc
chore: apply review feedback
lklimek Oct 30, 2024
bf07db8
Merge remote-tracking branch 'origin/v1.4-dev' into refactor/sdk-mock…
lklimek Oct 30, 2024
c1f972b
chore: regenerate test vectors
lklimek Oct 30, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 93 additions & 9 deletions packages/dapi-grpc/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,18 @@ where
/// # Panics
///
/// Panics on any error.
fn mock_serialize(&self) -> Option<Vec<u8>> {
None
}
#[cfg(feature = "mocks")]
fn mock_serialize(&self) -> Option<Vec<u8>>;

/// Deserialize the message serialized with [mock_serialize()].
///
/// Returns None if the message is not serializable or mocking is disabled.
///
/// # Panics
///
/// Panics on any error.
fn mock_deserialize(_data: &[u8]) -> Option<Self> {
None
}
#[cfg(feature = "mocks")]
fn mock_deserialize(_data: &[u8]) -> Option<Self>;
}
#[cfg(feature = "mocks")]
#[derive(serde::Serialize, serde::Deserialize)]
Expand Down Expand Up @@ -77,12 +76,37 @@ where
impl<T: Mockable> Mockable for Option<T> {
#[cfg(feature = "mocks")]
fn mock_serialize(&self) -> Option<Vec<u8>> {
self.as_ref().and_then(|value| value.mock_serialize())
let data = match self {
None => vec![0],
Some(value) => {
let mut data = vec![1]; // we return None if value does not support serialization
let mut serialized = value.mock_serialize()?;
data.append(&mut serialized);

data
}
};

Some(data)
}

#[cfg(feature = "mocks")]
fn mock_deserialize(data: &[u8]) -> Option<Self> {
T::mock_deserialize(data).map(Some)
if data.is_empty() {
panic!("empty data");
}

match data[0] {
0 => Some(None),
// Mind double Some - first says mock_deserialize is implemented, second is deserialized value
1 => Some(Some(
T::mock_deserialize(&data[1..]).expect("unable to deserialize Option<T>"),
)),
_ => panic!(
"unsupported first byte for Option<T>::mock_deserialize: {:x}",
data[0]
),
}
lklimek marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -97,6 +121,29 @@ impl Mockable for Vec<u8> {
serde_json::from_slice(data).ok()
}
}

impl<T: Mockable> Mockable for Vec<T> {
#[cfg(feature = "mocks")]
fn mock_serialize(&self) -> Option<Vec<u8>> {
let data: Vec<Vec<u8>> = self
.iter()
.map(|d| d.mock_serialize())
.collect::<Option<Vec<Vec<u8>>>>()?;

Some(serde_json::to_vec(&data).expect("unable to serialize Vec<T>"))
}

#[cfg(feature = "mocks")]
fn mock_deserialize(data: &[u8]) -> Option<Self> {
let data: Vec<Vec<u8>> =
serde_json::from_slice(data).expect("unable to deserialize Vec<T>");

data.into_iter()
.map(|d| T::mock_deserialize(&d))
.collect::<Option<Vec<T>>>()
}
}
lklimek marked this conversation as resolved.
Show resolved Hide resolved

#[cfg(feature = "mocks")]
#[derive(serde::Serialize, serde::Deserialize)]
struct MockableStatus {
Expand Down Expand Up @@ -127,4 +174,41 @@ impl Mockable for crate::tonic::Status {
///
/// This will return `None` on serialization,
/// effectively disabling mocking of streaming responses.
impl<T: Mockable> Mockable for Streaming<T> {}
impl<T: Mockable> Mockable for Streaming<T> {
#[cfg(feature = "mocks")]
fn mock_serialize(&self) -> Option<Vec<u8>> {
unimplemented!("mocking of streaming messages is not supported")
}

#[cfg(feature = "mocks")]
fn mock_deserialize(_data: &[u8]) -> Option<Self> {
unimplemented!("mocking of streaming messages is not supported")
}
}

/// Mocking of primitive types - just serialize them as little-endian bytes.
///
/// This is useful for mocking of messages that contain primitive types.
macro_rules! mockable_number {
($($t:ty),*) => {
$(
impl Mockable for $t {
#[cfg(feature = "mocks")]
fn mock_serialize(&self) -> Option<Vec<u8>> {
(*self).to_le_bytes().to_vec().mock_serialize()
}

#[cfg(feature = "mocks")]
fn mock_deserialize(data: &[u8]) -> Option<Self> {
let data: Vec<u8> = Mockable::mock_deserialize(data)?;
Some(Self::from_le_bytes(
data.try_into().expect("invalid serialized data"),
))
}
}
)*
};
}

// No `u8` as it would cause conflict between Vec<u8> and Vec<T: Mockable> impls.
mockable_number!(usize, u16, u32, u64, u128, isize, i8, i16, i32, i64, i128);
lklimek marked this conversation as resolved.
Show resolved Hide resolved
13 changes: 13 additions & 0 deletions packages/rs-dapi-client/src/address_list.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Subsystem to manage DAPI nodes.

use chrono::Utc;
use dapi_grpc::mock::Mockable;
use dapi_grpc::tonic::codegen::http;
use dapi_grpc::tonic::transport::Uri;
use rand::{rngs::SmallRng, seq::IteratorRandom, SeedableRng};
Expand Down Expand Up @@ -59,6 +60,18 @@ impl From<Uri> for Address {
}
}

impl Mockable for Address {
#[cfg(feature = "mocks")]
fn mock_serialize(&self) -> Option<Vec<u8>> {
Some(serde_json::to_vec(self).expect("unable to serialize Address"))
}

#[cfg(feature = "mocks")]
fn mock_deserialize(data: &[u8]) -> Option<Self> {
Some(serde_json::from_slice(data).expect("unable to deserialize Address"))
}
}
lklimek marked this conversation as resolved.
Show resolved Hide resolved

impl Address {
/// Ban the [Address] so it won't be available through [AddressList::get_live_address] for some time.
fn ban(&mut self, base_ban_period: &Duration) {
Expand Down
5 changes: 3 additions & 2 deletions packages/rs-dapi-client/src/dapi_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@

use crate::address_list::AddressListError;
use crate::connection_pool::ConnectionPool;
use crate::transport::TransportError;
#[cfg(feature = "mocks")]
use crate::Address;

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

View workflow job for this annotation

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

unused import: `crate::Address`

warning: unused import: `crate::Address` --> packages/rs-dapi-client/src/dapi_client.rs:15:5 | 15 | use crate::Address; | ^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default

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

View workflow job for this annotation

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

unused import: `crate::Address`

warning: unused import: `crate::Address` --> packages/rs-dapi-client/src/dapi_client.rs:15:5 | 15 | use crate::Address; | ^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default
lklimek marked this conversation as resolved.
Show resolved Hide resolved
use crate::{
transport::{TransportClient, TransportRequest},
transport::{TransportClient, TransportError, TransportRequest},
AddressList, CanRetry, DapiRequestExecutor, ExecutionError, ExecutionResponse, ExecutionResult,
RequestSettings,
};
Expand Down
2 changes: 2 additions & 0 deletions packages/rs-dapi-client/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ pub trait InnerInto<T> {

/// Error happened during request execution.
#[derive(Debug, Clone, thiserror::Error, Eq, PartialEq)]
#[cfg_attr(feature = "mocks", derive(serde::Serialize, serde::Deserialize))]
lklimek marked this conversation as resolved.
Show resolved Hide resolved
#[error("{inner}")]
pub struct ExecutionError<E> {
/// The cause of error
Expand Down Expand Up @@ -77,6 +78,7 @@ impl<E: CanRetry> CanRetry for ExecutionError<E> {

/// Request execution response.
#[derive(Debug, Clone, Eq, PartialEq)]
#[cfg_attr(feature = "mocks", derive(serde::Serialize, serde::Deserialize))]
pub struct ExecutionResponse<R> {
/// The response from the request
pub inner: R,
Expand Down
79 changes: 65 additions & 14 deletions packages/rs-dapi-client/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,32 +257,83 @@ impl Expectations {

impl<R: Mockable> Mockable for ExecutionResponse<R> {
Copy link
Member

Choose a reason for hiding this comment

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

We need to decided, either we keep mockable implementation in their modules, like address, or in mock, like execution types. I would say they should be in their modules otherwise the mock module will be a mess eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be refactored in separate PR.

fn mock_serialize(&self) -> Option<Vec<u8>> {
R::mock_serialize(&self.inner)
// We encode data as vec![inner, address, retries] where each od them is serialized to bytes
let ser: Vec<Vec<u8>> = vec![
self.inner
.mock_serialize()
.expect("unable to serialize ExecutionResponse inner"),
self.address
.mock_serialize()
.expect("unable to serialize ExecutionResponse address"),
(self.retries as u64)
.mock_serialize()
.expect("unable to serialize ExecutionResponse retries"),
];

ser.mock_serialize()
}

fn mock_deserialize(data: &[u8]) -> Option<Self> {
// TODO: We need serialize retries and address too
R::mock_deserialize(data).map(|inner| ExecutionResponse {
inner,
retries: 0,
address: "http://127.0.0.1:9000"
.parse()
.expect("failed to parse address"),
let deser: Vec<Vec<u8>> =
Mockable::mock_deserialize(data).expect("unable to deserialize ExecutionResponse");

let [inner, address, retries] = &deser[0..] else {
// panics intentionally, as this is just for mocking, and we know these types can be mocked
// because they were serialized somehow :)
panic!(
"invalid ExecutionResponse data: expected 3 elements, got {}",
deser.len()
);
};
lklimek marked this conversation as resolved.
Show resolved Hide resolved

Some(Self {
inner: Mockable::mock_deserialize(inner)
.expect("unable to deserialize ExecutionResponse inner"),
address: Mockable::mock_deserialize(address)
.expect("unable to deserialize ExecutionResponse address"),
retries: Mockable::mock_deserialize(retries)
.expect("unable to deserialize ExecutionResponse retries"),
lklimek marked this conversation as resolved.
Show resolved Hide resolved
})
}
}

impl<E: Mockable> Mockable for ExecutionError<E> {
fn mock_serialize(&self) -> Option<Vec<u8>> {
E::mock_serialize(&self.inner)
// We encode data as vec![inner, address, retries] where each od them is serialized to bytes
let ser: Vec<Vec<u8>> = vec![
self.inner
.mock_serialize()
.expect("unable to serialize ExecutionError inner"),
self.address
.mock_serialize()
.expect("unable to serialize ExecutionError address"),
self.retries
.mock_serialize()
.expect("unable to serialize ExecutionError retries"),
];

ser.mock_serialize()
}

fn mock_deserialize(data: &[u8]) -> Option<Self> {
// TODO: We need serialize retries and address too
E::mock_deserialize(data).map(|inner| ExecutionError {
inner,
retries: 0,
address: None,
let deser: Vec<Vec<u8>> =
Mockable::mock_deserialize(data).expect("unable to deserialize ExecutionError");

let [inner, address, retries] = &deser[0..] else {
// panics intentionally, as this is just for mocking, and we know these types can be mocked because they were
// serialized before
panic!(
"invalid ExecutionError data: expected 3 elements, got {}",
deser.len()
);
};
Some(Self {
inner: Mockable::mock_deserialize(inner)
.expect("unable to deserialize ExecutionError inner"),
address: Mockable::mock_deserialize(address)
.expect("unable to deserialize ExecutionError address"),
retries: Mockable::mock_deserialize(retries)
.expect("unable to deserialize ExecutionError retries"),
})
}
}
Expand Down
24 changes: 24 additions & 0 deletions packages/rs-dapi-client/src/request_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

use std::time::Duration;

use crate::transport::TransportRequest;

/// Default low-level client timeout
const DEFAULT_CONNECT_TIMEOUT: Option<Duration> = None;
const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10);
Expand Down Expand Up @@ -77,3 +79,25 @@ pub struct AppliedRequestSettings {
/// Ban DAPI address if node not responded or responded with error.
pub ban_failed_address: bool,
}

impl AppliedRequestSettings {
/// Create [AppliedRequestSettings] from [RequestSettings] with default values.
///
/// Combine provided [RequestSettings] together with [request-level overrides](TransportRequest::SETTINGS_OVERRIDES).
///
///
/// # Arguments
///
/// * `global_settings` - global settings for all requests.
/// * `request_settings` - settings for a specific request.
pub fn from_settings<R: TransportRequest>(
Copy link
Member

Choose a reason for hiding this comment

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

From<(&RequestSettings, &RequestSettings)> ? ))

Copy link
Contributor Author

@lklimek lklimek Oct 30, 2024

Choose a reason for hiding this comment

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

Function is not used, removed.

global_settings: &RequestSettings,
request_settings: &RequestSettings,
) -> Self {
RequestSettings::default()
.override_by(*global_settings)
.override_by(R::SETTINGS_OVERRIDES)
.override_by(*request_settings)
.finalize()
}
}
4 changes: 2 additions & 2 deletions packages/rs-drive-proof-verifier/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ default = ["mocks"]
mocks = [
"dep:serde",
"dep:serde_json",
"dep:bincode",
"bincode/serde",
"dep:platform-serialization-derive",
"dep:platform-serialization",
"dpp/document-serde-conversion",
Expand All @@ -31,7 +31,7 @@ dpp = { path = "../rs-dpp", features = [
"extended-document",
"core-types-serialization",
], default-features = false }
bincode = { version = "2.0.0-rc.3", features = ["serde"], optional = true }
bincode = { version = "2.0.0-rc.3", optional = false }
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review bincode dependency configuration.

Setting optional = false for bincode while it's under the mocks feature seems inconsistent. This could cause bincode to be always included in the binary even when the mocks feature is disabled.

Consider this fix to maintain consistency:

-bincode = { version = "2.0.0-rc.3", optional = false }
+bincode = { version = "2.0.0-rc.3", optional = true }

Committable suggestion was skipped due to low confidence.

Copy link
Member

Choose a reason for hiding this comment

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

optional = false sounds a bit strange

Copy link
Contributor

Choose a reason for hiding this comment

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

@shumkov, you're correct. Setting optional = false here can cause bincode to be included in the binary even when the mocks feature is disabled. Changing it to optional = true ensures that bincode is only included when the mocks feature is enabled, maintaining consistency.

(^‿^)/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed optional, as it's in use even without mocks feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

platform-serialization-derive = { path = "../rs-platform-serialization-derive", optional = true }
platform-serialization = { path = "../rs-platform-serialization", optional = true }
tenderdash-abci = { git = "https://github.com/dashpay/rs-tenderdash-abci", version = "1.2.1", tag = "v1.2.1+1.3.0", features = [
Expand Down
2 changes: 2 additions & 0 deletions packages/rs-drive-proof-verifier/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use dpp::ProtocolError;

/// Errors
#[derive(Debug, thiserror::Error)]
#[cfg_attr(feature = "mocks", derive(bincode::Encode, bincode::Decode))]
Copy link
Member

Choose a reason for hiding this comment

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

We use serde in dapi client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's all under mocks feature atm, we should introduce separate serde feature. TBD in next PR.

#[allow(missing_docs)]
pub enum Error {
/// Not initialized
Expand Down Expand Up @@ -86,6 +87,7 @@ pub enum Error {

/// Errors returned by the context provider
#[derive(Debug, thiserror::Error)]
#[cfg_attr(feature = "mocks", derive(bincode::Encode, bincode::Decode))]
pub enum ContextProviderError {
/// Generic Context provider error
#[error("Context provider error: {0}")]
Expand Down
4 changes: 2 additions & 2 deletions packages/rs-drive-proof-verifier/src/provider.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use crate::error::ContextProviderError;
use dpp::prelude::{CoreBlockHeight, DataContract, Identifier};
use drive::{error::proof::ProofError, query::ContractLookupFn};
use std::{ops::Deref, sync::Arc};
#[cfg(feature = "mocks")]
use hex::ToHex;
use std::{io::ErrorKind, ops::Deref, sync::Arc};
use {hex::ToHex, std::io::ErrorKind};

/// Interface between the Sdk and state of the application.
///
Expand Down
Loading
Loading