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

Aviv/build serislize trait #46

Closed
wants to merge 1 commit into from

Conversation

AvivYossef-starkware
Copy link
Collaborator

@AvivYossef-starkware AvivYossef-starkware commented Apr 17, 2024

This pull request introduces a new trait serde_trait along with error enums StorageError and SerdeError, and their respective implementations. These additions aim to provide a framework for serialization and deserialization operations within the storage module.


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 29.50%. Comparing base (327416b) to head (20edd40).

Files Patch % Lines
crates/committer/src/storage/errors.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
- Coverage   29.75%   29.50%   -0.25%     
==========================================
  Files           8        9       +1     
  Lines         121      122       +1     
  Branches      121      122       +1     
==========================================
  Hits           36       36              
- Misses         84       85       +1     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AvivYossef-starkware AvivYossef-starkware force-pushed the Aviv/build_serislize_trait branch 3 times, most recently from a592d43 to a8c4ba0 Compare April 17, 2024 07:59
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @amosStarkware, @AvivYossef-starkware, and @nimrod-starkware)


Cargo.toml line 21 at r1 (raw file):

rstest = "0.17.0"
starknet-types-core = { version = "0.0.11", features = ["hash"] }
thiserror = "1.0"

already added here, please sync

Code quote:

thiserror = "1.0"

crates/committer/src/storage/errors.rs line 8 at r1 (raw file):

#[derive(thiserror::Error, Debug, Display)]
#[allow(dead_code)]
pub(crate) enum SerdeError {

this can be confusing - there is a serde crate, which defines a serde_json::Error object already. Maybe just rename ?

Suggestion:

SerializationError

crates/committer/src/storage/serde_trait.rs line 4 at r1 (raw file):

use crate::storage::storage_trait::{StorageError, StorageKey, StorageValue};

pub(crate) trait serde_trait {
  1. don't use snake case for traits - capital camelcase (pascal case?)
  2. I would use "Serializable", not serde, because serde is an external crate

Suggestion:

pub(crate) trait Serializable {

crates/committer/src/storage/serde_trait.rs line 4 at r1 (raw file):

use crate::storage::storage_trait::{StorageError, StorageKey, StorageValue};

pub(crate) trait serde_trait {

add a db_key(&self) -> StorageKey method


crates/committer/src/storage/serde_trait.rs line 8 at r1 (raw file):

    fn serialize(&self) -> Result<StorageValue, SerdeError::Serialize>;
    /// Deserializes the given value.
    fn deserialize(vale: StorageValue) -> Result<Self, SerdeError::Deserialize>;

typo

Suggestion:

value

@AvivYossef-starkware AvivYossef-starkware deleted the Aviv/build_serislize_trait branch April 17, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants