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

Revert "Add contract spec type for hash (#182)" #184

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Apr 22, 2024

What

Revert "Add contract spec type for hash (#182)"

Why

The Hash type in the SDK is the first time we're adding a type that behaves anything like it does. It's a type that provides some guarantees, and so it can't be used anywhere where those guarantees can't be provided. This means the type really can't be used anywhere at contract boundaries, with the exception being the __check_auth function which is only invocable by the environment itself when auth is being performed.

For that reason the Hash type can't ever show up as a contract function input, and so there's no reason for it to show up in the spec as an input. Even though technically it is an input for the __check_auth function we don't make any of the types of that function included in the spec types because there's no utility in doing so since other contracts can't construct and call the function.

In stellar/rs-soroban-sdk#1246 (comment) I suggested that the Hash type would need to be included in the spec because a contract may wish to return the type from a function. The reason for this suggestion is because it will indeed be possible for someone to return the type from a function, and it would be inconvenient to introduce a compile error for this and require them to convert it to a Bytes. It just feels like one of those arbitrary errors. I meant well with trying to address this but I realise now this introduces a dangerous specification. If we include Hash into the spec someone can write a contract that looks like it guarantees to return a hash, but the system can't support that guarantee and anyone can write a contract that returns a fixed or random Bytes value with the spec saying it's a Hash when it isn't. i.e. This idea was a mistake on my part. To address my earlier concern we can simply map Hash<N> to BytesN<N> when mapping SDK types to spec types.

Notes

This is a breaking change from a raw semver perspective, however I think we can make it for a couple reasons:

  1. The contract spec types, like the overlay types, are not considered part of the protocol and are largely out of scope of any strict compatibility guarantee within a protocol version. We should actually document this compatibility guarantee though, just making it up like this on the spot even with precedence or logic is not the way we should be making decisions like this.

  2. This is a bug fix and should be included in a patch release. I requested Hash be added to the spec, but it was a mistake and no code will have ever been released that uses it.

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.

3 participants