CashScript / cashscript

⚖️ Easily write and interact with Bitcoin Cash smart contracts
https://cashscript.org
MIT License
112 stars 79 forks source link

Tuple Assignment #101

Closed nathanielCherian closed 2 years ago

nathanielCherian commented 2 years ago

Tuple destructuring to two variables. #23

rkalis commented 2 years ago

You're on fire 💪

Thanks for the PR, I'll look at it in more detail next week. One thing to think about is do we need to have two types for the two variables. Let's say we have a bytes32 and we split it at 8, we'll end up with a bytes8 and a bytes24 so ideally we'd be able to retain those types rather than storing them both as just bytes.

nathanielCherian commented 2 years ago

Thanks! I just realized that I had implemented the typecheck incorrectly (didn't work for bytes) so I went back and fixed that. Currently, bytes of declared size can be split but fails when trying to bound the variables ie bytes8 x, y = bytes16.split(8);. I'll add logic to determine the sizes of both halves.

Were you thinking to store the bounds implicitly or allow them to be declared in the program. bytes8 x, bytes24 y = bytes32.split(8)?

Edit: In the case that the split index relies on an unknown parameter, I don't think its possible to get the sizes for the resulting bytes.

rkalis commented 2 years ago

Hey @nathanielCherian, sorry for dropping the ball on this one. I'm on a sort of semi-holiday so I don't have a lot of time and forgot to get back to you.

I think probably what we want to do is go with the second option, where we have to specify the type of both sides, so bytes8 x, bytes24 y = bytes32.split(8). I think in this case it's good to be a bit more epxlicit about it so the user doesn't make any mistakes in the process.

nathanielCherian commented 2 years ago

No worries! Agreed on the explicit declaration. We could standardize the notation to avoid confusion so even string x, y = ... must be declared as string x, string y = ....

Also is there a way to get an expressions type during symbol table traversal or AST build?

rkalis commented 2 years ago

Hey Nathan! Just got back from my holiday, so I finally have time to really look aat this PR!

No worries! Agreed on the explicit declaration. We could standardize the notation to avoid confusion so even string x, y = ... must be declared as string x, string y = ....

I think we probably do want to go with the standardisation that you mentioned, so even if both sides of the split are the same (like with string) you would have to specify the type for both sides (string x, string y = ...).

Also is there a way to get an expressions type during symbol table traversal or AST build?

Types are added to all expression nodes during the type check traversal, so before that time the only available types are for "literal nodes". The order is like that because the type checking traversal needs to come after the symbol table traversal because it needs to know the definitions of identifiers.

rkalis commented 2 years ago

Just went over the code. Overall looks nice, just had a few comments about where to put certain code.

nathanielCherian commented 2 years ago

Hey Rosco, sorry for the late reply, and thanks for answering my question! I moved the code from AstBuilder to the type check traversal and cleaned up some other files. It works as is now but I was wondering what checks to place on the types during assignment. Should a type-check error be thrown if the user...

  1. specifies byte-size if the original tuple size is unknown (byte16 b1, byte16 b2 = bytes(b).split(16))
  2. specifies a byte-size that is greater than the known tuple size. (byte32 b1, byte32 b2 = bytes32(b).split(16))
rkalis commented 2 years ago

Hey Nathan, I just updated the HODL Vault example and all related tests to use the new destructuring assignment syntax. Also updated the docs and some other tests, so looks ready to merge!

Thanks so much for the PR, really looking forward to having this functionality available!

In terms of release, there's some other things I need to sort out, so I may wait a bit longer to see if I can include some more things in the release.

For future reference and as a sort of summary of the discussion above:

Since we don't always know what the types of the tuple will be at compile-time, it is difficult to perform fine-grained typechecking that includes bytesX types for the resulting tuple elements. So for now the only checking that we do is that both sides of the = are the same type, disregarding any bounded bytes checks.

nathanielCherian commented 2 years ago

Awesome! Many thanks for your help and clarification! I'm also working on #13, I'll try to submit before next release as well.

rkalis commented 2 years ago

That's great to hear! I'll probably be able to provide some more timely review this time around! I'll personally be working on getting CashScript working with the latest testnet functionality (native introspection + bigint) in the coming weeks so we'll be ready for the May 2022 network upgrade 💪