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

Document encoding of round in message #80

Closed
webmaster128 opened this issue Aug 17, 2020 · 4 comments · Fixed by #87
Closed

Document encoding of round in message #80

webmaster128 opened this issue Aug 17, 2020 · 4 comments · Fixed by #87
Labels
dif/easy Someone with a little familiarity can pick up effort/hours Estimated to take one or several hours kind/enhancement A net-new feature or an improvement to an existing feature P2 Medium: Good to have, but can wait until someone steps up topic/docs Documentation

Comments

@webmaster128
Copy link
Contributor

First of all, thank you for the great documentation! When implementing a drand verifier in Rust, this was extremly helpful. There is one thing I needed to lookup in Go code which was not specified:

In the blocks Randomness generation and Beacon signature the message format is documented via Go code:

func Message(currRound uint64, prevSig []byte) []byte {
	h := sha256.New()
	h.Write(prevSig)
	h.Write(roundToBytes(currRound))
	return h.Sum(nil)
}

However, it is unclear how currRound is encoded to bytes. It turns out the encoding is fixed length, 8 bytes big endian. It would be good to either document that somehow or just rename roundToBytes to uint64ToBytesBigEndian or something like that. This makes clear at first glance for non-Go developers that there is nothing special hidden in roundToBytes.

@alanshaw
Copy link
Contributor

alanshaw commented Sep 7, 2020

Thanks for bring this up - would you be willing to make a PR to https://github.com/drand/website/blob/master/docs/docs/specification.md to get this information documented? I think a PR to enhance the comment here would also be much appreciated 😁.

@alanshaw alanshaw added dif/easy Someone with a little familiarity can pick up effort/hours Estimated to take one or several hours kind/enhancement A net-new feature or an improvement to an existing feature P2 Medium: Good to have, but can wait until someone steps up topic/docs Documentation labels Sep 7, 2020
@webmaster128
Copy link
Contributor Author

@alanshaw sure, no problem. What do you think about renaming roundToBytes to uint64ToBytesBigEndian instead of explaining how roundToBytes works? Or do you want to keep the higher level name?

@alanshaw
Copy link
Contributor

alanshaw commented Sep 7, 2020

Personally I'd leave it as roundToBytes - I don't remember seeing any precedent within the code base for naming any other functions that do encoding very explicitly like your suggestion so I'd leave it for consistency and just make sure it's documented.

cc @nikkolasg for any override 😁

@nikkolasg
Copy link
Contributor

@webmaster128 hey there - Yeah i'd prefer to keep as roundToBytes because the name of the function indicates clearly what it does without going into the specific of how it does it - which is imo a good function name. If you want to know how it does it, then look at the function.
This has the added benefit that we can change the encoding without changing the function name, i.e. encapsulation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dif/easy Someone with a little familiarity can pick up effort/hours Estimated to take one or several hours kind/enhancement A net-new feature or an improvement to an existing feature P2 Medium: Good to have, but can wait until someone steps up topic/docs Documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants