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

core: simplify serde implementations #3485

Closed
Neotamandua opened this issue Feb 11, 2025 · 1 comment
Closed

core: simplify serde implementations #3485

Neotamandua opened this issue Feb 11, 2025 · 1 comment
Labels
priority:minor Low priority issues

Comments

@Neotamandua
Copy link
Member

Summary

Some of the current Serde implementations are very verbose and can be significantly reduced in code size by using a different approach.

Example

Current approach (108 LoC):

impl<'de> Deserialize<'de> for PhoenixTransactionEvent {
    fn deserialize<D: Deserializer<'de>>(
        deserializer: D,
    ) -> Result<Self, D::Error> {
        struct PhoenixTransactionEventVisitor;

        const FIELDS: [&str; 5] =
            ["nullifiers", "notes", "memo", "gas_spent", "refund_note"];

        impl<'de> Visitor<'de> for PhoenixTransactionEventVisitor {
            type Value = PhoenixTransactionEvent;

            fn expecting(
                &self,
                formatter: &mut alloc::fmt::Formatter,
            ) -> alloc::fmt::Result {
                formatter.write_str("expecting a struct with fields nullifiers, notes, memo, gas_spent and refund_note")
            }

            fn visit_map<A: MapAccess<'de>>(
                self,
                mut map: A,
            ) -> Result<Self::Value, A::Error> {
                let mut nullifiers = None;
                let mut notes = None;
                let mut memo: Option<String> = None;
                let mut gas_spent: Option<Bigint> = None;
                let mut refund_note = None;

                while let Some(key) = map.next_key()? {
                    match key {
                        "nullifiers" => {
                            if nullifiers.is_some() {
                                return Err(SerdeError::duplicate_field(
                                    "nullifiers",
                                ));
                            }
                            nullifiers = Some(map.next_value()?);
                        }
                        "notes" => {
                            if notes.is_some() {
                                return Err(SerdeError::duplicate_field(
                                    "notes",
                                ));
                            }
                            notes = Some(map.next_value()?);
                        }
                        "memo" => {
                            if memo.is_some() {
                                return Err(SerdeError::duplicate_field(
                                    "memo",
                                ));
                            }
                            memo = Some(map.next_value()?);
                        }
                        "gas_spent" => {
                            if gas_spent.is_some() {
                                return Err(SerdeError::duplicate_field(
                                    "gas_spent",
                                ));
                            }
                            gas_spent = Some(map.next_value()?);
                        }
                        "refund_note" => {
                            if refund_note.is_some() {
                                return Err(SerdeError::duplicate_field(
                                    "refund_note",
                                ));
                            }
                            refund_note = Some(map.next_value()?);
                        }
                        field => {
                            return Err(SerdeError::unknown_field(
                                field, &FIELDS,
                            ))
                        }
                    }
                }

                let nullifiers = nullifiers
                    .ok_or_else(|| SerdeError::missing_field("nullifiers"))?;
                let memo =
                    memo.ok_or_else(|| SerdeError::missing_field("memo"))?;
                let memo =
                    BASE64_STANDARD.decode(memo).map_err(SerdeError::custom)?;

                Ok(PhoenixTransactionEvent {
                    nullifiers,
                    notes: notes
                        .ok_or_else(|| SerdeError::missing_field("memo"))?,
                    memo,
                    gas_spent: gas_spent
                        .ok_or_else(|| SerdeError::missing_field("gas_spent"))?
                        .0,
                    refund_note: refund_note.ok_or_else(|| {
                        SerdeError::missing_field("refund_note")
                    })?,
                })
            }
        }

        deserializer.deserialize_struct(
            "PhoenixTransactionEvent",
            &FIELDS,
            PhoenixTransactionEventVisitor,
        )
    }
}

Possible other approach (23 LoC)

impl<'de> Deserialize<'de> for PhoenixTransactionEvent {
    fn deserialize<D: Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
        #[derive(Deserialize)]
        struct IntermediateEvent {
            nullifiers: Vec<BlsScalar>,
            notes: Vec<Note>,
            memo: String,
            gas_spent: Bigint,
            refund_note: Option<Note>,
        }

        let event = IntermediateEvent::deserialize(deserializer)?;
        let memo = BASE64_STANDARD.decode(event.memo).map_err(SerdeError::custom)?;

        Ok(PhoenixTransactionEvent {
            nullifiers: event.nullifiers,
            notes: event.notes,
            memo,
            gas_spent: event.gas_spent.0,
            refund_note: event.refund_note,
        })
    }
}
@Neotamandua Neotamandua added the priority:minor Low priority issues label Feb 11, 2025
@d-sonuga
Copy link
Collaborator

That is definitely the right way to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:minor Low priority issues
Projects
None yet
Development

No branches or pull requests

2 participants