fix(promise): PromiseOrValue sets should_return flag correctly on serialization

[austinabell]

Looking through the code, this seems to be the change that fixes #285

I will go create a minimal repro or see if this can be done in a test to make sure this doesn't come back

closes #285

[evgenykuzyakov]

Looks good. Can you confirm it. Probably can use cross-contract-high-level for this

[austinabell]

Looks good. Can you confirm it. Probably can use cross-contract-high-level for this

Will do now. Question, why is serde serialize implemented for promise, but borsh serialize isn't? Seems weird that Promise isn't but PromiseOrValue is. Should there be a borsh serialize which sets the return flag, or is that always done through serde for cross contract returns?

[austinabell]

Okay so I've dug a bit deeper, the return was set during a From<Promise> for the PromiseOrValue which seems like a strange side effect, which is why that example worked. I will try to restructure to remove that side effect, unless you indicate a reason for this to be the case

[austinabell]

So to be clear what has changed since your review is:

• Implemented borsh::BorshSerialize for Promise, which sets the should_return flag
  • Promise could only be used as the return value if it was serde::Serialize before
• Removed setting the should_return flag on conversion from Promise to PromiseOrValue
  • This side effect causes issues that could lead to a bug if the types are converted but not used as the return value
• Derived serialize for PromiseOrValue with untagged annotation, which is the same functionally as it would be if manually implemented

I've rebuilt and tested all examples, and can now look into writing tests to hit these other cases

[matklad]

As a drive by observer, this line of code (and the one for serde) looks reeeeeealy weird to me. I think it makes sense to at least write a comment here explaining what exactly are we doing here and why. I also have a very strong gut feeling that there should be a less magical way to achieve the desired effect, but I need to dig deeper into the code to confirm that.

[austinabell]

I agree, I'm just trying to make the least cost change to avoid breakage. This will definitely be a thing to keep in mind as evolving the API because this seems like it has the potential to be a footgun

Edit: And also I don't know if you see but there is this same pattern for the serde::Serialize, this just brings it inline with that to not have inconsistencies between serializations

[mikedotexe]

Love to see these fixes for community-created issues. Thank you