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

Convert references based on borrow type #3213

Merged

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Apr 2, 2024

Work towards #3081

Description

In Cadence v0.42, the valueType (which is a ReferenceStaticType) included not only the referenced-type, but also the "borrowed type" of the value. So if the borrowed-type/referenced-type was different than the expected-type, then the conversion happened. This was added in https://github.com/dapperlabs/cadence-internal/pull/201.

However, in Cadence 1.0, the the ReferenceStaticType (and therefore valueType) no longer contains the borrow-type. So even if the borrowed-type was different, the existing check would see the valueType and the expectedType as equal, and the conversion wouldn't happen. So this change here is to also consider the borrowed-type, just like it used to, but now the borrow-type has to be taken from the ReferenceValue, because ReferenceStaticType no longer has it.

The actual fix is #3213 (comment).


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@SupunS SupunS marked this pull request as ready for review April 3, 2024 00:10
@SupunS SupunS force-pushed the supun/fix-reference-conversion branch from 020d36a to 0077f05 Compare April 3, 2024 16:17
@turbolent
Copy link
Member

turbolent commented Apr 3, 2024

@SupunS Could you please add a description of what the PR does and why? I tried to have a look at the PR, but I don't quite see what the changes are and why they're needed (what the problem is). Thank you!

@SupunS
Copy link
Member Author

SupunS commented Apr 3, 2024

@turbolent #3213 (comment) has a description on the change. I put it as a comment, because then it is easier to point to where the actual fix is. I'll copy this over to the description as well.

@SupunS
Copy link
Member Author

SupunS commented Apr 3, 2024

Updated the description

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

It somewhat makes sense to me what was done before and how it needs to be adjusted for Cadence 1.0 👍

The TODO/Skip is left because we still need to investigate why the test fails, but still want to unblock #3079 and get that in before?

runtime/interpreter/interpreter.go Show resolved Hide resolved
@SupunS
Copy link
Member Author

SupunS commented Apr 3, 2024

The TODO/Skip is left because we still need to investigate why the test fails, but still want to unblock #3079 and get that in before?

Yes. I already figured out the cause, but needed to figure out a solution. It probably needs some extra work, so don't want to block the remaining fixes until we figure that one out.

@SupunS SupunS merged commit 1ea69b1 into bastian/v1.0-port-v0.42.8-patch.4 Apr 3, 2024
@SupunS SupunS deleted the supun/fix-reference-conversion branch April 3, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants