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

Enable retrieving both transactions and blocks in json format #1155

Merged
merged 12 commits into from
Aug 10, 2023

Conversation

TheQuantumPhysicist
Copy link
Collaborator

No description provided.

Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

Just gave it a quick first pass.

Besides the minor comments below, one more thought regarding the serialization of binary data. Binary data should really be encoded using encode_bytes rather than encode_str. However, I suspect serde-json encodes bytes as an json array of numbers. That could be addressed by creating a wrapper serializer that forwards encode_bytes to encode_str by converting it into hex and passes through everything else into the inner serializer. One shortcoming is that convenience methods like serde_json::to_string will no longer be available since they hard-code the serializer from the crate. Thoughts?

Comment on lines 33 to 35
impl serde::Serialize for InputWitness {
fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
serializer.serialize_str(&self.hex_encode())
Copy link
Contributor

Choose a reason for hiding this comment

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

Serialize for Destination adds the 0x prefix but here InputWitness does not. Is there a reason for the distinction? Is it to distinguish Destination encoded as hex vs bech32?

On a vaguely related note, maybe instead of calling serialize_str directly, we could exploit the fact that HexEncoded already implements Serialize:

HexEncoded::new(self).serialize(serializer)

The serialization of binary data in hex will have a unified code path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. It's to make that distinction.

Done that serialization thing.

crypto/Cargo.toml Outdated Show resolved Hide resolved
serialization/src/json_encoded.rs Show resolved Hide resolved
serialization/src/json_encoded.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

Still somewhat puzzling over the from_string name. Good otherwise.

@TheQuantumPhysicist TheQuantumPhysicist merged commit 29ac4c7 into master Aug 10, 2023
23 checks passed
@TheQuantumPhysicist TheQuantumPhysicist deleted the feat/json_blocks branch August 10, 2023 11:56
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.

None yet

4 participants