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

Support variants with multiple fields in UDTs #850

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

brson
Copy link
Contributor

@brson brson commented Feb 2, 2023

What

Add support for UDT enums with variants containing up to 12 fields.

Add more test cases.

Why

UDT enum variants are currently limited to a single field, which seems arbitrary and surprising.

Known limitations

This does not add support for struct-like variants.

The spec for tuple-like variants is now a ScpSpecUdtUnionCaseV0 of ScSpecTypeTuple:

        ScSpecUdtUnionCaseV0 {
            name: name.try_into().unwrap_or_else(|_| StringM::default()),
            type_: Some(ScSpecTypeDef::Tuple(Box::new(ScSpecTypeTuple {
                value_types: field_types,
            }))),
        }

Please consider whether this is correct. I am not sure.

This patch requires updates to rs-stellar-xdr and rs-soroban-sdk to support conversions of 13-tuples:

This patch went through a lot of iteration and ended up making a bigger diff than necessary.

cc #153

@leighmcculloch
Copy link
Member

The spec for tuple-like variants is now a ScpSpecUdtUnionCaseV0 of ScSpecTypeTuple:

        ScSpecUdtUnionCaseV0 {
            name: name.try_into().unwrap_or_else(|_| StringM::default()),
            type_: Some(ScSpecTypeDef::Tuple(Box::new(ScSpecTypeTuple {
                value_types: field_types,
            }))),
        }

I think this will result in code generated that doens't match the original code. For example:

type Enum enum {
    Variant(A1, A2)
}

Will result inthe following being code generated:

type Enum enum {
    Variant((A1, A2))
}

So I think we need to change the XDR so that the type_ value can be a vec instead of a optional, so that it can contain one or more types.

@leighmcculloch
Copy link
Member

The smallest diff we could make to the spec to support it is:

diff --git a/Stellar-contract-spec.x b/Stellar-contract-spec.x
index 56c3adf..d26c169 100644
--- a/Stellar-contract-spec.x
+++ b/Stellar-contract-spec.x
@@ -139,7 +139,7 @@ struct SCSpecUDTUnionCaseV0
 {
     string doc<SC_SPEC_DOC_LIMIT>;
     string name<60>;
-    SCSpecTypeDef *type;
+    SCSpecTypeDef type<12>;
 };
 
 struct SCSpecUDTUnionV0

But maybe we should make that struct a union, and explicitly distinguish between unit and tuple variants.

@leighmcculloch
Copy link
Member

I'm making some changes to the spec xdr to support multiple tuples in the spec itself: stellar/stellar-xdr#67.

@brson
Copy link
Contributor Author

brson commented Feb 5, 2023

Thanks for making the XDR changes for me.

I started upgrading rs-soroban-sdk to the latest XDR until I discovered the new doc fields, and the open questions about it in chat. Do you want me to hold off and let you deal with the SDK upgrade?

@leighmcculloch
Copy link
Member

Sorry about that, the docs changes are in #841 which I plan to finish and need on Monday. You could base your changes on that PR though if you wanted to move ahead.

@brson
Copy link
Contributor Author

brson commented Feb 10, 2023

Rebased.

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.

2 participants