Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
create vrf documentation #902
create vrf documentation #902
Changes from 1 commit
6661019
32de230
3caecb8
c88b38b
715faf2
fa60f82
7236a2b
64f8343
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not promote this
%
method to generate randoms in a range. It is not secure because of the modulo bias.On the Cadence side, we have purposely made the function accept a modulo argument, and we implemented internally a secure method. The argument was not ported from Cadence to EVM unfortunately.
Developers can always reproduce that safe method with solidity so I suggest to let developers figure out how to use EVM's
revertibleRandom
to generate other safe distributions (random less than a modulo, permutations, shuffles..), maybe we could just avoid this section so we don't promote the unsafe usage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understood the scheme, or at least this isn't the commit-reveal scheme Flow allows to be used on Cadence and EVM.
@sisyphusSmiling did a nice job explaining the idea here. We could use it to update this section. Btw the commit-reveal scheme doesn't use
revertibleRandom
, it uses a Cadence contract via another arch calledgetRandomSource
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitting the hash doesn't seem to add much to the scheme. The bet structure itself could be stored instead of the hash, and we can check later that the user didn't alter their bet. Or maybe it's a way to reduce storage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revealBet
and decide to revert the result. The user can runrevealBet
in a later block using a different randomness, and try till they win. This is the problem described inrevertibleRandom
, and this has the same issue since it usesrevertibleRandom
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a proper reading @tarakby. I overlooked that this contract uses
revertibleRandom
and assumed that it was usinggetRandomSource
in my review