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

Fix serialization errors #29

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

Conversation

rmgaray
Copy link
Collaborator

@rmgaray rmgaray commented Dec 6, 2024

No description provided.

For some reason, CSL serializes `ScriptPubkey` as a `NativeScript`
instead.

`NativeScript` is a tagged record, of which the first variant (variant
tag `0`) is of type `ScriptPubkey`. CDL represents a `ScriptPubkey` as
a newtype wrapper over a Ed25519KeyHash, so it simply serializes the hash
whenever the `ScriptPubKey` needs to be serialized. But CSL instead
serializes a `ScriptPubKey` by first serializing the tag `0` followed by
the actual hash (so, in reality, it is serializing the parent component
`NativeScript`).

The truth is that there is little reason to serialize a `ScriptPubKey`
alone, because this type is only used as a variant of `NativeScript`. So
this choice has no consequence when serializing/deserializing entire
transactions (both CSL and CDL know how to leverage the
serialize/deserialize methods of `ScriptPubKey` to serialize/deserialize
a `NativeScript` correctly). But this difference in implementation does
trigger errors in our suite, since we test each separate component,
including each branch of every sum type.

For the moment I just blacklisted this variant so it's not added to the
test tree, but this behaviour might happen with other (if not all) tagged
records that use variants with types that are never used alone. If that
is the case, we might need to change how we serialize tagged records and
its variants to match CSL's behaviour.
@rmgaray
Copy link
Collaborator Author

rmgaray commented Dec 9, 2024

@itsfarseen With these final fixes we are finally passing both the serialization and the API suite. I think the PR is ready for review and eventual merging.

@rmgaray rmgaray requested a review from itsfarseen December 9, 2024 18:53
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.

1 participant