Skip to content

Conversation

@smrz2001
Copy link
Contributor

No description provided.

@smrz2001 smrz2001 requested a review from dav1do August 14, 2024 21:47
@smrz2001 smrz2001 requested review from a team and nathanielc as code owners August 14, 2024 21:47
@smrz2001 smrz2001 requested review from stbrody and removed request for a team August 14, 2024 21:47
@smrz2001 smrz2001 temporarily deployed to github-tests-2024 August 14, 2024 21:57 — with GitHub Actions Inactive
@smrz2001 smrz2001 force-pushed the feat/serde-ipld-macro branch from c660f7a to 5866b60 Compare August 15, 2024 00:07
@smrz2001 smrz2001 temporarily deployed to github-tests-2024 August 15, 2024 00:18 — with GitHub Actions Inactive
Copy link
Collaborator

@dav1do dav1do left a comment

Choose a reason for hiding this comment

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

IMO using a proc macro for this is overkill (plus it won't work for lifetimes/generics)... a simple derive macro would work and makes it simpler to understand... e.g. this with the error type as a param if desired

#[derive(serde::Serialize, serde::Deserialize)]
struct Dog {
    s: String,
}

macro_rules! derive_serde_ipld {
    ($name: ident, $err: ty) => {
        impl $name {
            /// serde serialize to dag-json
            pub fn to_json(&self) -> Result<String, Box<$err> {
                 let json_bytes = serde_ipld_dagjson::to_vec(self)?;
                 String::from_utf8(json_bytes).map_err(Into::into)
            }

            /// serde serialize to dag-cbor
            pub fn to_cbor(&self) -> Result<Vec<u8>, $err> {
                serde_ipld_dagcbor::to_vec(self).map_err(Into::into)
            }

            /// CID of serde serialize to dag-cbor
            pub fn to_cid(&self) -> Result<Cid, $err> {
                let cbor_bytes = self.to_cbor()?;
                use multihash_codetable::MultihashDigest;
                let hash = multihash_codetable::Code::Sha2_256.digest(&cbor_bytes);
                Ok(Cid::new_v1(0x71, hash))
            }
        }
    };
}
derive_serde_ipld!(Dog, Box<dyn std::error::Error>);

Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

This feels like too much complexity to abstract away calling serde_ipld_* crate methods.

I won't push back super hard but this feels like a lot of code to maintain for a little ergonomic benefit.

Maybe just a crate with helper methods that take an impl of serde::Serialize and serde::Deserialize is all we need?

@smrz2001 smrz2001 force-pushed the feat/serde-ipld-macro branch 2 times, most recently from 59d2b2b to bbe3482 Compare August 22, 2024 00:03
@smrz2001 smrz2001 changed the title feat: serde ipld macro feat: trait for generating cbor bytes and cids Aug 22, 2024
@smrz2001 smrz2001 temporarily deployed to github-tests-2024 August 22, 2024 00:14 — with GitHub Actions Inactive
@AaronGoldman AaronGoldman force-pushed the feat/serde-ipld-macro branch from bbe3482 to 1e57390 Compare August 23, 2024 16:37
Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

Overall really like how this has simplified the code. Really good use of a trait. With some consensus on the name this should be good to go

@smrz2001 smrz2001 temporarily deployed to github-tests-2024 August 23, 2024 21:37 — with GitHub Actions Inactive
@smrz2001 smrz2001 changed the title feat: trait for generating cbor bytes and cids feat: trait for generating cbor bytes and cids (part 1) Aug 23, 2024
}

impl<T: Serialize> SerializeExt for T {
type Error = anyhow::Error;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using anyhow here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the ipld crates use two different error types and don't just use a serde error, so we need to provide something that matches both. Should we use an error that passes through the variants or is using anyhow okay?

// ipld_dag_json
#[derive(Debug)]
pub enum EncodeError {
    Message(String),
}

// ipld_dag_cbor
#[derive(Debug)]
pub enum EncodeError<E> {
    /// Custom error message.
    Msg(String),
    /// IO Error.
    Write(E),
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use thiserror and create a variant for each type of error that can be produced. Thiserror make this very easy. With this approach we expose the underlying errors which is what we want in the context of a library trait.

/// SerializeExt is a trait for serialization, deserialization, CID calculation assuming dag-cbor.
pub trait SerializeExt: Serialize {
/// Error type for SerializeExt
// type Error;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, remove these comments.

@AaronGoldman AaronGoldman added this pull request to the merge queue Aug 29, 2024
Merged via the queue into main with commit cde8736 Aug 29, 2024
@AaronGoldman AaronGoldman deleted the feat/serde-ipld-macro branch August 29, 2024 17:07
@smrz2001 smrz2001 mentioned this pull request Sep 9, 2024
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.

4 participants