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

main: add address encoding magic constants test #1458

Merged
merged 3 commits into from
Sep 26, 2018

Conversation

JFixby
Copy link
Contributor

@JFixby JFixby commented Sep 15, 2018

Ensures address encoding magics and params.NetworkAddressPrefix are consistent with each other. This test will light red when a new network is started with incorrect values.

Also there is a minor fix of *pointers for two other tests in this file.

Ensures address encoding magics and NetworkAddressPrefix are consistent with each other. This test will light red when a new network is started with incorrect values.
@JFixby JFixby changed the title main: add magic constants test main: add address encoding magic constants test Sep 15, 2018
@dnldd
Copy link
Member

dnldd commented Sep 23, 2018

Good catch for the non-pointer chaincfg.Params, the failure conditions for the encoding tests are if the constants get modified or CheckEncode breaks right? If that's the case then I think you should look at adding tests to github.com/decred/base58 instead since it's highly unlikely the defined constants will change.

@JFixby
Copy link
Contributor Author

JFixby commented Sep 23, 2018

This test ensures address encoding magics and params.NetworkAddressPrefix are consistent with each other.

For example: the test will fail when someone changed params.NetworkAddressPrefix but forgot to update or updated incorrectly values in the params.PubKeyAddrID, params.PubKeyHashAddrID, params.PKHEdwardsAddrID, params.PKHSchnorrAddrID, params.ScriptHashAddrID and params.PrivateKeyID.

Let's say a new network is started and the params.NetworkAddressPrefix is set to be K.
What would be the corresponding value for the params.PubKeyAddrID?
Is it [0xfe, 0x07]? Is the math correct?
The test will report when it is not.

@@ -70,13 +72,79 @@ func checkGenesisBlockRespectsNetworkPowLimit(
}
}

func checkPrefix(t *testing.T, prefix string, encoded, networkName string) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing function comment.

@@ -49,7 +51,7 @@ func checkPowLimitsAreConsistent(t *testing.T, params chaincfg.Params) {
//
// This test ensures these blocks will respect the network PoW limit.
func checkGenesisBlockRespectsNetworkPowLimit(
t *testing.T, params chaincfg.Params) {
t *testing.T, params *chaincfg.Params) {
Copy link
Member

Choose a reason for hiding this comment

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

Please put this on the same line as the function def so it is consistent with the rest. Function definitions split on multiple lines causes some undesirable behavior with various tooling.

// The interval is mapped to corresponding interval in base 58.
// Then prefixes are checked for mismatch.
func checkInterval(t *testing.T,
desiredPrefix string, keySize int, networkName string, magic [2]byte) {
Copy link
Member

Choose a reason for hiding this comment

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

Again, same line for func defs.

func checkInterval(t *testing.T,
desiredPrefix string, keySize int, networkName string, magic [2]byte) {
// min and max possible keys
minKey := preFillArray(keySize, 0x00) // all zeroes
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use postfix comments. I know there are a couple of instances of it, but almost nothing in the codebase does and consistency is more important.

func checkPrefix(t *testing.T, prefix string, encoded, networkName string) {
if strings.Index(encoded, prefix) != 0 {
t.Logf(
"Address prefix mismatch for <%s>: expected <%s> received <%s>",
Copy link
Member

Choose a reason for hiding this comment

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

Don't put single params per line like this. It is not consistent with the rest of the code base.

}
}

func preFillArray(size int, value byte) []byte {
Copy link
Member

@davecgh davecgh Sep 26, 2018

Choose a reason for hiding this comment

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

This function is unnecessary. Use bytes.Repeat instead.

func checkPrefix(t *testing.T, prefix string, targetString, networkName string) {
if strings.Index(targetString, prefix) != 0 {
t.Logf("Address prefix mismatch for <%s>: expected <%s> received <%s>",
networkName, prefix, targetString,
Copy link
Member

Choose a reason for hiding this comment

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

Closing paren here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right.

@davecgh davecgh merged commit b6d564d into decred:master Sep 26, 2018
@davecgh
Copy link
Member

davecgh commented Sep 26, 2018

@JFixby I fixed it for you, but please note the code contribution guidelines for commits in the future. Your commit message was not adhering to them in terms of max width.

@JFixby
Copy link
Contributor Author

JFixby commented Sep 27, 2018

Thank you @davecgh.

I may also suggest making a pre-push checklist similar to preflight checklist.
Smth like:

  • keep all code lines width within 80-90 chars limit, function contracts (defs) are ok to be longer
  • no empty lines before the closing bracket of a function
  • no empty lines after the opening bracket of a function
  • don't use postfix comments
  • ensure all functions have comments
  • set commit message width below 80 chars
  • check PR title width is below 80 chars
  • set PR title package prefix

and so on.

@JFixby JFixby deleted the checkprefix branch September 27, 2018 08:16
@davecgh
Copy link
Member

davecgh commented Sep 29, 2018

@JFixby There is a Contribution Checklist in the code contribution guidelines. I believe all of this information is in that document.

If there is anything missing, I have no objections to PRs which add additional detail.

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