refactor: whisper: Add type aliases and update rustdocs in message.rs #10812

ltfschoen · 2019-06-28T06:29:29Z

Add types aliases to message.rs and update Rustdocs
* [ ] Add missing memzero crate - N/A
* [ ] Fix use of parity_util_mem instead of memzero - N/A

Preview changes

cargo doc -p parity-whisper --no-deps --open

parity-cla-bot · 2019-06-28T06:29:32Z

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

Many thanks,

Parity Technologies CLA Bot

niklasad1 · 2019-06-28T09:27:32Z

whisper/src/message.rs


-			if data[3] & (1 << i) != 0 {
-				idx += 256;
+			// What does `1 << i` refer to


1 << i is the same as 2^i

thus, 2^0, 2^1, 2^2.. 2^n .. -> 1, 2, 4, 8,, ...

I for one decidedly prefer using constants or explicit numbers whenever possible.

niklasad1 · 2019-06-28T09:35:09Z

@ltfschoen

[x] Add missing memzero crate
[x] Fix use of parity_util_mem instead of memzero

I can't find those changes, are they missing?

ltfschoen · 2019-06-28T15:45:43Z

@ltfschoen

[x] Add missing memzero crate
[x] Fix use of parity_util_mem instead of memzero

I can't find those changes, are they missing?

Sorry for the confusion. I'm confused as well. I think we can ignore those dot points now. What I think happened this morning is that I got an error suggesting that use memzero needed me to add extern crate memzero, but I think VSCode may have added use memzero automatically or something. I think what's happened is that I tried adding the memzero dependency to the Cargo.toml and then adding extern crate memzero, but then I must have realised that we were using parity_util_mem instead. So then I must have reverted my memzero changes and the error went away.

niklasad1

LGTM

dvdplm

Overall looks good, great job. Using the newtype pattern like this is good for readability although it miiight be a little much here: in some cases it's just easier to read H256 than SomeOtherType as it requires the reader to be familiar with the types used here. There could also be extra friction on the API borders to other code, but time will tell.

dvdplm · 2019-07-03T07:20:08Z

whisper/src/message.rs

@@ -291,18 +324,23 @@ impl Message {
 			digest
 		};

+		// Find the best nonce based on using updating the digest with


"…using updating…" – not sure what this means, can you elaborate?

dvdplm · 2019-07-03T07:21:41Z

whisper/src/message.rs

-	// does checks for validity.
-	fn from_components(envelope: Envelope, size: usize, hash: H256, now: SystemTime)
+	// Create message from envelope, hash, and encoded size.
+	// Does checks for validity.


Maybe?

Suggested change

// Does checks for validity.

// Performs validity checks

dvdplm · 2019-07-03T07:27:43Z

whisper/src/message.rs

+type EnvelopeProvenWork = f64;
+/// Time-to-live of an envelope in seconds.
+type EnvelopeTTLDuration = u64;
+
 /// Work-factor proved. Takes 3 parameters: size of message, time to live,
 /// and hash.
 ///
 /// Panics if size or TTL is zero.


Document what the return value actually means.

dvdplm · 2019-07-04T11:45:52Z

@ltfschoen merging this now, feel free to address remaining grumbles in a diff PR.

…me-parent * master: refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812) Break circular dependency between Client and Engine (part 1) (#10833) tests: Relates to #10655: Test instructions for Readme (#10835) refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657) idiomatic changes to PodState (#10834) Allow --nat extip:your.host.here.org (#10830) When updating the client or when called from RPC, sleep should mean sleep (#10814) Remove excessive warning (#10831) Fix typo in README.md (#10828) ethcore does not use byteorder (#10829) Better logging when backfilling ancient blocks fail (#10796) depends: Update wordlist to v1.3 (#10823)

* master: refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812) Break circular dependency between Client and Engine (part 1) (#10833) tests: Relates to #10655: Test instructions for Readme (#10835) refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657) idiomatic changes to PodState (#10834) Allow --nat extip:your.host.here.org (#10830) When updating the client or when called from RPC, sleep should mean sleep (#10814) Remove excessive warning (#10831) Fix typo in README.md (#10828) ethcore does not use byteorder (#10829) Better logging when backfilling ancient blocks fail (#10796) depends: Update wordlist to v1.3 (#10823) cargo update -p smallvec (#10822) replace memzero with zeroize crate (#10816) Don't repeat the logic from Default impl (#10813) removed additional_params method (#10818) Add Constantinople eips to the dev (instant_seal) config (#10809)

* master: (21 commits) refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812) Break circular dependency between Client and Engine (part 1) (#10833) tests: Relates to #10655: Test instructions for Readme (#10835) refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657) idiomatic changes to PodState (#10834) Allow --nat extip:your.host.here.org (#10830) When updating the client or when called from RPC, sleep should mean sleep (#10814) Remove excessive warning (#10831) Fix typo in README.md (#10828) ethcore does not use byteorder (#10829) Better logging when backfilling ancient blocks fail (#10796) depends: Update wordlist to v1.3 (#10823) cargo update -p smallvec (#10822) replace memzero with zeroize crate (#10816) Don't repeat the logic from Default impl (#10813) removed additional_params method (#10818) Add Constantinople eips to the dev (instant_seal) config (#10809) removed redundant fmt::Display implementations (#10806) revert changes to .gitlab-ci.yml (#10807) Add filtering capability to `parity_pendingTransactions` (issue 8269) (#10506) ...

* master: Extricate PodAccount and state Account to own crates (#10838) logs (#10817) refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812) Break circular dependency between Client and Engine (part 1) (#10833) tests: Relates to #10655: Test instructions for Readme (#10835) refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)

…hore/extricate-state-backend * dp/chore/extricate-account-db-into-own-crate: third time's the charm test failure 2 test failure Extricate PodAccount and state Account to own crates (#10838) logs (#10817) refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812) Break circular dependency between Client and Engine (part 1) (#10833) tests: Relates to #10655: Test instructions for Readme (#10835) refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)

refactor: Add type aliases to Whisper and update rustdocs

4b618be

ltfschoen added the A0-pleasereview 🤓 Pull request needs code review. label Jun 28, 2019

niklasad1 reviewed Jun 28, 2019

View reviewed changes

remove my question that was answered

a3e7328

niklasad1 approved these changes Jun 29, 2019

View reviewed changes

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 29, 2019

ordian added this to the 2.6 milestone Jun 30, 2019

dvdplm approved these changes Jul 3, 2019

View reviewed changes

dvdplm merged commit bacc0f0 into master Jul 4, 2019

niklasad1 deleted the luke-whisper-message-refactor branch November 12, 2019 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: whisper: Add type aliases and update rustdocs in message.rs #10812

refactor: whisper: Add type aliases and update rustdocs in message.rs #10812

ltfschoen commented Jun 28, 2019 •

edited

Loading

parity-cla-bot commented Jun 28, 2019

niklasad1 Jun 28, 2019 •

edited

Loading

dvdplm Jul 3, 2019

niklasad1 commented Jun 28, 2019

ltfschoen commented Jun 28, 2019

niklasad1 left a comment

dvdplm left a comment

dvdplm Jul 3, 2019

dvdplm Jul 3, 2019

dvdplm Jul 3, 2019

dvdplm commented Jul 4, 2019

refactor: whisper: Add type aliases and update rustdocs in message.rs #10812

refactor: whisper: Add type aliases and update rustdocs in message.rs #10812

Conversation

ltfschoen commented Jun 28, 2019 • edited Loading

Preview changes

parity-cla-bot commented Jun 28, 2019

niklasad1 Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

dvdplm Jul 3, 2019

Choose a reason for hiding this comment

niklasad1 commented Jun 28, 2019

ltfschoen commented Jun 28, 2019

niklasad1 left a comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm Jul 3, 2019

Choose a reason for hiding this comment

dvdplm Jul 3, 2019

Choose a reason for hiding this comment

dvdplm Jul 3, 2019

Choose a reason for hiding this comment

dvdplm commented Jul 4, 2019

ltfschoen commented Jun 28, 2019 •

edited

Loading

niklasad1 Jun 28, 2019 •

edited

Loading