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 contractimport for custom discriminants (#834) #835

Merged
merged 4 commits into from
Jun 23, 2023

Conversation

Smephite
Copy link
Contributor

@Smephite Smephite commented Jan 20, 2023

What

Add the Copy trait to derived custom types when using contractimport

Why

Close #834.

Known limitations

This should not expose any issues as Integer Variant Enums as described in the docs should derive Clone anyhow.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

It looks like this PR adds Copy to all types of contract types, however I think we need to limit it to contracttype on integer enums, and contracterror types.

soroban-spec/src/gen/rust/types.rs Outdated Show resolved Hide resolved
@kalepail
Copy link

What's the recommended work around here? It just seems very odd/unintuitive that you could explicitly declare:

#[contracttype]
#[derive(Copy, Clone, Eq, PartialEq)]
pub enum MapElement {
    Asteroid,
    FuelPod,
}

and yet when getting the bindings from the wasm it's:

#[soroban_sdk::contracttype(export = false)]
#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd)]
pub enum MapElement {
    Asteroid,
    FuelPod,
}

@leighmcculloch
Copy link
Member

Only enums with integer values need the Copy derive.

See the examples here: https://soroban.stellar.org/docs/how-to-guides/custom-types#custom-type-enum

Specifically:

#[contracttype]
#[derive(Clone, Debug, Eq, PartialEq)]
pub enum Enum {
    A,
    B(...),
}
#[contracttype]
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
#[repr(u32)]
pub enum Enum {
    A = 1,
    B = 2,
}

@Smephite
Copy link
Contributor Author

I think @tyvdh's question might've been misleading here, as it is only loosely connected to the issue at hand.
Regarding custom discriminants it is true that only enum with integer values need the Copy trait, but the note by Tyler was that structs deriving Copy in their original source won't derive Copy after the wasm compile -> regenerating bindings using soroban flow.
To resolve this second issue, it is probable that a more sophisticated analysis of the provided struct is required to circumvent the already mentioned issue of the possibility of the struct containing un-copyable types.

As this seems to be only loosely connected to this issue / PR it might be suitable to open a new issue.

@Smephite
Copy link
Contributor Author

I think @tyvdh's question might've been misleading here, as it is only loosely connected to the issue at hand. Regarding custom discriminants it is true that only enum with integer values need the Copy trait, but the note by Tyler was that structs deriving Copy in their original source won't derive Copy after the wasm compile -> regenerating bindings using soroban flow. To resolve this second issue, it is probable that a more sophisticated analysis of the provided struct is required to circumvent the already mentioned issue of the possibility of the struct containing un-copyable types.

As this seems to be only loosely connected to this issue / PR it might be suitable to open a new issue.

Another possibility could be to throw a compiler warning when trying to derive Copy on a contract_type that does not support it.

@leighmcculloch
Copy link
Member

leighmcculloch commented Feb 17, 2023

the note by Tyler was that structs deriving Copy in their original source won't derive Copy after the wasm compile -> regenerating bindings using soroban flow.
To resolve this second issue, it is probable that a more sophisticated analysis of the provided struct is required to circumvent the already mentioned issue of the possibility of the struct containing un-copyable types.

Ah yes, indeed. The contract specification doesn't capture third party trait implementations. We could extend the contract specification to try and capture other things like that. Something worth considering, although not without tradeoffs.

@Smephite
Copy link
Contributor Author

I updated the PR to only add the Copy derive in the generate_enum function.

For my own understanding, I have one question regarding the Spec.

#[contracttype]
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)]
#[repr(u32)]
pub enum MyCoolType {
    ThisTypeIsSadBecauseTheNameIsTooLong = 0,
    Type = 1
}

#[contracttype]
#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd)]
pub enum MyNormalType {
    One(Vec<Symbol>),
    Two
}

in a rust sense both of these are enums, but when inspecting the wasm using the soroban cli the second enum is of "type": "union".
What kind of rust enums will be mapped to the soroban enum?
Is it safe to assume that every enum variant reaching generate_enum(...) will not contain an un-copyable value?

@leighmcculloch leighmcculloch enabled auto-merge (squash) June 23, 2023 14:27
@leighmcculloch leighmcculloch merged commit fa7965d into stellar:main Jun 23, 2023
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.

contractimport fails when importing custom enums with discriminants
3 participants