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

Type Safety in Collections: Remove generics from collection types #165

Closed
wants to merge 4 commits into from

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Jul 5, 2022

What

Remove the generic types from the collection types in the SDK:

  • Vec<T> => Vec
  • Map<K, V> => Map

Why

It is convenient to represent the Vec and Map types as homogeneous data structures, but in reality there is nothing in the data definition (XDR) or the runtime environment that guarantees they will be. The SDK misrepresents the underlying data by presenting the collections as homogeneous, and while this misrepresentation is convenient in the common case, it could also be a cause of vulnerabilities since developers could assume the types are backed by values of the same type when they are not. @graydon, @jonjove, and I have discussed several options, including making the types homogeneous in the XDR, and including runtime checks. However, since the SDK type system is likely to be a super set of the runtime environment there are edge cases the host would be unable to test against.

Removing the generics from the types themselves and moving them to the functions so that the functions still aid with conversion seems like the most explicit way to make these types exhibit reality.

Related discussion https://discord.com/channels/897514728459468821/989953055829135380/993984627180056749

Close #152

Known limitations

This will introduce some ambiguity into contracts.

❗ A Vec will allow any type to be added to it, even if the developer intends only a specific type to be added.

‼️ Also, exported types will no longer have Vec<T> as their field type, but instead just Vec. While this describes reality better, it is less effective at communicating the intent for what data should be placed in the Vec, and therefore this harms the developer experience. It seems better to harm the developer experience if the alternative is footgun code.

TODO

The contract spec is not updated in this PR, and needs updating to remove the generic types from the specification of the collection types.

@leighmcculloch
Copy link
Member Author

A Vec will allow any type to be added to it, even if the developer intends only a specific type to be added.

An example of this is in the tests of this PR, where it is really easy to add the wrong type of integer to the Vec. This would be event easier in a contract interface, where I expect u32, but someone passes u64s. There would be nothing in the generated code to help someone make the right call.

@leighmcculloch leighmcculloch changed the title Remove generics from collection types Type Checking: Remove generics from collection types Jul 6, 2022
@leighmcculloch leighmcculloch mentioned this pull request Jul 6, 2022
4 tasks
@leighmcculloch
Copy link
Member Author

I changed the Vec impl in this PR to try a middleground where the Vec type is presented as containing only types of type T, and all the mutation functions only allow adding T, but the accessor functions don't guarantee the return value is of type T. In theory it felt like it might achieve something, but it makes the API inconvenient to use. As a developer accessing a Vec I have to hardcode what the return type will be in the code, which increases the chance I'm creating runtime panics out of human error.

@leighmcculloch
Copy link
Member Author

Overall I think this approach achieves the goal of creating visibility of the fact the underlying Vec is not guaranteed to deserialize, but in the process of addressing that issue this change introduces new problems and new footguns through runtime errors. I'm not pursuing this change due to this.

@leighmcculloch leighmcculloch deleted the vecmapfngen branch July 6, 2022 19:36
@leighmcculloch leighmcculloch self-assigned this Jul 6, 2022
@leighmcculloch leighmcculloch changed the title Type Checking: Remove generics from collection types Type Safety in Collections: Remove generics from collection types Jul 12, 2022
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.

Type Safety in Collections
1 participant