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

revertible random #15

Closed
wants to merge 1 commit into from
Closed

revertible random #15

wants to merge 1 commit into from

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Nov 7, 2023

  • add doc for revertibleRandom
  • update doc for unsafeRandom

Changes are copied from onflow/docs#423

@tarakby tarakby requested a review from bthaile November 7, 2023 18:33
Copy link

vercel bot commented Nov 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
cadence-lang-docs ✅ Ready (Inspect) Visit Preview Nov 7, 2023 6:33pm
cadence-lang-org ✅ Ready (Inspect) Visit Preview Nov 7, 2023 6:33pm

@tarakby tarakby changed the title add revertible random and update unsafe random revertible random Nov 7, 2023
Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

Very nice. Added some suggestion for minor word-smithing.

Comment on lines +39 to +42
<Callout type="info">
`unsafeRandom()` function currently behaves the same way as `revertibleRandom()`.
It is deprecated and will be removed in an upcoming release of Cadence.
</Callout>
Copy link
Member

Choose a reason for hiding this comment

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

do we need this here? We already have a very similar callout for unsafeRandom:

<Callout type="info">
`unsafeRandom()` function currently behaves the same way as `revertibleRandom()`.
It is deprecated and will be removed in an upcoming release of Cadence.
</Callout>

I feel this is largely duplicated and best fits within the context of unsafeRandom

Suggested change
<Callout type="info">
`unsafeRandom()` function currently behaves the same way as `revertibleRandom()`.
It is deprecated and will be removed in an upcoming release of Cadence.
</Callout>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually duplicated it purposely. I think there is an advantage to clarify why this function is here and what's its relationship with the existing unsafeRandom. Developers used to unsafeRandom may be asking how revertible is different and this provides them with a quick answer.

I added the section for unsafeRandom afterwards as per your suggestion to keep a section for deprecated functions. I think the duplication is minor and it is temporary anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay you guys win 😄 onflow/docs#439 (comment)

Comment on lines +86 to +87
`unsafeRandom` has the same interface and implementation as `revertibleRandom()` although
it is called unsafe.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`unsafeRandom` has the same interface and implementation as `revertibleRandom()` although
it is called unsafe.
Its implementation has been upgraded to the same implementation as `revertibleRandom()` for improved security. However, the name was retained for downwards compatibility despite it technically being no longer unsafe (see `revertibleRandom()` for details).

@turbolent
Copy link
Member

We'll do a final sync by copying all content from the docs repo to this repo before removing the content from there. Please make changes in the docs repo, otherwise we'll end up with conflicts.

@turbolent turbolent closed this Nov 7, 2023
@tarakby
Copy link
Contributor Author

tarakby commented Nov 7, 2023

cc @gregsantos

@tarakby
Copy link
Contributor Author

tarakby commented Nov 7, 2023

@AlexHentschel thanks for the review.
I think the comments belong to the original PR onflow/docs#423. I'll open a new PR against the original docs to address your comments from here.

Update: here is the PR with your suggestions

@tarakby tarakby deleted the tarak/revertible-random branch December 23, 2023 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants