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

Introduce signable, signature, pubkey, verifier, signer, and certific… #162

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@CoderZhi
Copy link
Collaborator

commented Oct 10, 2018

Also add uint test for these interfaces

@CoderZhi CoderZhi requested a review from iotexproject/protocol-team as a code owner Oct 10, 2018

@codecov

This comment has been minimized.

Copy link

commented Oct 10, 2018

Codecov Report

Merging #162 into master will increase coverage by 0.17%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
+ Coverage   68.84%   69.02%   +0.17%     
==========================================
  Files          83       84       +1     
  Lines        8980     9016      +36     
==========================================
+ Hits         6182     6223      +41     
+ Misses       2050     2042       -8     
- Partials      748      751       +3
Impacted Files Coverage Δ
crypto/certificate.go 83.33% <83.33%> (ø)
network/healthchecker.go 66.66% <0%> (+16.66%) ⬆️
network/pinger.go 50% <0%> (+37.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9dc6ab...8899958. Read the comment docs.

crypto/certificate.go Outdated

// VerifiedByAll checks whether the certificate contains the signatures of all these verifiers
// and only these verifiers
func (c *Certificate) VerifiedByAll(verifiers []Verifier) bool {

This comment has been minimized.

Copy link
@yutongp

yutongp Oct 10, 2018

Contributor

can we combine VerifiedBy and VerifiedByAll to VerifiedBy(verifiers ...Verifier)?

crypto/certificate.go Outdated
}

// Signature returns the signature of the verifier saved in this certificate
func (c *Certificate) Signature(pk PublicKey) Signature {

This comment has been minimized.

Copy link
@yutongp

yutongp Oct 10, 2018

Contributor

maybe func (c *Certificate) Signature(pk PublicKey) (Signature, bool)?

@zjshen14
Copy link
Contributor

left a comment

I'm somewhat concerned that we over killed the data model. PK and sig are very well defined, and hardly to be polymorphic. Can we do something like https://gist.github.com/zjshen14/020208400148bc80fcbee1a25e216c58

crypto/certificate.go Outdated

// Verifier defines an interface which could verify a signature
type Verifier interface {
Verify(signature Signature) bool

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Oct 10, 2018

Contributor

Why not Verify(sig, pk)?

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Oct 10, 2018

Author Collaborator

I assumed that the interface defined here contains the public key. What you suggested above is more like the interface of crypto algorithm

// Signer defines an interface which could sign a signable
type Signer interface {
Verifier
Sign(Signable) (Signature, error)

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Oct 10, 2018

Contributor

Sign(signable, sk)

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Oct 10, 2018

Author Collaborator

same here

crypto/certificate.go Outdated

// Signer defines an interface which could sign a signable
type Signer interface {
Verifier

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Oct 10, 2018

Contributor

Decouple?

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Oct 10, 2018

Author Collaborator

will do


// Certificate represents a certificate for a signable
type Certificate struct {
signable Signable

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Oct 10, 2018

Contributor

Instead of signable, how about hash Hash256/160

}

// Certificate represents a certificate for a signable
type Certificate struct {

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Oct 10, 2018

Contributor

Is it better one sig/pk per Certificate?

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Oct 10, 2018

Author Collaborator

In most cases, we will store a list of signatures for a given signable, thus the signable will be redundant data if we store one sig/pk per certificate.

@CoderZhi CoderZhi force-pushed the CoderZhi:signable_certificate branch 2 times, most recently Oct 10, 2018

@CoderZhi CoderZhi force-pushed the CoderZhi:signable_certificate branch to 8899958 Oct 11, 2018

@CoderZhi CoderZhi closed this Apr 25, 2019

@CoderZhi CoderZhi deleted the CoderZhi:signable_certificate branch Apr 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.