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

feat: introduce no-cid-as-bytes feature #23

Merged
merged 4 commits into from
Feb 23, 2024
Merged

feat: introduce no-cid-as-bytes feature #23

merged 4 commits into from
Feb 23, 2024

Conversation

vmx
Copy link
Member

@vmx vmx commented Feb 14, 2024

This commit changes the default behaviour of the deserializer. It's now possible to deserialize CIDs into bytes. This is needed in order to have the Serde derives for internally tagged and untagged enums working correctly.

If you need the current behaviour of erroring at run-time in case a CID is tried to be deserialized as bytes, you can enable the no-cid-as-bytes feature.

BREAKING CHANGE: CIDs may be deserialized into bytes. In order to restore the original behaviour, enable the no-cid-as-bytes feature.

vmx and others added 2 commits February 14, 2024 13:04
This commit changes the default behaviour of the deserializer. It's now
possible to deserialize CIDs into bytes. This is needed in order to have
the Serde derives for internally tagged and untagged enums working correctly.

If you need the current behaviour of erroring at run-time in case a CID
is tried to be deserialized as bytes, you can enable the `no-cid-as-bytes`
feature.

BREAKING CHANGE: CIDs may be deserialized into bytes. In order to restore
the original behaviour, enable the `no-cid-as-bytes` feature.
@rvagg
Copy link
Member

rvagg commented Feb 19, 2024

CIDs may be deserialized into bytes

I think if you want to go with this as the default and document it like this you're going to have to demonstrate how this might happen. It's still a little bit lost on me how this will happen, is it simply that I may have a

    #[derive(Debug, Deserialize, PartialEq)]
    pub enum Boop {
        Cid(Vec<u8>),
    }

And that receives the raw bytes? It seems like users might need guidance on the subtleties here since many are not going to be as familiar with the internals of serde that may this a problem. Perhaps even some test cases for both feature on and feature off that show how this can happen.

@vmx
Copy link
Member Author

vmx commented Feb 19, 2024

Perhaps even some test cases for both feature on and feature off that show how this can happen.

Tests for both cases are part of this PR.

tests/cid.rs Outdated
Comment on lines 88 to 95
#[derive(Debug, Deserialize)]
struct NewType(ByteBuf);

#[derive(Debug, Deserialize)]
#[serde(untagged)]
enum BytesInEnum {
MyCid(NewType),
}
Copy link
Member

Choose a reason for hiding this comment

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

What I'm interested in is what happens when you do this without the feature. Can you add a test for that with these types and run the assertions as expected but also document that this behaviour may be undesirable? This is the key point of the issue right here, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rvagg sorry for the delay, I've pushed a new commit which should address your concerns. If not, please let me know.

Copy link
Contributor

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I can live with this. It'll be a bit problematic if someone tries to import a crate specifying this feature but... we can enable this in the builtin actors and nowhere else.

tests/cid.rs Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Rod Vagg <[email protected]>
@vmx vmx merged commit 2afcb96 into master Feb 23, 2024
6 checks passed
@vmx vmx deleted the no-cid-as-bytes branch February 23, 2024 12:43
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