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.

Sign up for GitHub

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

Change anyhow for thiserror #1

Merged
merged 6 commits into from
Jan 3, 2025
Merged

Change anyhow for thiserror #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
25ada9c
Add thiserror
gianbelinche Nov 29, 2024
a44decf
Fix warnings
gianbelinche Nov 29, 2024
6a439ae
Fix remaining warnings
gianbelinche Nov 29, 2024
c925859
Fix PR comments
gianbelinche Dec 2, 2024
402ec48
Fix comments
gianbelinche Dec 23, 2024
4974c95
Merge branch 'client-implementation' into thiserror
gianbelinche Dec 23, 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
4 changes: 0 additions & 4 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,3 @@ async-trait = "0.1"
hex = "0.4"
secrecy = "0.8.0"
byteorder = "1.5.0"



anyhow = "1" #TODO: Remove
119 changes: 8 additions & 111 deletions src/blob_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::fmt;

use rlp::{Decodable, DecoderError, Encodable, Rlp, RlpStream};

use crate::errors::ConversionError;

use super::{
common::G1Commitment as DisperserG1Commitment,
disperser::{
Expand All @@ -12,37 +12,12 @@ use super::{
},
};

#[derive(Debug)]
pub enum ConversionError {
NotPresentError,
}

impl fmt::Display for ConversionError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
ConversionError::NotPresentError => write!(f, "Failed to convert BlobInfo"),
}
}
}

#[derive(Debug, PartialEq, Clone)]
pub struct G1Commitment {
pub x: Vec<u8>,
pub y: Vec<u8>,
}

impl G1Commitment {
pub fn to_bytes(&self) -> Vec<u8> {
let mut bytes = vec![];
bytes.extend(&self.x.len().to_be_bytes());
bytes.extend(&self.x);
bytes.extend(&self.y.len().to_be_bytes());
bytes.extend(&self.y);

bytes
}
Comment on lines -35 to -43
Copy link

Choose a reason for hiding this comment

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

why are you deleting all these to_bytes? Were they used for something before?

Copy link

Choose a reason for hiding this comment

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

they were not needed.

}

impl Decodable for G1Commitment {
fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
let x: Vec<u8> = rlp.val_at(0)?; // Decode first element as Vec<u8>
Expand Down Expand Up @@ -77,18 +52,6 @@ pub struct BlobQuorumParam {
pub chunk_length: u32,
}

impl BlobQuorumParam {
pub fn to_bytes(&self) -> Vec<u8> {
let mut bytes = vec![];
bytes.extend(&self.quorum_number.to_be_bytes());
bytes.extend(&self.adversary_threshold_percentage.to_be_bytes());
bytes.extend(&self.confirmation_threshold_percentage.to_be_bytes());
bytes.extend(&self.chunk_length.to_be_bytes());

bytes
}
}

impl Decodable for BlobQuorumParam {
fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
Ok(BlobQuorumParam {
Expand Down Expand Up @@ -128,21 +91,6 @@ pub struct BlobHeader {
pub blob_quorum_params: Vec<BlobQuorumParam>,
}

impl BlobHeader {
pub fn to_bytes(&self) -> Vec<u8> {
let mut bytes = vec![];
bytes.extend(self.commitment.to_bytes());
bytes.extend(&self.data_length.to_be_bytes());
bytes.extend(&self.blob_quorum_params.len().to_be_bytes());

for quorum in &self.blob_quorum_params {
bytes.extend(quorum.to_bytes());
}

bytes
}
}

impl Decodable for BlobHeader {
fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
let commitment: G1Commitment = rlp.val_at(0)?;
Expand Down Expand Up @@ -170,7 +118,7 @@ impl TryFrom<DisperserBlobHeader> for BlobHeader {
type Error = ConversionError;
fn try_from(value: DisperserBlobHeader) -> Result<Self, Self::Error> {
if value.commitment.is_none() {
return Err(ConversionError::NotPresentError);
return Err(ConversionError::NotPresent("BlobHeader".to_string()));
}
let blob_quorum_params: Vec<BlobQuorumParam> = value
.blob_quorum_params
Expand All @@ -193,21 +141,6 @@ pub struct BatchHeader {
pub reference_block_number: u32,
}

impl BatchHeader {
pub fn to_bytes(&self) -> Vec<u8> {
let mut bytes = vec![];
bytes.extend(&self.batch_root.len().to_be_bytes());
bytes.extend(&self.batch_root);
bytes.extend(&self.quorum_numbers.len().to_be_bytes());
bytes.extend(&self.quorum_numbers);
bytes.extend(&self.quorum_signed_percentages.len().to_be_bytes());
bytes.extend(&self.quorum_signed_percentages);
bytes.extend(&self.reference_block_number.to_be_bytes());

bytes
}
}

impl Decodable for BatchHeader {
fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
Ok(BatchHeader {
Expand Down Expand Up @@ -249,17 +182,6 @@ pub struct BatchMetadata {
pub batch_header_hash: Vec<u8>,
}

impl BatchMetadata {
pub fn to_bytes(&self) -> Vec<u8> {
let mut bytes = vec![];
bytes.extend(self.batch_header.to_bytes());
bytes.extend(&self.signatory_record_hash);
bytes.extend(&self.confirmation_block_number.to_be_bytes());

bytes
}
}

impl Decodable for BatchMetadata {
fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
let batch_header: BatchHeader = rlp.val_at(0)?;
Expand Down Expand Up @@ -289,7 +211,7 @@ impl TryFrom<DisperserBatchMetadata> for BatchMetadata {
type Error = ConversionError;
fn try_from(value: DisperserBatchMetadata) -> Result<Self, Self::Error> {
if value.batch_header.is_none() {
return Err(ConversionError::NotPresentError);
return Err(ConversionError::NotPresent("BatchMetadata".to_string()));
}
Ok(Self {
batch_header: BatchHeader::from(value.batch_header.unwrap()),
Expand All @@ -310,21 +232,6 @@ pub struct BlobVerificationProof {
pub quorum_indexes: Vec<u8>,
}

impl BlobVerificationProof {
pub fn to_bytes(&self) -> Vec<u8> {
let mut bytes = vec![];
bytes.extend(&self.batch_id.to_be_bytes());
bytes.extend(&self.blob_index.to_be_bytes());
bytes.extend(self.batch_medatada.to_bytes());
bytes.extend(&self.inclusion_proof.len().to_be_bytes());
bytes.extend(&self.inclusion_proof);
bytes.extend(&self.quorum_indexes.len().to_be_bytes());
bytes.extend(&self.quorum_indexes);

bytes
}
}

impl Decodable for BlobVerificationProof {
fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
Ok(BlobVerificationProof {
Expand Down Expand Up @@ -352,7 +259,9 @@ impl TryFrom<DisperserBlobVerificationProof> for BlobVerificationProof {
type Error = ConversionError;
fn try_from(value: DisperserBlobVerificationProof) -> Result<Self, Self::Error> {
if value.batch_metadata.is_none() {
return Err(ConversionError::NotPresentError);
return Err(ConversionError::NotPresent(
"BlobVerificationProof".to_string(),
));
}
Ok(Self {
batch_id: value.batch_id,
Expand All @@ -370,18 +279,6 @@ pub struct BlobInfo {
pub blob_verification_proof: BlobVerificationProof,
}

impl BlobInfo {
pub fn to_bytes(&self) -> Vec<u8> {
let mut bytes = vec![];
let blob_header_bytes = self.blob_header.to_bytes();
bytes.extend(blob_header_bytes.len().to_be_bytes());
bytes.extend(blob_header_bytes);
let blob_verification_proof_bytes = self.blob_verification_proof.to_bytes();
bytes.extend(blob_verification_proof_bytes);
bytes
}
}

impl Decodable for BlobInfo {
fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
let blob_header: BlobHeader = rlp.val_at(0)?;
Expand All @@ -406,7 +303,7 @@ impl TryFrom<DisperserBlobInfo> for BlobInfo {
type Error = ConversionError;
fn try_from(value: DisperserBlobInfo) -> Result<Self, Self::Error> {
if value.blob_header.is_none() || value.blob_verification_proof.is_none() {
return Err(ConversionError::NotPresentError);
return Err(ConversionError::NotPresent("BlobInfo".to_string()));
}
Ok(Self {
blob_header: BlobHeader::try_from(value.blob_header.unwrap())?,
Expand Down
24 changes: 15 additions & 9 deletions src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::errors::{CommunicationError, ConfigError, EigenClientError};

use super::{
blob_info::BlobInfo,
config::{EigenConfig, EigenSecrets},
Expand All @@ -14,36 +16,37 @@ pub struct EigenClient {
}

impl EigenClient {
pub async fn new(config: EigenConfig, secrets: EigenSecrets) -> anyhow::Result<Self> {
pub async fn new(config: EigenConfig, secrets: EigenSecrets) -> Result<Self, EigenClientError> {
Copy link

Choose a reason for hiding this comment

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

Is using a single huge crate-level error enum like EigenClientError the typical way to go in rust? Reading https://www.reddit.com/r/rust/comments/1bb7dco/error_handling_goodbest_practices/ it seems like positions are mitigated on this issue.

It feels a bit weird to me. Wouldn't it be better to have a specific error enum per method? For eg, one guy says:

As a rule of thumb, yes, errors should be specific to the operation. This ensures that client code doesn't have to be littered with unreachable!() statements for failure cases that are known to be impossible for that operation.

Copy link

Choose a reason for hiding this comment

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

Having a single error type that encapsulates the possible errors from a library makes it easier to use (specially if you use several modules from the same library together).

We did a small refactor of the error types since your last review. So feel free to take a look again.

let private_key = SecretKey::from_str(secrets.private_key.0.expose_secret().as_str())
.map_err(|e| anyhow::anyhow!("Failed to parse private key: {}", e))?;
.map_err(ConfigError::Secp)?;

let client = RawEigenClient::new(private_key, config).await?;
Ok(Self {
client: Arc::new(client),
})
}

pub async fn get_commitment(&self, blob_id: &str) -> anyhow::Result<String> {
pub async fn get_commitment(&self, blob_id: &str) -> Result<String, EigenClientError> {
let blob_info = self.client.get_inclusion_data(blob_id).await?;
Ok(blob_info)
}

async fn dispatch_blob(&self, data: Vec<u8>) -> anyhow::Result<String> {
pub async fn dispatch_blob(&self, data: Vec<u8>) -> Result<String, EigenClientError> {
let blob_id = self.client.dispatch_blob(data).await?;

Ok(blob_id)
}

async fn get_inclusion_data(&self, blob_id: &str) -> anyhow::Result<Vec<u8>> {
pub async fn get_inclusion_data(&self, blob_id: &str) -> Result<Vec<u8>, EigenClientError> {
let blob_info = self.get_commitment(blob_id).await?;
let rlp_encoded_bytes = hex::decode(blob_info)?;
let blob_info: BlobInfo = rlp::decode(&rlp_encoded_bytes)?;
let rlp_encoded_bytes = hex::decode(blob_info).map_err(CommunicationError::Hex)?;
let blob_info: BlobInfo =
rlp::decode(&rlp_encoded_bytes).map_err(CommunicationError::Rlp)?;
let inclusion_data = blob_info.blob_verification_proof.inclusion_proof;
Ok(inclusion_data)
}

fn blob_size_limit(&self) -> Option<usize> {
pub fn blob_size_limit(&self) -> Option<usize> {
Copy link

Choose a reason for hiding this comment

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

Is it necessary to expose this? BLOCK_SIZE_LIMIT is a const right now

Some(RawEigenClient::blob_size_limit())
}
}
Expand All @@ -61,7 +64,10 @@ mod tests {
use crate::blob_info::BlobInfo;

impl EigenClient {
pub async fn get_blob_data(&self, blob_id: &str) -> anyhow::Result<Option<Vec<u8>>> {
pub async fn get_blob_data(
&self,
blob_id: &str,
) -> Result<Option<Vec<u8>>, EigenClientError> {
self.client.get_blob_data(blob_id).await
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use secrecy::{ExposeSecret, Secret};
use std::str::FromStr;

use crate::errors::{ConfigError, EigenClientError};

#[derive(Clone, Debug, PartialEq)]
pub enum SRSPointsSource {
Path(String),
Expand Down Expand Up @@ -56,9 +58,9 @@ impl PartialEq for PrivateKey {
}

impl FromStr for PrivateKey {
type Err = anyhow::Error;
type Err = EigenClientError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
Ok(PrivateKey(s.parse()?))
Ok(PrivateKey(s.parse().map_err(|_| ConfigError::PrivateKey)?))
}
}
Loading