onflow / flips

Flow Improvement Proposals
25 stars 22 forks source link

[Cadence] FLIP - random function #120

Closed tarakby closed 1 year ago

tarakby commented 1 year ago

Proposes a FLIP to:

dsainati1 commented 1 year ago

My only concern with this change is that I think it may communicate the wrong thing to Cadence users. I understand how from the protocol perspective our randomness is no longer "unsafe", but from the perspective of a Cadence user, I think they will probably not consider it to be such. As far as they are concerned, the randomness (used naively) is still "unsafe" because it is still exploitable by a transaction that reverts to postselect a favorable result. Changing the name of the function to remove the unsafe prefix I think would suggest to people that the function is "safe" in the sense that it is immune to this exploit, which it's not.

Users can use certain patterns to make their randomness truly safe, but they need to structure their code in a specific way for this to work, and I am worried that people won't realize they need to do this without the built-in warning that a name like unsafeRandom provides. IMO we should have the name (or the API) here indicate that the user needs to do something additional beyond just calling the function to get a random number; something like revertibleRandom, for example.

tarakby commented 1 year ago

Thanks @dsainati1

I see your point but I'll add counter-arguments to keep the discussion going:

turbolent commented 1 year ago

Great points @dsainati1 and @tarakby! @tarakby Could you please incorporate the concerns and your feedback into the FLIP itself? That way the reasoning is not lost

janezpodhostnik commented 1 year ago

Would it be better if FVM was injecting the random function (via Environment.Declare(valueDeclaration stdlib.StandardLibraryValue)) so that it would be entirely handled by the FVM this way we could remove the random from the runtime interface entirely. @turbolent?

turbolent commented 1 year ago

@janezpodhostnik That could be an option. There's always the question what should be part of the built-in functionality / standard library, and what is only provided by a particular environment.

So far, the random function is part of the built-in functionality (standard library). I feel like every implementation of Cadence should provide one, so developers can be assured the function is available and reason about the behaviour. For now I'd keep it that way, however, I don't feel strongly about it.

(The same question / reasoning applies to other functions/APIs (like the block API).

turbolent commented 1 year ago

The plan is to decide on this FLIP in the next Cadence Language Design Meeting on Tue 12th, Sep.

So far there is positive sentiment. Unless there are any new insights or significant reasons to reject are brought forward, the plan is to approve the FLIP.

turbolent commented 1 year ago

@tarakby The FLIP got approved 🎉 Please update the status and merge :-)