Skip to content

Commit

Permalink
refactor(proto)!: remove proto bytes for std bytes (#5835)
Browse files Browse the repository at this point in the history
Description
---
This PR will remove the google proto bytes wrapper in favour of using a
standard vec.

Motivation and Context
---
Use standard rust types, instead of library specific types.

Related to #5833

How Has This Been Tested?
---
CI

What process can a PR reviewer use to test or verify this change?
---
Previously used test values included `None`, `Some(vec![])` and
`Some(FixedHash::zero())`. This PR removes the option wrappers and as a
result the previous `Some(vec![])` values will be `vec![]` and
considered the equivalent to `None`.

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [ ] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [x] Other - Please specify

Messages between new and old nodes are likely to break.
  • Loading branch information
brianp authored Oct 12, 2023
1 parent 1f87226 commit 491ed83
Show file tree
Hide file tree
Showing 12 changed files with 102 additions and 94 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,10 @@ mod test {
let diff: u128 = 1;
proto::ChainMetadata {
height_of_longest_chain: Some(1),
best_block: Some(vec![
best_block: vec![
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27,
28, 29, 30, 31,
]),
],
pruned_height: 0,
accumulated_difficulty: diff.to_be_bytes().to_vec(),
timestamp: Some(0),
Expand Down
2 changes: 1 addition & 1 deletion base_layer/core/src/base_node/proto/chain_metadata.proto
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ message ChainMetadata {
// The current chain height, or the block number of the longest valid chain, or `None` if there is no chain
google.protobuf.UInt64Value height_of_longest_chain = 1;
// The block hash of the current tip of the longest valid chain, or `None` for an empty chain
google.protobuf.BytesValue best_block = 2;
bytes best_block = 2;
// The current geometric mean of the pow of the chain tip, or `None` if there is no chain
bytes accumulated_difficulty = 5;
// The effective height of the pruning horizon. This indicates from what height
Expand Down
7 changes: 5 additions & 2 deletions base_layer/core/src/base_node/proto/chain_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,12 @@ impl TryFrom<proto::ChainMetadata> for ChainMetadata {
} else {
height_of_longest_chain.saturating_sub(metadata.pruned_height)
};

if metadata.best_block.is_empty() {
return Err("Best block is missing".to_string());
}
let hash: FixedHash = metadata
.best_block
.ok_or_else(|| "Best block is missing".to_string())?
.try_into()
.map_err(|e| format!("Malformed best block: {}", e))?;
Ok(ChainMetadata::new(
Expand All @@ -74,7 +77,7 @@ impl From<ChainMetadata> for proto::ChainMetadata {
let accumulated_difficulty = metadata.accumulated_difficulty().to_be_bytes().to_vec();
Self {
height_of_longest_chain: Some(metadata.height_of_longest_chain()),
best_block: Some(metadata.best_block().to_vec()),
best_block: metadata.best_block().to_vec(),
pruned_height: metadata.pruned_height(),
accumulated_difficulty,
timestamp: Some(metadata.timestamp()),
Expand Down
8 changes: 4 additions & 4 deletions base_layer/core/src/base_node/proto/wallet_rpc.proto
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ enum TxLocation {

message TxQueryResponse {
TxLocation location = 1;
google.protobuf.BytesValue block_hash = 2;
bytes block_hash = 2;
uint64 confirmations = 3;
bool is_synced = 4;
uint64 height_of_longest_chain = 5;
Expand All @@ -45,7 +45,7 @@ message TxQueryResponse {
message TxQueryBatchResponse {
tari.types.Signature signature = 1;
TxLocation location = 2;
google.protobuf.BytesValue block_hash = 3;
bytes block_hash = 3;
uint64 confirmations = 4;
uint64 block_height = 5;
google.protobuf.UInt64Value mined_timestamp = 6;
Expand All @@ -54,7 +54,7 @@ message TxQueryBatchResponse {
message TxQueryBatchResponses {
repeated TxQueryBatchResponse responses = 1;
bool is_synced = 2;
google.protobuf.BytesValue tip_hash = 3;
bytes tip_hash = 3;
uint64 height_of_longest_chain = 4;
google.protobuf.UInt64Value tip_mined_timestamp = 5;
}
Expand All @@ -70,7 +70,7 @@ message FetchUtxosResponse {

message QueryDeletedRequest {
repeated uint64 mmr_positions = 1;
google.protobuf.BytesValue chain_must_include_header = 2;
bytes chain_must_include_header = 2;
bool include_deleted_block_data = 3;
}

Expand Down
35 changes: 21 additions & 14 deletions base_layer/core/src/base_node/proto/wallet_rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@
// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

use std::{
convert::{TryFrom, TryInto},
convert::TryFrom,
fmt::{Display, Error, Formatter},
};

use serde::{Deserialize, Serialize};
use tari_common_types::types::{BlockHash, Signature};
use tari_utilities::ByteArray;

use crate::proto::{base_node as proto, types};

Expand Down Expand Up @@ -191,12 +192,15 @@ impl TryFrom<proto::TxQueryResponse> for TxQueryResponse {
type Error = String;

fn try_from(proto_response: proto::TxQueryResponse) -> Result<Self, Self::Error> {
let hash = match proto_response.block_hash {
Some(v) => match v.try_into() {
Ok(v) => Some(v),
Err(e) => return Err(format!("Malformed block hash: {}", e)),
},
None => None,
let hash = if proto_response.block_hash.is_empty() {
None
} else {
Some(match BlockHash::try_from(proto_response.block_hash.clone()) {
Ok(h) => h,
Err(e) => {
return Err(format!("Malformed block hash: {}", e));
},
})
};
Ok(Self {
location: TxLocation::try_from(
Expand All @@ -216,7 +220,7 @@ impl From<TxQueryResponse> for proto::TxQueryResponse {
fn from(response: TxQueryResponse) -> Self {
Self {
location: proto::TxLocation::from(response.location) as i32,
block_hash: response.block_hash.map(|v| v.to_vec()),
block_hash: response.block_hash.map(|v| v.to_vec()).unwrap_or(vec![]),
confirmations: response.confirmations,
is_synced: response.is_synced,
height_of_longest_chain: response.height_of_longest_chain,
Expand All @@ -229,12 +233,15 @@ impl TryFrom<proto::TxQueryBatchResponse> for TxQueryBatchResponse {
type Error = String;

fn try_from(proto_response: proto::TxQueryBatchResponse) -> Result<Self, Self::Error> {
let hash = match proto_response.block_hash {
Some(v) => match v.try_into() {
Ok(v) => Some(v),
Err(e) => return Err(format!("Malformed block hash: {}", e)),
},
None => None,
let hash = if proto_response.block_hash.is_empty() {
None
} else {
Some(match BlockHash::try_from(proto_response.block_hash.clone()) {
Ok(h) => h,
Err(e) => {
return Err(format!("Malformed block hash: {}", e));
},
})
};
Ok(Self {
signature: Signature::try_from(
Expand Down
13 changes: 7 additions & 6 deletions base_layer/core/src/base_node/rpc/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl<B: BlockchainBackend + 'static> BaseNodeWalletRpcService<B> {
let confirmations = chain_metadata.height_of_longest_chain().saturating_sub(header.height);
let response = TxQueryResponse {
location: TxLocation::Mined as i32,
block_hash: Some(block_hash.to_vec()),
block_hash: block_hash.to_vec(),
confirmations,
is_synced,
height_of_longest_chain: chain_metadata.height_of_longest_chain(),
Expand All @@ -146,7 +146,7 @@ impl<B: BlockchainBackend + 'static> BaseNodeWalletRpcService<B> {
{
TxStorageResponse::UnconfirmedPool => TxQueryResponse {
location: TxLocation::InMempool as i32,
block_hash: None,
block_hash: vec![],
confirmations: 0,
is_synced,
height_of_longest_chain: chain_metadata.height_of_longest_chain(),
Expand All @@ -161,7 +161,7 @@ impl<B: BlockchainBackend + 'static> BaseNodeWalletRpcService<B> {
TxStorageResponse::NotStoredFeeTooLow |
TxStorageResponse::NotStoredAlreadyMined => TxQueryResponse {
location: TxLocation::NotStored as i32,
block_hash: None,
block_hash: vec![],
confirmations: 0,
is_synced,
height_of_longest_chain: chain_metadata.height_of_longest_chain(),
Expand Down Expand Up @@ -318,7 +318,7 @@ impl<B: BlockchainBackend + 'static> BaseNodeWalletService for BaseNodeWalletRpc
Ok(Response::new(TxQueryBatchResponses {
responses,
is_synced,
tip_hash: Some(metadata.best_block().to_vec()),
tip_hash: metadata.best_block().to_vec(),
height_of_longest_chain: metadata.height_of_longest_chain(),
tip_mined_timestamp: Some(metadata.timestamp()),
}))
Expand Down Expand Up @@ -458,8 +458,9 @@ impl<B: BlockchainBackend + 'static> BaseNodeWalletService for BaseNodeWalletRpc
) -> Result<Response<QueryDeletedResponse>, RpcStatus> {
let message = request.into_message();

if let Some(chain_must_include_header) = message.chain_must_include_header {
let hash = chain_must_include_header
if !message.chain_must_include_header.is_empty() {
let hash = message
.chain_must_include_header
.try_into()
.map_err(|_| RpcStatus::bad_request(&"Malformed block hash received".to_string()))?;
if self
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ where
// This assumes that the base node has not reorged since the last time we asked.
let deleted_bitmap_response = wallet_client
.query_deleted(QueryDeletedRequest {
chain_must_include_header: last_mined_header_hash.map(|v| v.to_vec()),
chain_must_include_header: last_mined_header_hash.map(|v| v.to_vec()).unwrap_or_default(),
mmr_positions: batch.iter().filter_map(|ub| ub.mined_mmr_position).collect(),
include_deleted_block_data: true,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -341,10 +341,9 @@ where
}
}
}
let tip = batch_response
.tip_hash
.ok_or_else(|| TransactionServiceError::ProtobufConversionError("Missing `tip_hash` field".to_string()))?
.try_into()?;

let tip = batch_response.tip_hash.try_into()?;

Ok((
mined,
unmined,
Expand Down
10 changes: 5 additions & 5 deletions base_layer/wallet/tests/support/comms_rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use std::{
time::{Duration, Instant},
};

use tari_common_types::types::{HashOutput, Signature};
use tari_common_types::types::{FixedHash, HashOutput, Signature};
use tari_comms::{
protocol::rpc::{NamedProtocolService, Request, Response, RpcClient, RpcStatus, Streaming},
PeerConnection,
Expand Down Expand Up @@ -141,15 +141,15 @@ impl BaseNodeWalletRpcMockState {
})),
transaction_query_batch_response: Arc::new(Mutex::new(TxQueryBatchResponsesProto {
responses: vec![],
tip_hash: Some(vec![]),
tip_hash: FixedHash::zero().to_vec(),
is_synced: true,
height_of_longest_chain: 0,
tip_mined_timestamp: Some(0),
})),
tip_info_response: Arc::new(Mutex::new(TipInfoResponse {
metadata: Some(ChainMetadataProto {
height_of_longest_chain: Some(std::i64::MAX as u64),
best_block: Some(Vec::new()),
best_block: FixedHash::zero().to_vec(),
accumulated_difficulty: Vec::new(),
pruned_height: 0,
timestamp: Some(0),
Expand Down Expand Up @@ -932,8 +932,8 @@ mod test {

let chain_metadata = ChainMetadata {
height_of_longest_chain: Some(444),
best_block: Some(Vec::new()),
accumulated_difficulty: Vec::new(),
best_block: vec![],
accumulated_difficulty: vec![],
pruned_height: 0,
timestamp: Some(0),
};
Expand Down
Loading

0 comments on commit 491ed83

Please sign in to comment.