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

[FAB-17462] Create signature for config update #669

Merged
merged 1 commit into from Feb 14, 2020

Conversation

DereckLuo
Copy link
Contributor

Type of change

  • New feature

Description

This PR is part of an effort to create an easier way to create and update the channel configuration for fabric networks. This specific PR creates a signature for the provided configuration update.

Related issues

FAB-17467

@DereckLuo DereckLuo requested a review from a team as a code owner February 13, 2020 20:25
Copy link
Contributor

@ale-linux ale-linux left a comment

Choose a reason for hiding this comment

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

Thanks for this PR - a few nits from my side

pkg/config/signer.go Show resolved Hide resolved
pkg/config/signer_test.go Outdated Show resolved Hide resolved
pkg/config/signer.go Outdated Show resolved Hide resolved
pkg/config/signer.go Outdated Show resolved Hide resolved
pkg/config/signer.go Outdated Show resolved Hide resolved
pkg/config/config_test.go Outdated Show resolved Hide resolved
pkg/config/signer_test.go Outdated Show resolved Hide resolved
@wlahti
Copy link
Contributor

wlahti commented Feb 14, 2020

/ci-run

@hyperledger hyperledger deleted a comment from github-actions bot Feb 14, 2020
Signed-off-by: Chongxin Luo <Chongxin.Luo@ibm.com>
Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
Copy link
Contributor

@ale-linux ale-linux left a comment

Choose a reason for hiding this comment

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

You can add the missing test coverage from ./bccsp/utils/ecdsa.go - thx!

@ale-linux ale-linux merged commit 21724c4 into hyperledger:master Feb 14, 2020
Copy link
Contributor

@sykesm sykesm left a comment

Choose a reason for hiding this comment

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

I realize this has been merged but there are number of things that I think we need to improve before progressing to the next task.

pkg/config/config.go Show resolved Hide resolved
pkg/config/signer.go Show resolved Hide resolved
Comment on lines +28 to +35
var publicKey, privateKey []byte

func TestMain(m *testing.M) {
publicKey, privateKey = generatePublicAndPrivateKey()

os.Exit(m.Run())
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use package level state. Call the function to generate one when you need it. Do it in the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

// createNonce generates a nonce using the crypto/rand package.
func createNonce() ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, createNonce creates a nonce; it calls getRandomNonce that explicitly creates a 24 byte long slice. That length seems awfully specific...

If we want a reusable function, we need to make it reusable by having proper parameterization. We should either fold getRandomNonce into createNonce or rename getRandomNonce to reflect that it's getting random data of a certain length and pass in that length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pkg/config/signer.go Show resolved Hide resolved
pkg/config/signer.go Show resolved Hide resolved
pkg/config/config.go Show resolved Hide resolved
}

func TestECDSAPublicKeyImport(t *testing.T) {
t.Run("certificate does not contain valid ecdsa publicKey", func(t *testing.T) {
Copy link

@caod123 caod123 Feb 17, 2020

Choose a reason for hiding this comment

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

See comment above as well as for the ones below. If you want to annotate the test you can add the context to the matcher https://onsi.github.io/gomega/#annotating-assertions
t.Run is useful for sub-testing (table testing etc) and allows for finer grained control when trying to run a specific test from CLI. If there's only one test in the function you can still run that specific test via the CLI using the top level function name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@DereckLuo
Copy link
Contributor Author

fixes for additional requests has been applied in this PR: #687

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.

None yet

6 participants