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.

Sign up for GitHub

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

Jump to bottom

/protocol organizational refactoring #183

Merged
merged 1 commit into from
Sep 27, 2017
Merged

/protocol organizational refactoring #183

merged 1 commit into from
Sep 27, 2017

Conversation

vqhuy
Copy link
Member

@vqhuy vqhuy commented Aug 22, 2017

Merge #170 & #182 first.

@vqhuy vqhuy added this to the 0.2.0 milestone Aug 22, 2017
@vqhuy vqhuy self-assigned this Aug 22, 2017
@vqhuy vqhuy force-pushed the refactor-protocol branch 2 times, most recently from f1952bb to 9c39ca2 Compare August 22, 2017 04:32
@masomel masomel added this to In Progress in Sprint: Nov 24 - Dec 7 Sep 1, 2017
@vqhuy vqhuy changed the base branch from generic-auditor to master September 3, 2017 20:10
@masomel masomel removed the blocked label Sep 21, 2017
masomel
masomel reviewed Sep 21, 2017
View reviewed changes
Copy link
Member

@masomel masomel left a comment

Choose a reason for hiding this comment

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

LGTM! I only have one minor nit pick.

protocol/directory/directory.go Outdated

import (
"bytes"

"github.com/coniks-sys/coniks-go/crypto/sign"
"github.com/coniks-sys/coniks-go/crypto/vrf"
"github.com/coniks-sys/coniks-go/merkletree"
p "github.com/coniks-sys/coniks-go/protocol"
Copy link
Member

Choose a reason for hiding this comment

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

Nit pick: We should probably be consistent with how we import some other coniks-go packages. We always import protocol as p which I think is fine, but merkletree sometimes is imported as m (e.g. https://github.com/coniks-sys/coniks-go/blob/master/protocol/consistencychecks.go#L13), and other times (like here) not. I think I've seen this with directory as well (dir vs the full name).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I am fixing this.

@masomel
Copy link
Member

masomel commented Sep 21, 2017

@c633 I went ahead and moved around the new test cases I added in #182 to make the tests pass again. There are, however, two tests in consistency_checks.go that no longer pass (TestVerifyRegistrationWithTB and TestVerifyFulfilledPromise). I don't know if you encountered this issue before.

@vqhuy vqhuy assigned vqhuy and unassigned vqhuy Sep 21, 2017
vqhuy
vqhuy commented Sep 22, 2017
View reviewed changes
protocol/client/consistencychecks.go

package protocol
package client
Copy link
Member Author

Choose a reason for hiding this comment

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

@masomel @liamsi @arlolra I would like to have your inputs about this name. It is collided with our application client package (see https://github.com/coniks-sys/coniks-go/pull/183/files#diff-50a5a7c3748e6d839ee365d7f4291a1cR13). A name I think of is verifier. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I think the auditor can also be considered a type of verifier, so I don't think this is the best name for this package.

How about we change the client CLI package name? (Btw, we'll have to find a name other than auditor for the auditor CLI, too). Can we call it something like client-bin or client-app?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about 4253664?

@vqhuy
Copy link
Member Author

vqhuy commented Sep 22, 2017

Our error codes are a mess for now. Another PR will follow up.

@vqhuy vqhuy changed the title [WIP] /protocol organizational refactor /protocol organizational refactor Sep 22, 2017
@vqhuy
Copy link
Member Author

vqhuy commented Sep 22, 2017

All tests are fixed. PTAL.

masomel
masomel reviewed Sep 22, 2017
View reviewed changes
Copy link
Member

@masomel masomel left a comment

Choose a reason for hiding this comment

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

LGTM again. Are we going to go back to using import . for the test cases that used to have cyclical dependencies?

@vqhuy
Copy link
Member Author

vqhuy commented Sep 23, 2017

Are we going to go back to using import . for the test cases that used to have cyclical dependencies?

I think yes.

@vqhuy vqhuy moved this from In Progress to In Review in Sprint: Nov 24 - Dec 7 Sep 23, 2017
masomel
masomel reviewed Sep 26, 2017
View reviewed changes
Copy link
Member

@masomel masomel left a comment

Choose a reason for hiding this comment

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

This is great :)

@masomel
Copy link
Member

masomel commented Sep 26, 2017 •
edited
Loading

In the spirit of refactoring: Is there an application layer API that we can refactor from the CLIs? I haven't spent much time looking into this, but it could be useful to have an application layer API for each component in the same way that we have a protocol level API for each. This could then help developers integrate things like a client with their existing applications. What do you think?

@vqhuy
Copy link
Member Author

vqhuy commented Sep 27, 2017

Is there an application layer API that we can refactor from the CLIs?

Yes, it is. Actually, every module that doesn't belong to cli package should be used to provide APIs for developers. We do have plan for this, please see #101, I think it belongs to milestone 0.3 and 0.5.

@vqhuy vqhuy changed the title /protocol organizational refactor /protocol organizational refactoring Sep 27, 2017
@vqhuy vqhuy merged commit f25a821 into master Sep 27, 2017
@vqhuy vqhuy deleted the refactor-protocol branch September 27, 2017 18:42
@vqhuy vqhuy moved this from In Review to Done in Sprint: Nov 24 - Dec 7 Sep 27, 2017
@masomel masomel removed this from Done in Sprint: Nov 24 - Dec 7 Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masomel masomel masomel approved these changes

@arlolra arlolra Awaiting requested review from arlolra

@liamsi liamsi Awaiting requested review from liamsi

Assignees

@vqhuy vqhuy

Labels
maintainability
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vqhuy @masomel