Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@expenses
Copy link
Contributor

@expenses expenses commented Sep 26, 2019

This closes #1451.

@parity-cla-bot
Copy link

It looks like @expenses signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

1 similar comment
@parity-cla-bot
Copy link

It looks like @expenses signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

expenses and others added 2 commits September 27, 2019 19:20
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks quite good in general. Needs fixing a few minor issues + maybe some tests would also be good.

@bkchr bkchr requested a review from rphmeier September 27, 2019 08:09
@bkchr
Copy link
Member

bkchr commented Sep 27, 2019

Could you please add some sort of tests?

@expenses
Copy link
Contributor Author

both of those would do the trick, but ideally I'd like to have something more concise.

@expenses
Copy link
Contributor Author

Would something like an ArrayVec be useful here, given we're only storing up to 81 hashes?

Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Looks reasonable.

@expenses expenses requested a review from bkchr October 6, 2019 03:57
@rphmeier
Copy link
Contributor

rphmeier commented Oct 6, 2019

LGTM after a crate rename - it shouldn't be called randomness unless it's the only / forever guaranteed / safest way to do randomness. which it is none of

@expenses
Copy link
Contributor Author

expenses commented Oct 7, 2019

@rphmeier I've renamed the crate. Should documentation such as

//! # Randomness Module
//!
//! The Randomness module provides a [`random`](./struct.Module.html#method.random) function that
//! generates low-influence random values based on the block hashes from the previous 81 blocks.
//! Low-influence randomness can be useful when defending against relatively weak adversaries.

be renamed too?

@kianenigma
Copy link
Contributor

@rphmeier I've renamed the crate. Should documentation such as

//! # Randomness Module
//!
//! The Randomness module provides a [`random`](./struct.Module.html#method.random) function that
//! generates low-influence random values based on the block hashes from the previous 81 blocks.
//! Low-influence randomness can be useful when defending against relatively weak adversaries.

be renamed too?

None of by business but I assume yes. Make sure to also merge master before merging this to make sure all external usages of randomness::randomness() etc. are properly refactored

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Some last nitpicks, otherwise looks good.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Sorry, 2 last nitpicks :(

Could you please prepare a pr for polkadot using this guide: https://github.com/paritytech/substrate/blob/master/CONTRIBUTING.adoc#updating-polkadot-as-well

When the Polkadot pr is ready, I will merge this one. (Please make sure your branch in Polkadot is editable by us, so that I can finish it).

@rphmeier rphmeier merged commit f3a830d into paritytech:master Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Split SafeMix and random_seed out of System module.

9 participants