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

feat(keystore): keystore implmenetation #1602

Merged
merged 2 commits into from
Jan 20, 2021
Merged

feat(keystore): keystore implmenetation #1602

merged 2 commits into from
Jan 20, 2021

Conversation

Arqu
Copy link
Contributor

@Arqu Arqu commented Jan 14, 2021

No description provided.

@Arqu Arqu requested a review from b5 January 14, 2021 22:03
@Arqu Arqu self-assigned this Jan 14, 2021
key/keybook.go Outdated Show resolved Hide resolved
key/key.go Outdated Show resolved Hide resolved
key/store.go Outdated Show resolved Hide resolved
profile/store.go Outdated Show resolved Hide resolved
@Arqu Arqu force-pushed the arqu/keystore branch 4 times, most recently from 652bee2 to 86b30ce Compare January 15, 2021 20:30
@Arqu Arqu changed the title WIP: Keystore feat(keystore): keystore implmenetation Jan 15, 2021
@Arqu Arqu marked this pull request as ready for review January 15, 2021 20:32
@@ -143,6 +152,7 @@ func (p Profile) Encode() (*config.ProfilePod, error) {
for _, maddr := range p.NetworkAddrs {
addrs = append(addrs, maddr.String())
}
// TODO(aqru): see if we should also show pubkey/keyID 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.

@b5 This is still TODO before I merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dustmop any opinion on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a no? We have the profileID, and can use that to look up the key{public,id} from the keystore. No reason to store it twice, better to keep it normalized.

@Arqu Arqu requested a review from b5 January 15, 2021 20:33
key/store.go Show resolved Hide resolved
key/keybook.go Outdated Show resolved Hide resolved
key/keybook.go Outdated Show resolved Hide resolved
key/keybook_test.go Outdated Show resolved Hide resolved
key/keybook_test.go Outdated Show resolved Hide resolved
key/store.go Outdated Show resolved Hide resolved
key/store.go Outdated Show resolved Hide resolved
profile/store.go Outdated Show resolved Hide resolved
@Arqu Arqu force-pushed the arqu/keystore branch 3 times, most recently from 4c871c6 to c7a9a6c Compare January 18, 2021 12:07
@Arqu Arqu requested a review from dustmop January 18, 2021 12:28
Copy link
Contributor

@dustmop dustmop left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -143,6 +152,7 @@ func (p Profile) Encode() (*config.ProfilePod, error) {
for _, maddr := range p.NetworkAddrs {
addrs = append(addrs, maddr.String())
}
// TODO(aqru): see if we should also show pubkey/keyID here
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a no? We have the profileID, and can use that to look up the key{public,id} from the keystore. No reason to store it twice, better to keep it normalized.

@Arqu Arqu merged commit 205165a into master Jan 20, 2021
@Arqu Arqu deleted the arqu/keystore branch January 20, 2021 10:51
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